Disable auto-fix when source has syntax errors (#12134)

## Summary

This PR updates Ruff to **not** generate auto-fixes if the source code
contains syntax errors as determined by the parser.

The main motivation behind this is to avoid infinite autofix loop when
the token-based rules are run over any source with syntax errors in
#11950.

Although even after this, it's not certain that there won't be an
infinite autofix loop because the logic might be incorrect. For example,
https://github.com/astral-sh/ruff/issues/12094 and
https://github.com/astral-sh/ruff/pull/12136.

This requires updating the test infrastructure to not validate for fix
availability status when the source contained syntax errors. This is
required because otherwise the fuzzer might fail as it uses the test
function to run the linter and validate the source code.

resolves: #11455 

## Test Plan

`cargo insta test`

crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py

@@ -565,10 +565,6 @@ foo(
     **kwargs
 }
 
-(
-    *args
-)
-
 {
     *args
 }

crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81_syntax_error.py

@@ -0,0 +1,3 @@
+(
+    *args
+)

crates/ruff_linter/src/linter.rs

@@ -288,23 +288,30 @@ pub fn check_path(
         }
     }
 
-    // Remove fixes for any rules marked as unfixable.
+    if parsed.is_valid() {
-    for diagnostic in &mut diagnostics {
+        // Remove fixes for any rules marked as unfixable.
-        if !settings.rules.should_fix(diagnostic.kind.rule()) {
-            diagnostic.fix = None;
-        }
-    }
-
-    // Update fix applicability to account for overrides
-    if !settings.fix_safety.is_empty() {
         for diagnostic in &mut diagnostics {
-            if let Some(fix) = diagnostic.fix.take() {
+            if !settings.rules.should_fix(diagnostic.kind.rule()) {
-                let fixed_applicability = settings
+                diagnostic.fix = None;
-                    .fix_safety
-                    .resolve_applicability(diagnostic.kind.rule(), fix.applicability());
-                diagnostic.set_fix(fix.with_applicability(fixed_applicability));
             }
         }
+
+        // Update fix applicability to account for overrides
+        if !settings.fix_safety.is_empty() {
+            for diagnostic in &mut diagnostics {
+                if let Some(fix) = diagnostic.fix.take() {
+                    let fixed_applicability = settings
+                        .fix_safety
+                        .resolve_applicability(diagnostic.kind.rule(), fix.applicability());
+                    diagnostic.set_fix(fix.with_applicability(fixed_applicability));
+                }
+            }
+        }
+    } else {
+        // Avoid fixing in case the source code contains syntax errors.
+        for diagnostic in &mut diagnostics {
+            diagnostic.fix = None;
+        }
     }
 
     diagnostics

crates/ruff_linter/src/rules/flake8_commas/mod.rs

@@ -13,6 +13,7 @@ mod tests {
     use crate::{assert_messages, settings};
 
     #[test_case(Path::new("COM81.py"))]
+    #[test_case(Path::new("COM81_syntax_error.py"))]
     fn rules(path: &Path) -> Result<()> {
         let snapshot = path.to_string_lossy().into_owned();
         let diagnostics = test_path(

crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap

@@ -796,192 +796,184 @@ COM81.py:565:13: COM812 [*] Trailing comma missing
     565 |+    **kwargs,
 566 566 | }
 567 567 | 
-568 568 | (
+568 568 | {
 
-COM81.py:569:5: SyntaxError: Starred expression cannot be used here
+COM81.py:569:10: COM812 [*] Trailing comma missing
     |
-568 | (
+568 | {
 569 |     *args
-    |     ^
+    |           COM812
-570 | )
+570 | }
     |
+    = help: Add trailing comma
+
+ℹ Safe fix
+566 566 | }
+567 567 | 
+568 568 | {
+569     |-    *args
+    569 |+    *args,
+570 570 | }
+571 571 | 
+572 572 | [
 
 COM81.py:573:10: COM812 [*] Trailing comma missing
     |
-572 | {
+572 | [
 573 |     *args
     |           COM812
-574 | }
+574 | ]
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-570 570 | )
+570 570 | }
 571 571 | 
-572 572 | {
+572 572 | [
 573     |-    *args
     573 |+    *args,
-574 574 | }
+574 574 | ]
 575 575 | 
-576 576 | [
+576 576 | def foo(
 
-COM81.py:577:10: COM812 [*] Trailing comma missing
+COM81.py:579:10: COM812 [*] Trailing comma missing
     |
-576 | [
+577 |     ham,
-577 |     *args
+578 |     spam,
+579 |     *args
     |           COM812
-578 | ]
+580 |     ):
+581 |     pass
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-574 574 | }
+576 576 | def foo(
-575 575 | 
+577 577 |     ham,
-576 576 | [
+578 578 |     spam,
-577     |-    *args
+579     |-    *args
-    577 |+    *args,
+    579 |+    *args,
-578 578 | ]
+580 580 |     ):
-579 579 | 
+581 581 |     pass
-580 580 | def foo(
+582 582 | 
 
-COM81.py:583:10: COM812 [*] Trailing comma missing
+COM81.py:586:13: COM812 [*] Trailing comma missing
     |
-581 |     ham,
+584 |     ham,
-582 |     spam,
+585 |     spam,
-583 |     *args
+586 |     **kwargs
-    |           COM812
-584 |     ):
-585 |     pass
-    |
-    = help: Add trailing comma
-
-ℹ Safe fix
-580 580 | def foo(
-581 581 |     ham,
-582 582 |     spam,
-583     |-    *args
-    583 |+    *args,
-584 584 |     ):
-585 585 |     pass
-586 586 | 
-
-COM81.py:590:13: COM812 [*] Trailing comma missing
-    |
-588 |     ham,
-589 |     spam,
-590 |     **kwargs
     |              COM812
-591 |     ):
+587 |     ):
-592 |     pass
+588 |     pass
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-587 587 | def foo(
+583 583 | def foo(
-588 588 |     ham,
+584 584 |     ham,
-589 589 |     spam,
+585 585 |     spam,
-590     |-    **kwargs
+586     |-    **kwargs
-    590 |+    **kwargs,
+    586 |+    **kwargs,
-591 591 |     ):
+587 587 |     ):
-592 592 |     pass
+588 588 |     pass
-593 593 | 
+589 589 | 
 
-COM81.py:598:15: COM812 [*] Trailing comma missing
+COM81.py:594:15: COM812 [*] Trailing comma missing
     |
-596 |     spam,
+592 |     spam,
-597 |     *args,
+593 |     *args,
-598 |     kwarg_only
+594 |     kwarg_only
     |                COM812
-599 |     ):
+595 |     ):
-600 |     pass
+596 |     pass
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-595 595 |     ham,
+591 591 |     ham,
-596 596 |     spam,
+592 592 |     spam,
-597 597 |     *args,
+593 593 |     *args,
-598     |-    kwarg_only
+594     |-    kwarg_only
-    598 |+    kwarg_only,
+    594 |+    kwarg_only,
-599 599 |     ):
+595 595 |     ):
-600 600 |     pass
+596 596 |     pass
-601 601 | 
+597 597 | 
 
