From 7d6889b5d903f554cc8aae1a361443e53aab4e2f Mon Sep 17 00:00:00 2001 From: Alexander Saites Date: Tue, 29 Oct 2024 20:33:06 -0700 Subject: [PATCH] replace special-case duplicate handling with error --- crates/rustfix/src/error.rs | 10 ++++-- crates/rustfix/src/lib.rs | 37 ++++++++++++-------- crates/rustfix/src/replace.rs | 65 +++++++++++++++++++++++------------ src/cargo/ops/fix.rs | 24 ++++++------- 4 files changed, 84 insertions(+), 52 deletions(-) diff --git a/crates/rustfix/src/error.rs b/crates/rustfix/src/error.rs index 51d9a1191b0..cb1d724cc22 100644 --- a/crates/rustfix/src/error.rs +++ b/crates/rustfix/src/error.rs @@ -11,9 +11,15 @@ pub enum Error { #[error("invalid range {0:?}, original data is only {1} byte long")] DataLengthExceeded(Range, usize), - #[non_exhaustive] // There are plans to add fields to this variant at a later time. + #[non_exhaustive] #[error("cannot replace slice of data that was already replaced")] - AlreadyReplaced, + AlreadyReplaced { + /// The location of the intended replacement. + range: Range, + /// Whether the modification exactly matches (both range and data) the one it conflicts with. + /// Some clients may wish to simply ignore this condition. + is_identical: bool, + }, #[error(transparent)] Utf8(#[from] std::string::FromUtf8Error), diff --git a/crates/rustfix/src/lib.rs b/crates/rustfix/src/lib.rs index 41b4d7affe2..ce72127fa2d 100644 --- a/crates/rustfix/src/lib.rs +++ b/crates/rustfix/src/lib.rs @@ -234,8 +234,14 @@ impl CodeFix { /// Applies a suggestion to the code. pub fn apply(&mut self, suggestion: &Suggestion) -> Result<(), Error> { for solution in &suggestion.solutions { - self.apply_solution(solution)?; + for r in &solution.replacements { + self.data + .replace_range(r.snippet.range.clone(), r.replacement.as_bytes()) + .inspect_err(|_| self.data.restore())?; + } } + self.data.commit(); + self.modified = true; Ok(()) } @@ -262,22 +268,25 @@ impl CodeFix { } } -/// Applies multiple `suggestions` to the given `code`. +/// Applies multiple `suggestions` to the given `code`, handling certain conflicts automatically. +/// +/// If a replacement in a suggestion exactly matches a replacement of a previously applied solution, +/// that entire suggestion will be skipped without generating an error. +/// This is currently done to alleviate issues like rust-lang/rust#51211, +/// although it may be removed if that's fixed deeper in the compiler. +/// +/// The intent of this design is that the overall application process +/// should repeatedly apply non-conflicting suggestions then rëevaluate the result, +/// looping until either there are no more suggestions to apply or some budget is exhausted. pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result { - let mut already_applied = HashSet::new(); let mut fix = CodeFix::new(code); for suggestion in suggestions.iter().rev() { - // This assumes that if any of the machine applicable fixes in - // a diagnostic suggestion is a duplicate, we should see the - // entire suggestion as a duplicate. - if suggestion - .solutions - .iter() - .any(|sol| !already_applied.insert(sol)) - { - continue; - } - fix.apply(suggestion)?; + fix.apply(suggestion).or_else(|err| match err { + Error::AlreadyReplaced { + is_identical: true, .. + } => Ok(()), + _ => Err(err), + })?; } fix.finish() } diff --git a/crates/rustfix/src/replace.rs b/crates/rustfix/src/replace.rs index e4230a56b68..3eb130203d0 100644 --- a/crates/rustfix/src/replace.rs +++ b/crates/rustfix/src/replace.rs @@ -169,26 +169,21 @@ impl Data { // Reject if the change starts before the previous one ends. if let Some(before) = ins_point.checked_sub(1).and_then(|i| self.parts.get(i)) { if incoming.range.start < before.range.end { - return Err(Error::AlreadyReplaced); + return Err(Error::AlreadyReplaced { + is_identical: incoming == *before, + range: incoming.range, + }); } } - // Reject if the change ends after the next one starts. + // Reject if the change ends after the next one starts, + // or if this is an insert and there's already an insert there. if let Some(after) = self.parts.get(ins_point) { - // If this replacement matches exactly the part that we would - // otherwise split then we ignore this for now. This means that you - // can replace the exact same range with the exact same content - // multiple times and we'll process and allow it. - // - // This is currently done to alleviate issues like - // rust-lang/rust#51211 although this clause likely wants to be - // removed if that's fixed deeper in the compiler. - if incoming == *after { - return Ok(()); - } - - if incoming.range.end > after.range.start { - return Err(Error::AlreadyReplaced); + if incoming.range.end > after.range.start || incoming.range == after.range { + return Err(Error::AlreadyReplaced { + is_identical: incoming == *after, + range: incoming.range, + }); } } @@ -274,7 +269,7 @@ mod tests { } #[test] - fn replace_overlapping_stuff_errs() { + fn replace_same_range_diff_data() { let mut d = Data::new(b"foo bar baz"); d.replace_range(4..7, b"lol").unwrap(); @@ -282,7 +277,26 @@ mod tests { assert!(matches!( d.replace_range(4..7, b"lol2").unwrap_err(), - Error::AlreadyReplaced, + Error::AlreadyReplaced { + is_identical: false, + .. + }, + )); + } + + #[test] + fn replace_same_range_same_data() { + let mut d = Data::new(b"foo bar baz"); + + d.replace_range(4..7, b"lol").unwrap(); + assert_eq!("foo lol baz", str(&d.to_vec())); + + assert!(matches!( + d.replace_range(4..7, b"lol").unwrap_err(), + Error::AlreadyReplaced { + is_identical: true, + .. + }, )); } @@ -296,11 +310,18 @@ mod tests { } #[test] - fn replace_same_twice() { + fn insert_same_twice() { let mut d = Data::new(b"foo"); - d.replace_range(0..1, b"b").unwrap(); - d.replace_range(0..1, b"b").unwrap(); - assert_eq!("boo", str(&d.to_vec())); + d.replace_range(1..1, b"b").unwrap(); + assert_eq!("fboo", str(&d.to_vec())); + assert!(matches!( + d.replace_range(1..1, b"b").unwrap_err(), + Error::AlreadyReplaced { + is_identical: true, + .. + }, + )); + assert_eq!("fboo", str(&d.to_vec())); } #[test] diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index ba3c9f2666c..a5b94919240 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -968,23 +968,19 @@ fn rustfix_and_fix( }); let mut fixed = CodeFix::new(&code); - let mut already_applied = HashSet::new(); for suggestion in suggestions.iter().rev() { - // This assumes that if any of the machine applicable fixes in - // a diagnostic suggestion is a duplicate, we should see the - // entire suggestion as a duplicate. - if suggestion - .solutions - .iter() - .any(|sol| !already_applied.insert(sol)) - { - continue; - } + // As mentioned above in `rustfix_crate`, + // we don't immediately warn about suggestions that fail to apply here, + // and instead we save them off for later processing. + // + // However, we don't bother reporting conflicts that exactly match prior replacements. + // This is currently done to reduce noise for things like rust-lang/rust#51211, + // although it may be removed if that's fixed deeper in the compiler. match fixed.apply(suggestion) { Ok(()) => fixed_file.fixes_applied += 1, - // As mentioned above in `rustfix_crate`, we don't immediately - // warn about suggestions that fail to apply here, and instead - // we save them off for later processing. + Err(rustfix::Error::AlreadyReplaced { + is_identical: true, .. + }) => continue, Err(e) => fixed_file.errors_applying_fixes.push(e.to_string()), } }