Auto merge of #14782 - saites:rustfix-duplicate-errors, r=weihanglo

rustfix: replace special-case duplicate handling with error

### What does this PR try to resolve?

This PR changes how conflicts are handled in `rustfix`.

By design, `cargo fix` repeatedly compiles the code, collects suggested changes, and applies changes that don't conflict with one another. Often, conflicts arise because the same suggestion is reported multiple times e.g., due to a macro expansion. There have been previous changes to address that specific case, but following [the PR to add transactional semantics](#14747), it makes sense to change how this is handled.

Specifically, this PR adds details to `Error::AlreadyReplaced` to indicate whether the specific replacement is identical to the one it conflicts with, allowing callers to handle this case, rather than special-casing it within `replace.rs` itself. To that point, this PR changes `fix.rs` and `lib.rs` to handle identical replacements by skipping the conflicting suggestion. This is not exactly identical to their previous behavior: before, it skipped a suggestion if any _solution_ had been applied, whereas this skips it if any _replacement within a solution_ has been applied. While more expansive, this is very much the spirit of the goal described above.

### How should we test and review this PR?

The existing tests have been updated to account for this change.

### Additional information

See: #13027

Author: bors

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>, 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<usize>,
+        /// 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),

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<String, Error> {
-    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()
 }

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,15 +269,34 @@ 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();
         assert_eq!("foo lol baz", str(&d.to_vec()));
 
         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]

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()),
             }
         }