-COM81.py:627:20: COM812 [*] Trailing comma missing
+COM81.py:623:20: COM812 [*] Trailing comma missing
     |
-625 |     foo,
+621 |     foo,
-626 |     bar,
+622 |     bar,
-627 |     **{'ham': spam}
+623 |     **{'ham': spam}
     |                     COM812
-628 | )
+624 | )
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-624 624 | result = function(
+620 620 | result = function(
-625 625 |     foo,
+621 621 |     foo,
-626 626 |     bar,
+622 622 |     bar,
-627     |-    **{'ham': spam}
+623     |-    **{'ham': spam}
-    627 |+    **{'ham': spam},
+    623 |+    **{'ham': spam},
-628 628 | )
+624 624 | )
-629 629 | 
+625 625 | 
-630 630 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error.
+626 626 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error.
 
-COM81.py:632:42: COM812 [*] Trailing comma missing
+COM81.py:628:42: COM812 [*] Trailing comma missing
     |
-630 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error.
+626 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error.
-631 | the_first_one = next(
+627 | the_first_one = next(
-632 |     (i for i in range(10) if i // 2 == 0)  # COM812 fix should include the final bracket
+628 |     (i for i in range(10) if i // 2 == 0)  # COM812 fix should include the final bracket
     |                                           COM812
-633 | )
+629 | )
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-629 629 | 
+625 625 | 
-630 630 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error.
+626 626 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error.
-631 631 | the_first_one = next(
+627 627 | the_first_one = next(
-632     |-    (i for i in range(10) if i // 2 == 0)  # COM812 fix should include the final bracket
+628     |-    (i for i in range(10) if i // 2 == 0)  # COM812 fix should include the final bracket
-    632 |+    (i for i in range(10) if i // 2 == 0),  # COM812 fix should include the final bracket
+    628 |+    (i for i in range(10) if i // 2 == 0),  # COM812 fix should include the final bracket
-633 633 | )
+629 629 | )
-634 634 | 
+630 630 | 
-635 635 | foo = namedtuple(
+631 631 | foo = namedtuple(
 
-COM81.py:644:46: COM819 [*] Trailing comma prohibited
+COM81.py:640:46: COM819 [*] Trailing comma prohibited
     |
-643 | # F-strings
+639 | # F-strings
-644 | kwargs.pop("remove", f"this {trailing_comma}",)
+640 | kwargs.pop("remove", f"this {trailing_comma}",)
     |                                              ^ COM819
-645 | 
+641 | 
-646 | raise Exception(
+642 | raise Exception(
     |
     = help: Remove trailing comma
 
 ℹ Safe fix
-641 641 | )
+637 637 | )
-642 642 | 
+638 638 | 
-643 643 | # F-strings
+639 639 | # F-strings
-644     |-kwargs.pop("remove", f"this {trailing_comma}",)
+640     |-kwargs.pop("remove", f"this {trailing_comma}",)
-    644 |+kwargs.pop("remove", f"this {trailing_comma}")
+    640 |+kwargs.pop("remove", f"this {trailing_comma}")
-645 645 | 
+641 641 | 
-646 646 | raise Exception(
+642 642 | raise Exception(
-647 647 |     "first", extra=f"Add trailing comma here ->"
+643 643 |     "first", extra=f"Add trailing comma here ->"
 
-COM81.py:647:49: COM812 [*] Trailing comma missing
+COM81.py:643:49: COM812 [*] Trailing comma missing
     |
-646 | raise Exception(
+642 | raise Exception(
-647 |     "first", extra=f"Add trailing comma here ->"
+643 |     "first", extra=f"Add trailing comma here ->"
     |                                                  COM812
-648 | )
+644 | )
     |
     = help: Add trailing comma
 
 ℹ Safe fix
-644 644 | kwargs.pop("remove", f"this {trailing_comma}",)
+640 640 | kwargs.pop("remove", f"this {trailing_comma}",)
+641 641 | 
+642 642 | raise Exception(
+643     |-    "first", extra=f"Add trailing comma here ->"
+    643 |+    "first", extra=f"Add trailing comma here ->",
+644 644 | )
 645 645 | 
-646 646 | raise Exception(
+646 646 | assert False, f"<- This is not a trailing comma"
-647     |-    "first", extra=f"Add trailing comma here ->"
-    647 |+    "first", extra=f"Add trailing comma here ->",
-648 648 | )
-649 649 | 
-650 650 | assert False, f"<- This is not a trailing comma"

crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81_syntax_error.py.snap

@@ -0,0 +1,10 @@
+---
+source: crates/ruff_linter/src/rules/flake8_commas/mod.rs
+---
+COM81_syntax_error.py:2:5: SyntaxError: Starred expression cannot be used here
+  |
+1 | (
+2 |     *args
+  |     ^
+3 | )
+  |

crates/ruff_linter/src/test.rs

@@ -204,12 +204,12 @@ pub(crate) fn test_contents<'a>(
                     print_syntax_errors(parsed.errors(), path, &locator, &transformed);
 
                 panic!(
-                    r#"Fixed source has a syntax error where the source document does not. This is a bug in one of the generated fixes:
+                    "Fixed source has a syntax error where the source document does not. This is a bug in one of the generated fixes:
 {syntax_errors}
 Last generated fixes:
 {fixes}
 Source with applied fixes:
-{}"#,
+{}",
                     transformed.source_code()
                 );
             }
@@ -228,7 +228,12 @@ Source with applied fixes:
         .into_iter()
         .map(|diagnostic| {
             let rule = diagnostic.kind.rule();
-            let fixable = diagnostic.fix.as_ref().is_some_and(|fix| matches!(fix.applicability(), Applicability::Safe | Applicability::Unsafe));
+            let fixable = diagnostic.fix.as_ref().is_some_and(|fix| {
+                matches!(
+                    fix.applicability(),
+                    Applicability::Safe | Applicability::Unsafe
+                )
+            });
 
             match (fixable, rule.fixable()) {
                 (true, FixAvailability::Sometimes | FixAvailability::Always)
@@ -236,28 +241,39 @@ Source with applied fixes:
                     // Ok
                 }
                 (true, FixAvailability::None) => {
-                    panic!("Rule {rule:?} is marked as non-fixable but it created a fix. Change the `Violation::FIX_AVAILABILITY` to either `FixAvailability::Sometimes` or `FixAvailability::Always`");
+                    panic!(
-                },
+                        "Rule {rule:?} is marked as non-fixable but it created a fix.
+Change the `Violation::FIX_AVAILABILITY` to either \
+`FixAvailability::Sometimes` or `FixAvailability::Always`"
+                    );
+                }
+                (false, FixAvailability::Always) if source_has_errors => {
+                    // Ok
+                }
                 (false, FixAvailability::Always) => {
-                    panic!("Rule {rule:?} is marked to always-fixable but the diagnostic has no fix. Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to either `FixAvailability::Sometimes` or `FixAvailability::None")
+                    panic!(
+                        "\
+Rule {rule:?} is marked to always-fixable but the diagnostic has no fix.
+Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to either \
+`FixAvailability::Sometimes` or `FixAvailability::None`"
+                    )
                 }
             }
 
-            assert!(!(fixable && diagnostic.kind.suggestion.is_none()), "Diagnostic emitted by {rule:?} is fixable but `Violation::fix_title` returns `None`.`");
+            assert!(
+                !(fixable && diagnostic.kind.suggestion.is_none()),
+                "Diagnostic emitted by {rule:?} is fixable but \
+                `Violation::fix_title` returns `None`"
+            );
 
             // Not strictly necessary but adds some coverage for this code path
             let noqa = directives.noqa_line_for.resolve(diagnostic.start());
 
             Message::from_diagnostic(diagnostic, source_code.clone(), noqa)
         })
-        .chain(
+        .chain(parsed.errors().iter().map(|parse_error| {
-            parsed
+            Message::from_parse_error(parse_error, &locator, source_code.clone())
-                .errors()
+        }))
-                .iter()
-                .map(|parse_error| {
-                    Message::from_parse_error(parse_error, &locator, source_code.clone())
-                })
-        )
         .sorted()
         .collect();
     (messages, transformed)