diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py b/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py index 833c74ab0d999..d257b263173ba 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py @@ -565,10 +565,6 @@ def foo( **kwargs } -( - *args -) - { *args } diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81_syntax_error.py b/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81_syntax_error.py new file mode 100644 index 0000000000000..16a9bbc121f44 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81_syntax_error.py @@ -0,0 +1,3 @@ +( + *args +) diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 960743e3e751a..11017d3a749eb 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -288,23 +288,30 @@ pub fn check_path( } } - // Remove fixes for any rules marked as unfixable. - for diagnostic in &mut diagnostics { - if !settings.rules.should_fix(diagnostic.kind.rule()) { - diagnostic.fix = None; + if parsed.is_valid() { + // Remove fixes for any rules marked as unfixable. + for diagnostic in &mut diagnostics { + 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() { - let fixed_applicability = settings - .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 diff --git a/crates/ruff_linter/src/rules/flake8_commas/mod.rs b/crates/ruff_linter/src/rules/flake8_commas/mod.rs index c7a274f1b3da7..1e4f88ca35568 100644 --- a/crates/ruff_linter/src/rules/flake8_commas/mod.rs +++ b/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( diff --git a/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap b/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap index 51b3ede78fff1..1955cf08d063f 100644 --- a/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap +++ b/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 - | ^ -570 | ) + | COM812 +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 | *args +577 | ham, +578 | spam, +579 | *args | COM812 -578 | ] +580 | ): +581 | pass | = help: Add trailing comma ℹ Safe fix -574 574 | } -575 575 | -576 576 | [ -577 |- *args - 577 |+ *args, -578 578 | ] -579 579 | -580 580 | def foo( - -COM81.py:583:10: COM812 [*] Trailing comma missing - | -581 | ham, -582 | spam, -583 | *args - | COM812 -584 | ): -585 | pass - | - = help: Add trailing comma +576 576 | def foo( +577 577 | ham, +578 578 | spam, +579 |- *args + 579 |+ *args, +580 580 | ): +581 581 | pass +582 582 | -ℹ 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 +COM81.py:586:13: COM812 [*] Trailing comma missing | -588 | ham, -589 | spam, -590 | **kwargs +584 | ham, +585 | spam, +586 | **kwargs | COM812 -591 | ): -592 | pass +587 | ): +588 | pass | = help: Add trailing comma ℹ Safe fix -587 587 | def foo( -588 588 | ham, -589 589 | spam, -590 |- **kwargs - 590 |+ **kwargs, -591 591 | ): -592 592 | pass -593 593 | +583 583 | def foo( +584 584 | ham, +585 585 | spam, +586 |- **kwargs + 586 |+ **kwargs, +587 587 | ): +588 588 | pass +589 589 | -COM81.py:598:15: COM812 [*] Trailing comma missing +COM81.py:594:15: COM812 [*] Trailing comma missing | -596 | spam, -597 | *args, -598 | kwarg_only +592 | spam, +593 | *args, +594 | kwarg_only | COM812 -599 | ): -600 | pass +595 | ): +596 | pass | = help: Add trailing comma ℹ Safe fix -595 595 | ham, -596 596 | spam, -597 597 | *args, -598 |- kwarg_only - 598 |+ kwarg_only, -599 599 | ): -600 600 | pass -601 601 | +591 591 | ham, +592 592 | spam, +593 593 | *args, +594 |- kwarg_only + 594 |+ kwarg_only, +595 595 | ): +596 596 | pass +597 597 | -COM81.py:627:20: COM812 [*] Trailing comma missing +COM81.py:623:20: COM812 [*] Trailing comma missing | -625 | foo, -626 | bar, -627 | **{'ham': spam} +621 | foo, +622 | bar, +623 | **{'ham': spam} | COM812 -628 | ) +624 | ) | = help: Add trailing comma ℹ Safe fix -624 624 | result = function( -625 625 | foo, -626 626 | bar, -627 |- **{'ham': spam} - 627 |+ **{'ham': spam}, -628 628 | ) -629 629 | -630 630 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error. +620 620 | result = function( +621 621 | foo, +622 622 | bar, +623 |- **{'ham': spam} + 623 |+ **{'ham': spam}, +624 624 | ) +625 625 | +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. -631 | the_first_one = next( -632 | (i for i in range(10) if i // 2 == 0) # COM812 fix should include the final bracket +626 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error. +627 | the_first_one = next( +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 | -630 630 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error. -631 631 | the_first_one = next( -632 |- (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 -633 633 | ) -634 634 | -635 635 | foo = namedtuple( +625 625 | +626 626 | # Make sure the COM812 and UP034 rules don't fix simultaneously and cause a syntax error. +627 627 | the_first_one = next( +628 |- (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 +629 629 | ) +630 630 | +631 631 | foo = namedtuple( -COM81.py:644:46: COM819 [*] Trailing comma prohibited +COM81.py:640:46: COM819 [*] Trailing comma prohibited | -643 | # F-strings -644 | kwargs.pop("remove", f"this {trailing_comma}",) +639 | # F-strings +640 | kwargs.pop("remove", f"this {trailing_comma}",) | ^ COM819 -645 | -646 | raise Exception( +641 | +642 | raise Exception( | = help: Remove trailing comma ℹ Safe fix -641 641 | ) -642 642 | -643 643 | # F-strings -644 |-kwargs.pop("remove", f"this {trailing_comma}",) - 644 |+kwargs.pop("remove", f"this {trailing_comma}") -645 645 | -646 646 | raise Exception( -647 647 | "first", extra=f"Add trailing comma here ->" +637 637 | ) +638 638 | +639 639 | # F-strings +640 |-kwargs.pop("remove", f"this {trailing_comma}",) + 640 |+kwargs.pop("remove", f"this {trailing_comma}") +641 641 | +642 642 | raise Exception( +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( -647 | "first", extra=f"Add trailing comma here ->" +642 | raise Exception( +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( -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" +646 646 | assert False, f"<- This is not a trailing comma" diff --git a/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81_syntax_error.py.snap b/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81_syntax_error.py.snap new file mode 100644 index 0000000000000..d33492fb6bf23 --- /dev/null +++ b/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 | ) + | diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index 55a259ff4fe90..a4ac856499b7b 100644 --- a/crates/ruff_linter/src/test.rs +++ b/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( - parsed - .errors() - .iter() - .map(|parse_error| { - Message::from_parse_error(parse_error, &locator, source_code.clone()) - }) - ) + .chain(parsed.errors().iter().map(|parse_error| { + Message::from_parse_error(parse_error, &locator, source_code.clone()) + })) .sorted() .collect(); (messages, transformed)