From fa3a8e68e625e035cab14209489f69382721f5db Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 11:52:00 -0600 Subject: [PATCH 01/17] Add internal hidden rules for testing --- crates/ruff/Cargo.toml | 2 + crates/ruff/tests/integration_test.rs | 347 +++++++++--------- crates/ruff_linter/Cargo.toml | 2 + crates/ruff_linter/src/codes.rs | 14 + crates/ruff_linter/src/linter.rs | 98 ++++- crates/ruff_linter/src/rule_selector.rs | 2 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 4 + .../src/rules/ruff/rules/test_rules.rs | 203 ++++++++++ crates/ruff_workspace/Cargo.toml | 2 + crates/ruff_workspace/src/configuration.rs | 2 + 10 files changed, 491 insertions(+), 185 deletions(-) create mode 100644 crates/ruff_linter/src/rules/ruff/rules/test_rules.rs diff --git a/crates/ruff/Cargo.toml b/crates/ruff/Cargo.toml index 71d1dce4097ff1..0f54993d28da99 100644 --- a/crates/ruff/Cargo.toml +++ b/crates/ruff/Cargo.toml @@ -54,6 +54,8 @@ walkdir = { workspace = true } wild = { workspace = true } [dev-dependencies] +# Enable test rules during development +ruff_linter = { path = "../ruff_linter", features = ["clap", "test-rules"] } assert_cmd = { workspace = true } # Avoid writing colored snapshots when running tests from the terminal colored = { workspace = true, features = ["no-color"]} diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 7aed76fee2df3b..afe71d41ba2434 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -799,15 +799,18 @@ fn show_statistics() { #[test] fn nursery_prefix() { - // `--select E` should detect E741, but not E225, which is in the nursery. - let mut cmd = RuffCheck::default().args(["--select", "E"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + // Should only detect RUF900, but not the unstable test rules + let mut cmd = RuffCheck::default().args(["--select", "RUF9"]).build(); + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:1: E741 Ambiguous variable name: `I` - Found 1 error. + -:1:1: RUF900 Hey this is a stable test rule. + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. + Found 4 errors. + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- "###); @@ -815,16 +818,19 @@ fn nursery_prefix() { #[test] fn nursery_all() { - // `--select ALL` should detect E741, but not E225, which is in the nursery. + // Should detect RUF900, but not the unstable test rules let mut cmd = RuffCheck::default().args(["--select", "ALL"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:1: E741 Ambiguous variable name: `I` -:1:1: D100 Missing docstring in public module - Found 2 errors. + -:1:1: RUF900 Hey this is a stable test rule. + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. + Found 5 errors. + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`. @@ -834,35 +840,32 @@ fn nursery_all() { #[test] fn nursery_direct() { - // `--select E225` should detect E225. - let mut cmd = RuffCheck::default().args(["--select", "E225"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + // Should warn that the nursery rule is selected without preview flag but still + // include the diagnostic + let mut cmd = RuffCheck::default().args(["--select", "RUF912"]).build(); + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:2: E225 [*] Missing whitespace around operator + -:1:1: RUF912 Hey this is a nursery test rule. Found 1 error. - [*] 1 fixable with the `--fix` option. ----- stderr ----- - warning: Selection of nursery rule `E225` without the `--preview` flag is deprecated. + warning: Selection of nursery rule `RUF912` without the `--preview` flag is deprecated. "###); } #[test] fn nursery_group_selector() { - // Only nursery rules should be detected e.g. E225 and a warning should be displayed + // Only nursery rules should be detected e.g. RUF912 let mut cmd = RuffCheck::default().args(["--select", "NURSERY"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- -:1:1: CPY001 Missing copyright notice at top of file - -:1:2: E225 [*] Missing whitespace around operator + -:1:1: RUF912 Hey this is a nursery test rule. Found 2 errors. - [*] 1 fixable with the `--fix` option. ----- stderr ----- warning: The `NURSERY` selector has been deprecated. Use the `--preview` flag instead. @@ -871,12 +874,11 @@ fn nursery_group_selector() { #[test] fn nursery_group_selector_preview_enabled() { - // Only nursery rules should be detected e.g. E225 and a warning should be displayed + // A warning should be displayed due to deprecated selector usage let mut cmd = RuffCheck::default() .args(["--select", "NURSERY", "--preview"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 2 ----- stdout ----- @@ -889,19 +891,22 @@ fn nursery_group_selector_preview_enabled() { #[test] fn preview_enabled_prefix() { - // E741 and E225 (preview) should both be detected + // All the RUF9XX test rules should be triggered let mut cmd = RuffCheck::default() - .args(["--select", "E", "--preview"]) + .args(["--select", "RUF9", "--preview"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:1: E741 Ambiguous variable name: `I` - -:1:2: E225 [*] Missing whitespace around operator - Found 2 errors. - [*] 1 fixable with the `--fix` option. + -:1:1: RUF900 Hey this is a stable test rule. + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. + -:1:1: RUF911 Hey this is a preview test rule. + -:1:1: RUF912 Hey this is a nursery test rule. + Found 6 errors. + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- "###); @@ -912,17 +917,20 @@ fn preview_enabled_all() { let mut cmd = RuffCheck::default() .args(["--select", "ALL", "--preview"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:1: E741 Ambiguous variable name: `I` -:1:1: D100 Missing docstring in public module -:1:1: CPY001 Missing copyright notice at top of file - -:1:2: E225 [*] Missing whitespace around operator - Found 4 errors. - [*] 1 fixable with the `--fix` option. + -:1:1: RUF900 Hey this is a stable test rule. + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. + -:1:1: RUF911 Hey this is a preview test rule. + -:1:1: RUF912 Hey this is a nursery test rule. + Found 8 errors. + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`. @@ -932,18 +940,16 @@ fn preview_enabled_all() { #[test] fn preview_enabled_direct() { - // E225 should be detected without warning + // Should be enabled without warning let mut cmd = RuffCheck::default() - .args(["--select", "E225", "--preview"]) + .args(["--select", "RUF911", "--preview"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:2: E225 [*] Missing whitespace around operator + -:1:1: RUF911 Hey this is a preview test rule. Found 1 error. - [*] 1 fixable with the `--fix` option. ----- stderr ----- "###); @@ -951,45 +957,40 @@ fn preview_enabled_direct() { #[test] fn preview_disabled_direct() { - // FURB145 is preview not nursery so selecting should be empty - let mut cmd = RuffCheck::default().args(["--select", "FURB145"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("a = l[:]\n"), @r###" + // RUFF911 is preview not nursery so the selection should be empty + let mut cmd = RuffCheck::default().args(["--select", "RUF911"]).build(); + assert_cmd_snapshot!(cmd, @r###" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- - warning: Selection `FURB145` has no effect because the `--preview` flag was not included. + warning: Selection `RUF911` has no effect because the `--preview` flag was not included. "###); } #[test] fn preview_disabled_prefix_empty() { - // Warns that the selection is empty since all of the CPY rules are in preview - let mut cmd = RuffCheck::default().args(["--select", "CPY"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" + // Warns that the selection is empty since all of the RUF91 rules are in preview + let mut cmd = RuffCheck::default().args(["--select", "RUF91"]).build(); + assert_cmd_snapshot!(cmd, @r###" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- - warning: Selection `CPY` has no effect because the `--preview` flag was not included. + warning: Selection `RUF91` has no effect because the `--preview` flag was not included. "###); } #[test] fn preview_disabled_does_not_warn_for_empty_ignore_selections() { // Does not warn that the selection is empty since the user is not trying to enable the rule - let mut cmd = RuffCheck::default().args(["--ignore", "CPY"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" - success: false - exit_code: 1 + let mut cmd = RuffCheck::default().args(["--ignore", "RUF9"]).build(); + assert_cmd_snapshot!(cmd, @r###" + success: true + exit_code: 0 ----- stdout ----- - -:1:1: E741 Ambiguous variable name: `I` - Found 1 error. ----- stderr ----- "###); @@ -998,14 +999,11 @@ fn preview_disabled_does_not_warn_for_empty_ignore_selections() { #[test] fn preview_disabled_does_not_warn_for_empty_fixable_selections() { // Does not warn that the selection is empty since the user is not trying to enable the rule - let mut cmd = RuffCheck::default().args(["--fixable", "CPY"]).build(); - assert_cmd_snapshot!(cmd - .pass_stdin("I=42\n"), @r###" - success: false - exit_code: 1 + let mut cmd = RuffCheck::default().args(["--fixable", "RUF9"]).build(); + assert_cmd_snapshot!(cmd, @r###" + success: true + exit_code: 0 ----- stdout ----- - -:1:1: E741 Ambiguous variable name: `I` - Found 1 error. ----- stderr ----- "###); @@ -1032,20 +1030,25 @@ fn preview_group_selector() { #[test] fn preview_enabled_group_ignore() { - // `--select E --ignore PREVIEW` should detect E741 and E225, which is in preview but "E" is more specific. + // Should detect stable and unstable rules, RUF9 is more specific than RUF so ignore has no effect let mut cmd = RuffCheck::default() - .args(["--select", "E", "--ignore", "PREVIEW", "--preview"]) + .args(["--select", "RUF9", "--ignore", "RUF", "--preview"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("I=42\n"), @r###" success: false - exit_code: 2 + exit_code: 1 ----- stdout ----- + -:1:1: RUF900 Hey this is a stable test rule. + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. + -:1:1: RUF911 Hey this is a preview test rule. + -:1:1: RUF912 Hey this is a nursery test rule. + Found 6 errors. + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- - error: invalid value 'PREVIEW' for '--ignore ' - - For more information, try '--help'. "###); } @@ -1150,16 +1153,15 @@ fn check_input_from_argfile() -> Result<()> { #[test] fn check_hints_hidden_unsafe_fixes() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034"]) + .args(["--select", "RUF901,RUF902"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - -:2:7: UP034 [*] Avoid extraneous parentheses + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). @@ -1169,14 +1171,14 @@ fn check_hints_hidden_unsafe_fixes() { #[test] fn check_hints_hidden_unsafe_fixes_with_no_safe_fixes() { - let mut cmd = RuffCheck::default().args(["--select", "F601"]).build(); + let mut cmd = RuffCheck::default().args(["--select", "RUF902"]).build(); assert_cmd_snapshot!(cmd .pass_stdin("x = {'a': 1, 'a': 1}\n"), @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. Found 1 error. No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option). @@ -1187,16 +1189,15 @@ fn check_hints_hidden_unsafe_fixes_with_no_safe_fixes() { #[test] fn check_no_hint_for_hidden_unsafe_fixes_when_disabled() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--no-unsafe-fixes"]) + .args(["--select", "RUF901,RUF902", "--no-unsafe-fixes"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - -:2:7: UP034 [*] Avoid extraneous parentheses + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. Found 2 errors. [*] 1 fixable with the --fix option. @@ -1207,7 +1208,7 @@ fn check_no_hint_for_hidden_unsafe_fixes_when_disabled() { #[test] fn check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled() { let mut cmd = RuffCheck::default() - .args(["--select", "F601", "--no-unsafe-fixes"]) + .args(["--select", "RUF901", "--no-unsafe-fixes"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("x = {'a': 1, 'a': 1}\n"), @@ -1215,8 +1216,9 @@ fn check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled() { success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. Found 1 error. + [*] 1 fixable with the --fix option. ----- stderr ----- "###); @@ -1225,16 +1227,15 @@ fn check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled() { #[test] fn check_shows_unsafe_fixes_with_opt_in() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--unsafe-fixes"]) + .args(["--select", "RUF901,RUF902", "--unsafe-fixes"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 [*] Dictionary key literal `'a'` repeated - -:2:7: UP034 [*] Avoid extraneous parentheses + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix. Found 2 errors. [*] 2 fixable with the --fix option. @@ -1245,47 +1246,48 @@ fn check_shows_unsafe_fixes_with_opt_in() { #[test] fn fix_applies_safe_fixes_by_default() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--fix"]) + .args(["--select", "RUF901,RUF902", "--fix"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - x = {'a': 1, 'a': 1} - print('foo') + # safe insertion ----- stderr ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - Found 2 errors (1 fixed, 1 remaining). - No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option). + -:1:1: RUF901 Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + Found 3 errors (1 fixed, 2 remaining). + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). "###); } #[test] fn fix_applies_unsafe_fixes_with_opt_in() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--fix", "--unsafe-fixes"]) + .args(["--select", "RUF901,RUF902", "--fix", "--unsafe-fixes"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" - success: true - exit_code: 0 + success: false + exit_code: 1 ----- stdout ----- - x = {'a': 1} - print('foo') + # unsafe insertion + # safe insertion ----- stderr ----- - Found 2 errors (2 fixed, 0 remaining). + -:1:1: RUF901 Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + Found 4 errors (2 fixed, 2 remaining). + [*] 2 fixable with the --fix option. "###); } #[test] fn fix_does_not_apply_display_only_fixes() { let mut cmd = RuffCheck::default() - .args(["--select", "B006", "--fix"]) + .args(["--select", "RUF903", "--fix"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("def add_to_list(item, some_list=[]): ..."), @@ -1295,7 +1297,7 @@ fn fix_does_not_apply_display_only_fixes() { ----- stdout ----- def add_to_list(item, some_list=[]): ... ----- stderr ----- - -:1:33: B006 Do not use mutable data structures for argument defaults + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. Found 1 error. "###); } @@ -1303,7 +1305,7 @@ fn fix_does_not_apply_display_only_fixes() { #[test] fn fix_does_not_apply_display_only_fixes_with_unsafe_fixes_enabled() { let mut cmd = RuffCheck::default() - .args(["--select", "B006", "--fix", "--unsafe-fixes"]) + .args(["--select", "RUF903", "--fix", "--unsafe-fixes"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("def add_to_list(item, some_list=[]): ..."), @@ -1313,7 +1315,7 @@ fn fix_does_not_apply_display_only_fixes_with_unsafe_fixes_enabled() { ----- stdout ----- def add_to_list(item, some_list=[]): ... ----- stderr ----- - -:1:33: B006 Do not use mutable data structures for argument defaults + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. Found 1 error. "###); } @@ -1321,37 +1323,33 @@ fn fix_does_not_apply_display_only_fixes_with_unsafe_fixes_enabled() { #[test] fn fix_only_unsafe_fixes_available() { let mut cmd = RuffCheck::default() - .args(["--select", "F601", "--fix"]) + .args(["--select", "RUF901", "--fix"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - x = {'a': 1, 'a': 1} - print(('foo')) + # safe insertion ----- stderr ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - Found 1 error. - No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option). + -:1:1: RUF901 Hey this is a stable test rule with a safe fix. + Found 2 errors (1 fixed, 1 remaining). + [*] 1 fixable with the `--fix` option. "###); } #[test] fn fix_only_flag_applies_safe_fixes_by_default() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--fix-only"]) + .args(["--select", "RUF901,RUF902", "--fix-only"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: true exit_code: 0 ----- stdout ----- - x = {'a': 1, 'a': 1} - print('foo') + # safe insertion ----- stderr ----- Fixed 1 error (1 additional fix available with `--unsafe-fixes`). @@ -1361,16 +1359,15 @@ fn fix_only_flag_applies_safe_fixes_by_default() { #[test] fn fix_only_flag_applies_unsafe_fixes_with_opt_in() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--fix-only", "--unsafe-fixes"]) + .args(["--select", "RUF901,RUF902", "--fix-only", "--unsafe-fixes"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: true exit_code: 0 ----- stdout ----- - x = {'a': 1} - print('foo') + # unsafe insertion + # safe insertion ----- stderr ----- Fixed 2 errors. @@ -1380,18 +1377,15 @@ fn fix_only_flag_applies_unsafe_fixes_with_opt_in() { #[test] fn diff_shows_safe_fixes_by_default() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--diff"]) + .args(["--select", "RUF901,RUF902", "--diff"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - @@ -1,2 +1,2 @@ - x = {'a': 1, 'a': 1} - -print(('foo')) - +print('foo') + @@ -0,0 +1 @@ + +# safe insertion ----- stderr ----- @@ -1403,19 +1397,16 @@ fn diff_shows_safe_fixes_by_default() { #[test] fn diff_shows_unsafe_fixes_with_opt_in() { let mut cmd = RuffCheck::default() - .args(["--select", "F601,UP034", "--diff", "--unsafe-fixes"]) + .args(["--select", "RUF901,RUF902", "--diff", "--unsafe-fixes"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - @@ -1,2 +1,2 @@ - -x = {'a': 1, 'a': 1} - -print(('foo')) - +x = {'a': 1} - +print('foo') + @@ -0,0 +1,2 @@ + +# unsafe insertion + +# safe insertion ----- stderr ----- @@ -1427,7 +1418,7 @@ fn diff_shows_unsafe_fixes_with_opt_in() { #[test] fn diff_does_not_show_display_only_fixes_with_unsafe_fixes_enabled() { let mut cmd = RuffCheck::default() - .args(["--select", "B006", "--diff", "--unsafe-fixes"]) + .args(["--select", "RUF903", "--diff", "--unsafe-fixes"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("def add_to_list(item, some_list=[]): ..."), @@ -1443,10 +1434,9 @@ fn diff_does_not_show_display_only_fixes_with_unsafe_fixes_enabled() { #[test] fn diff_only_unsafe_fixes_available() { let mut cmd = RuffCheck::default() - .args(["--select", "F601", "--diff"]) + .args(["--select", "RUF902", "--diff"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: true exit_code: 0 @@ -1466,22 +1456,21 @@ fn check_extend_unsafe_fixes() -> Result<()> { &ruff_toml, r#" [lint] -extend-unsafe-fixes = ["UP034"] +extend-unsafe-fixes = ["RUF901"] "#, )?; let mut cmd = RuffCheck::default() .config(&ruff_toml) - .args(["--select", "F601,UP034"]) + .args(["--select", "RUF901,RUF902"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - -:2:7: UP034 Avoid extraneous parentheses + -:1:1: RUF901 Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. Found 2 errors. No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option). @@ -1499,22 +1488,21 @@ fn check_extend_safe_fixes() -> Result<()> { &ruff_toml, r#" [lint] -extend-safe-fixes = ["F601"] +extend-safe-fixes = ["RUF902"] "#, )?; let mut cmd = RuffCheck::default() .config(&ruff_toml) - .args(["--select", "F601,UP034"]) + .args(["--select", "RUF901,RUF902"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 [*] Dictionary key literal `'a'` repeated - -:2:7: UP034 [*] Avoid extraneous parentheses + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix. Found 2 errors. [*] 2 fixable with the `--fix` option. @@ -1533,25 +1521,24 @@ fn check_extend_unsafe_fixes_conflict_with_extend_safe_fixes() -> Result<()> { &ruff_toml, r#" [lint] -extend-unsafe-fixes = ["UP034"] -extend-safe-fixes = ["UP034"] +extend-unsafe-fixes = ["RUF902"] +extend-safe-fixes = ["RUF902"] "#, )?; let mut cmd = RuffCheck::default() .config(&ruff_toml) - .args(["--select", "F601,UP034"]) + .args(["--select", "RUF901,RUF902"]) .build(); - assert_cmd_snapshot!(cmd - .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + assert_cmd_snapshot!(cmd, @r###" success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - -:2:7: UP034 Avoid extraneous parentheses + -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. Found 2 errors. - No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option). + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- "###); @@ -1569,14 +1556,14 @@ fn check_extend_unsafe_fixes_conflict_with_extend_safe_fixes_by_specificity() -> r#" target-version = "py310" [lint] -extend-unsafe-fixes = ["UP", "UP034"] -extend-safe-fixes = ["UP03"] +extend-unsafe-fixes = ["RUF", "RUF901"] +extend-safe-fixes = ["RUF9"] "#, )?; let mut cmd = RuffCheck::default() .config(&ruff_toml) - .args(["--select", "F601,UP018,UP034,UP038"]) + .args(["--select", "RUF9"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\nprint(str('foo'))\nisinstance(x, (int, str))\n"), @@ -1584,12 +1571,12 @@ extend-safe-fixes = ["UP03"] success: false exit_code: 1 ----- stdout ----- - -:1:14: F601 Dictionary key literal `'a'` repeated - -:2:7: UP034 Avoid extraneous parentheses - -:3:7: UP018 Unnecessary `str` call (rewrite as a literal) - -:4:1: UP038 [*] Use `X | Y` in `isinstance` call instead of `(X, Y)` + -:1:1: RUF900 Hey this is a stable test rule. + -:1:1: RUF901 Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix. + -:1:1: RUF903 Hey this is a stable test rule with a display only fix. Found 4 errors. - [*] 1 fixable with the `--fix` option (3 hidden fixes can be enabled with the `--unsafe-fixes` option). + [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ----- stderr ----- "###); diff --git a/crates/ruff_linter/Cargo.toml b/crates/ruff_linter/Cargo.toml index 00f0b3d82a5480..4fd4dc77f0f7de 100644 --- a/crates/ruff_linter/Cargo.toml +++ b/crates/ruff_linter/Cargo.toml @@ -85,6 +85,8 @@ tempfile = { workspace = true } [features] default = [] schemars = ["dep:schemars"] +# Enables rules for internal integration tests +test-rules = [] [lints] workspace = true diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 6f802c79bd1e44..5ec7449d3d8d14 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -934,6 +934,20 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml), + #[cfg(feature = "test-rules")] + (Ruff, "900") => (RuleGroup::Stable, rules::ruff::rules::StableTestRule), + #[cfg(feature = "test-rules")] + (Ruff, "901") => (RuleGroup::Stable, rules::ruff::rules::StableTestRuleSafeFix), + #[cfg(feature = "test-rules")] + (Ruff, "902") => (RuleGroup::Stable, rules::ruff::rules::StableTestRuleUnsafeFix), + #[cfg(feature = "test-rules")] + (Ruff, "903") => (RuleGroup::Stable, rules::ruff::rules::StableTestRuleDisplayOnlyFix), + #[cfg(feature = "test-rules")] + (Ruff, "911") => (RuleGroup::Preview, rules::ruff::rules::PreviewTestRule), + #[cfg(feature = "test-rules")] + #[allow(deprecated)] + (Ruff, "912") => (RuleGroup::Nursery, rules::ruff::rules::NurseryTestRule), + // flake8-django (Flake8Django, "001") => (RuleGroup::Stable, rules::flake8_django::rules::DjangoNullableModelStringField), diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index af7baf4f7d8f96..d48d245f0701e3 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -6,8 +6,6 @@ use anyhow::{anyhow, Result}; use colored::Colorize; use itertools::Itertools; use log::error; -use rustc_hash::FxHashMap; - use ruff_diagnostics::Diagnostic; use ruff_notebook::Notebook; use ruff_python_ast::imports::ImportMap; @@ -18,6 +16,9 @@ use ruff_python_parser::lexer::LexResult; use ruff_python_parser::{AsMode, ParseError}; use ruff_source_file::{Locator, SourceFileBuilder}; use ruff_text_size::Ranged; +use rustc_hash::FxHashMap; +#[cfg(feature = "test-rules")] +use rustc_hash::FxHashSet; use crate::checkers::ast::check_ast; use crate::checkers::filesystem::check_file_path; @@ -33,6 +34,8 @@ use crate::message::Message; use crate::noqa::add_noqa; use crate::registry::{AsRule, Rule, RuleSet}; use crate::rules::pycodestyle; +#[cfg(feature = "test-rules")] +use crate::rules::ruff::rules::TEST_RULES; use crate::settings::types::UnsafeFixes; use crate::settings::{flags, LinterSettings}; use crate::source_kind::SourceKind; @@ -214,6 +217,63 @@ pub fn check_path( )); } + // Raise violations for internal test rules + #[cfg(feature = "test-rules")] + { + if settings.rules.enabled(Rule::StableTestRule) { + diagnostics.push(Diagnostic::new( + crate::rules::ruff::rules::StableTestRule, + ruff_text_size::TextRange::default(), + )); + } + + // Applying the fix must "resolve" the diagnostic or we will not converge + // So only raise the diagnostics on the "first" iteration + if settings.rules.enabled(Rule::StableTestRuleSafeFix) { + diagnostics.push( + Diagnostic::new( + crate::rules::ruff::rules::StableTestRuleSafeFix, + ruff_text_size::TextRange::default(), + ) + .with_fix(crate::rules::ruff::rules::StableTestRuleSafeFix::fix()), + ); + } + + if settings.rules.enabled(Rule::StableTestRuleUnsafeFix) { + diagnostics.push( + Diagnostic::new( + crate::rules::ruff::rules::StableTestRuleUnsafeFix, + ruff_text_size::TextRange::default(), + ) + .with_fix(crate::rules::ruff::rules::StableTestRuleUnsafeFix::fix()), + ); + } + + if settings.rules.enabled(Rule::StableTestRuleDisplayOnlyFix) { + diagnostics.push( + Diagnostic::new( + crate::rules::ruff::rules::StableTestRuleDisplayOnlyFix, + ruff_text_size::TextRange::default(), + ) + .with_fix(crate::rules::ruff::rules::StableTestRuleDisplayOnlyFix::fix()), + ); + } + + if settings.rules.enabled(Rule::PreviewTestRule) { + diagnostics.push(Diagnostic::new( + crate::rules::ruff::rules::PreviewTestRule, + ruff_text_size::TextRange::default(), + )); + } + + if settings.rules.enabled(Rule::NurseryTestRule) { + diagnostics.push(Diagnostic::new( + crate::rules::ruff::rules::NurseryTestRule, + ruff_text_size::TextRange::default(), + )); + } + } + // Ignore diagnostics based on per-file-ignores. let per_file_ignores = if !diagnostics.is_empty() && !settings.per_file_ignores.is_empty() { fs::ignores_from_path(path, &settings.per_file_ignores) @@ -460,6 +520,11 @@ pub fn lint_fix<'a>( // Track the number of fixed errors across iterations. let mut fixed = FxHashMap::default(); + // Track the applied fixes when using test rules + // So we can filter them out after "fixing" them + #[cfg(feature = "test-rules")] + let mut applied_test_fixes = FxHashSet::default(); + // As an escape hatch, bail after 100 iterations. let mut iterations = 0; @@ -521,16 +586,39 @@ pub fn lint_fix<'a>( } } + #[cfg(not(feature = "test-rules"))] + let diagnostics = &result.data.0; + + // HACK(zanieb): We cannot raise violations for "fixed" test diagnostics + // or we will never converge since they raise unconditionally + // so we filter them out here + #[cfg(feature = "test-rules")] + let filtered_diagnostics = result + .data + .0 + .iter() + .filter(|diagnostic| !applied_test_fixes.contains(&diagnostic.kind.rule())) + .cloned() + .collect::>(); + #[cfg(feature = "test-rules")] + let diagnostics = filtered_diagnostics.as_slice(); + // Apply fix. if let Some(FixResult { code: fixed_contents, fixes: applied, source_map, - }) = fix_file(&result.data.0, &locator, unsafe_fixes) + }) = fix_file(diagnostics, &locator, unsafe_fixes) { if iterations < MAX_ITERATIONS { // Count the number of fixed errors. for (rule, count) in applied { + // Track which test rule fixes are applied + #[cfg(feature = "test-rules")] + if TEST_RULES.contains(&rule) { + applied_test_fixes.insert(rule.clone()); + } + *fixed.entry(rule).or_default() += count; } @@ -539,11 +627,11 @@ pub fn lint_fix<'a>( // Increment the iteration count. iterations += 1; - // Re-run the linter pass (by avoiding the break). + // Re-run the linter pass (by avoiding the return). continue; } - report_failed_to_converge_error(path, transformed.source_code(), &result.data.0); + report_failed_to_converge_error(path, transformed.source_code(), diagnostics); } return Ok(FixerResult { diff --git a/crates/ruff_linter/src/rule_selector.rs b/crates/ruff_linter/src/rule_selector.rs index 2f6e7a5da12ac4..4197c4d4478593 100644 --- a/crates/ruff_linter/src/rule_selector.rs +++ b/crates/ruff_linter/src/rule_selector.rs @@ -289,6 +289,8 @@ mod schema { (!prefix.is_empty()).then(|| prefix.to_string()) })), ) + // Strip the test rules from the schema + .filter(|code| !code.starts_with("RUF9")) .sorted() .map(Value::String) .collect(), diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 7a2f8dc2f7fdc9..f4c23b28112d89 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -18,6 +18,8 @@ pub(crate) use quadratic_list_summation::*; pub(crate) use sort_dunder_all::*; pub(crate) use sort_dunder_slots::*; pub(crate) use static_key_dict_comprehension::*; +#[cfg(feature = "test-rules")] +pub(crate) use test_rules::*; pub(crate) use unnecessary_dict_comprehension_for_iterable::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; @@ -45,6 +47,8 @@ mod sequence_sorting; mod sort_dunder_all; mod sort_dunder_slots; mod static_key_dict_comprehension; +#[cfg(feature = "test-rules")] +mod test_rules; mod unnecessary_dict_comprehension_for_iterable; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; diff --git a/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs b/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs new file mode 100644 index 00000000000000..36835d59121824 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs @@ -0,0 +1,203 @@ +use ruff_diagnostics::{Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_text_size::TextSize; + +use crate::registry::Rule; + +pub(crate) const TEST_RULES: &[Rule] = &[ + Rule::StableTestRule, + Rule::StableTestRuleSafeFix, + Rule::StableTestRuleUnsafeFix, + Rule::StableTestRuleDisplayOnlyFix, + Rule::NurseryTestRule, + Rule::PreviewTestRule, +]; + +/// ## What it does +/// Fake rule for testing. +/// +/// ## Why is this bad? +/// Tests must pass! +/// +/// ## Example +/// ```python +/// foo +/// ``` +/// +/// Use instead: +/// ```python +/// bar +/// ``` +#[violation] +pub struct StableTestRule; + +impl Violation for StableTestRule { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::None; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Hey this is a stable test rule.") + } +} + +/// ## What it does +/// Fake rule for testing. +/// +/// ## Why is this bad? +/// Tests must pass! +/// +/// ## Example +/// ```python +/// foo +/// ``` +/// +/// Use instead: +/// ```python +/// bar +/// ``` +#[violation] +pub struct StableTestRuleSafeFix; + +impl Violation for StableTestRuleSafeFix { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Hey this is a stable test rule with a safe fix.") + } +} + +impl StableTestRuleSafeFix { + pub(crate) fn fix() -> Fix { + Fix::safe_edit(Edit::insertion( + "# safe insertion\n".to_string(), + TextSize::new(0), + )) + } +} + +/// ## What it does +/// Fake rule for testing. +/// +/// ## Why is this bad? +/// Tests must pass! +/// +/// ## Example +/// ```python +/// foo +/// ``` +/// +/// Use instead: +/// ```python +/// bar +/// ``` +#[violation] +pub struct StableTestRuleUnsafeFix; + +impl Violation for StableTestRuleUnsafeFix { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Hey this is a stable test rule with an unsafe fix.") + } +} + +impl StableTestRuleUnsafeFix { + pub(crate) fn fix() -> Fix { + Fix::unsafe_edit(Edit::insertion( + "# unsafe insertion\n".to_string(), + TextSize::new(0), + )) + } +} + +/// ## What it does +/// Fake rule for testing. +/// +/// ## Why is this bad? +/// Tests must pass! +/// +/// ## Example +/// ```python +/// foo +/// ``` +/// +/// Use instead: +/// ```python +/// bar +/// ``` +#[violation] +pub struct StableTestRuleDisplayOnlyFix; + +impl Violation for StableTestRuleDisplayOnlyFix { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Hey this is a stable test rule with a display only fix.") + } +} + +impl StableTestRuleDisplayOnlyFix { + pub(crate) fn fix() -> Fix { + Fix::display_only_edit(Edit::insertion( + "# display only insertion\n".to_string(), + TextSize::new(0), + )) + } +} + +/// ## What it does +/// Fake rule for testing. +/// +/// ## Why is this bad? +/// Tests must pass! +/// +/// ## Example +/// ```python +/// foo +/// ``` +/// +/// Use instead: +/// ```python +/// bar +/// ``` +#[violation] +pub struct PreviewTestRule; + +impl Violation for PreviewTestRule { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::None; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Hey this is a preview test rule.") + } +} + +/// ## What it does +/// Fake rule for testing. +/// +/// ## Why is this bad? +/// Tests must pass! +/// +/// ## Example +/// ```python +/// foo +/// ``` +/// +/// Use instead: +/// ```python +/// bar +/// ``` +#[violation] +pub struct NurseryTestRule; + +impl Violation for NurseryTestRule { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::None; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Hey this is a nursery test rule.") + } +} diff --git a/crates/ruff_workspace/Cargo.toml b/crates/ruff_workspace/Cargo.toml index b7781678bf0b48..809fc227b6df35 100644 --- a/crates/ruff_workspace/Cargo.toml +++ b/crates/ruff_workspace/Cargo.toml @@ -42,6 +42,8 @@ strum = { workspace = true } toml = { workspace = true } [dev-dependencies] +# Enable test rules during development +ruff_linter = { path = "../ruff_linter", features = ["clap", "test-rules"] } tempfile = { workspace = true } [features] diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index f759febfccb7ae..fbaeec25228ce2 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1184,6 +1184,7 @@ mod tests { Rule::DeleteFullSlice, Rule::CheckAndRemoveFromSet, Rule::QuadraticListSummation, + Rule::NurseryTestRule, ]; const PREVIEW_RULES: &[Rule] = &[ @@ -1201,6 +1202,7 @@ mod tests { Rule::UndocumentedWarn, Rule::UnnecessaryEnumerate, Rule::MathConstant, + Rule::PreviewTestRule, ]; #[allow(clippy::needless_pass_by_value)] From 43ed884b350421d97b820afe1dd3b217744cf8bd Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 15:06:05 -0600 Subject: [PATCH 02/17] Ignore internal rules in ecosystem checks --- python/ruff-ecosystem/ruff_ecosystem/projects.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/python/ruff-ecosystem/ruff_ecosystem/projects.py b/python/ruff-ecosystem/ruff_ecosystem/projects.py index fc0139c7d1e140..487ed444125f1c 100644 --- a/python/ruff-ecosystem/ruff_ecosystem/projects.py +++ b/python/ruff-ecosystem/ruff_ecosystem/projects.py @@ -206,6 +206,8 @@ def to_ruff_args(self) -> list[str]: "check", "--no-cache", "--exit-zero", + # Ignore internal test rules + "--ignore RUF9", f"--{'' if self.preview else 'no-'}preview", ] if self.select: From 0fea08cdebb36c6e10d0521a9f68720c45583882 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 15:07:29 -0600 Subject: [PATCH 03/17] Remove uncessary clone --- crates/ruff_linter/src/linter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index d48d245f0701e3..06ab2c43efdc4c 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -616,7 +616,7 @@ pub fn lint_fix<'a>( // Track which test rule fixes are applied #[cfg(feature = "test-rules")] if TEST_RULES.contains(&rule) { - applied_test_fixes.insert(rule.clone()); + applied_test_fixes.insert(rule); } *fixed.entry(rule).or_default() += count; From f5e44b6436ba3d25fb4ff4a3735ca39822c6298e Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 15:10:02 -0600 Subject: [PATCH 04/17] Fix RUF900 comments --- crates/ruff/tests/integration_test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index afe71d41ba2434..3ddcae5eb395cd 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -799,7 +799,7 @@ fn show_statistics() { #[test] fn nursery_prefix() { - // Should only detect RUF900, but not the unstable test rules + // Should only detect RUF90X, but not the unstable test rules let mut cmd = RuffCheck::default().args(["--select", "RUF9"]).build(); assert_cmd_snapshot!(cmd, @r###" success: false @@ -818,7 +818,7 @@ fn nursery_prefix() { #[test] fn nursery_all() { - // Should detect RUF900, but not the unstable test rules + // Should detect RUF90X, but not the unstable test rules let mut cmd = RuffCheck::default().args(["--select", "ALL"]).build(); assert_cmd_snapshot!(cmd, @r###" success: false From ca92070ce93e2d32da7045ce39cf17050c071a1e Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 15:12:00 -0600 Subject: [PATCH 05/17] Fix code for `check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled` --- crates/ruff/tests/integration_test.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 3ddcae5eb395cd..9d2fcdfb43a49c 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -1208,7 +1208,7 @@ fn check_no_hint_for_hidden_unsafe_fixes_when_disabled() { #[test] fn check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled() { let mut cmd = RuffCheck::default() - .args(["--select", "RUF901", "--no-unsafe-fixes"]) + .args(["--select", "RUF902", "--no-unsafe-fixes"]) .build(); assert_cmd_snapshot!(cmd .pass_stdin("x = {'a': 1, 'a': 1}\n"), @@ -1216,9 +1216,8 @@ fn check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled() { success: false exit_code: 1 ----- stdout ----- - -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix. + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. Found 1 error. - [*] 1 fixable with the --fix option. ----- stderr ----- "###); From 7d80d95fd586358649c2e1f22cbbb23663b40fc1 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:22:08 -0600 Subject: [PATCH 06/17] Skip diagnostics if fix comment inserted --- crates/ruff/tests/integration_test.rs | 42 +++--- crates/ruff_linter/src/linter.rs | 114 ++++------------ .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 +- .../src/rules/ruff/rules/test_rules.rs | 128 +++++++++++++++--- 4 files changed, 157 insertions(+), 129 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 9d2fcdfb43a49c..354e95e6c24a00 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -1252,13 +1252,12 @@ fn fix_applies_safe_fixes_by_default() { success: false exit_code: 1 ----- stdout ----- - # safe insertion + # fix from stable-test-rule-safe-fix ----- stderr ----- - -:1:1: RUF901 Hey this is a stable test rule with a safe fix. -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. - Found 3 errors (1 fixed, 2 remaining). - [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). + Found 2 errors (1 fixed, 1 remaining). + No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option). "###); } @@ -1269,17 +1268,14 @@ fn fix_applies_unsafe_fixes_with_opt_in() { .build(); assert_cmd_snapshot!(cmd, @r###" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- - # unsafe insertion - # safe insertion + # fix from stable-test-rule-unsafe-fix + # fix from stable-test-rule-safe-fix ----- stderr ----- - -:1:1: RUF901 Hey this is a stable test rule with a safe fix. - -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. - Found 4 errors (2 fixed, 2 remaining). - [*] 2 fixable with the --fix option. + Found 2 errors (2 fixed, 0 remaining). "###); } @@ -1326,15 +1322,13 @@ fn fix_only_unsafe_fixes_available() { .build(); assert_cmd_snapshot!(cmd, @r###" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- - # safe insertion + # fix from stable-test-rule-safe-fix ----- stderr ----- - -:1:1: RUF901 Hey this is a stable test rule with a safe fix. - Found 2 errors (1 fixed, 1 remaining). - [*] 1 fixable with the `--fix` option. + Found 1 error (1 fixed, 0 remaining). "###); } @@ -1348,7 +1342,7 @@ fn fix_only_flag_applies_safe_fixes_by_default() { success: true exit_code: 0 ----- stdout ----- - # safe insertion + # fix from stable-test-rule-safe-fix ----- stderr ----- Fixed 1 error (1 additional fix available with `--unsafe-fixes`). @@ -1365,8 +1359,8 @@ fn fix_only_flag_applies_unsafe_fixes_with_opt_in() { success: true exit_code: 0 ----- stdout ----- - # unsafe insertion - # safe insertion + # fix from stable-test-rule-unsafe-fix + # fix from stable-test-rule-safe-fix ----- stderr ----- Fixed 2 errors. @@ -1384,7 +1378,7 @@ fn diff_shows_safe_fixes_by_default() { exit_code: 1 ----- stdout ----- @@ -0,0 +1 @@ - +# safe insertion + +# fix from stable-test-rule-safe-fix ----- stderr ----- @@ -1404,8 +1398,8 @@ fn diff_shows_unsafe_fixes_with_opt_in() { exit_code: 1 ----- stdout ----- @@ -0,0 +1,2 @@ - +# unsafe insertion - +# safe insertion + +# fix from stable-test-rule-unsafe-fix + +# fix from stable-test-rule-safe-fix ----- stderr ----- diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 06ab2c43efdc4c..92ad8bbf0a3850 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -17,8 +17,6 @@ use ruff_python_parser::{AsMode, ParseError}; use ruff_source_file::{Locator, SourceFileBuilder}; use ruff_text_size::Ranged; use rustc_hash::FxHashMap; -#[cfg(feature = "test-rules")] -use rustc_hash::FxHashSet; use crate::checkers::ast::check_ast; use crate::checkers::filesystem::check_file_path; @@ -35,7 +33,7 @@ use crate::noqa::add_noqa; use crate::registry::{AsRule, Rule, RuleSet}; use crate::rules::pycodestyle; #[cfg(feature = "test-rules")] -use crate::rules::ruff::rules::TEST_RULES; +use crate::rules::ruff::rules::test_rules::{self, TestRule, TEST_RULES}; use crate::settings::types::UnsafeFixes; use crate::settings::{flags, LinterSettings}; use crate::source_kind::SourceKind; @@ -220,57 +218,33 @@ pub fn check_path( // Raise violations for internal test rules #[cfg(feature = "test-rules")] { - if settings.rules.enabled(Rule::StableTestRule) { - diagnostics.push(Diagnostic::new( - crate::rules::ruff::rules::StableTestRule, - ruff_text_size::TextRange::default(), - )); - } - - // Applying the fix must "resolve" the diagnostic or we will not converge - // So only raise the diagnostics on the "first" iteration - if settings.rules.enabled(Rule::StableTestRuleSafeFix) { - diagnostics.push( - Diagnostic::new( - crate::rules::ruff::rules::StableTestRuleSafeFix, - ruff_text_size::TextRange::default(), - ) - .with_fix(crate::rules::ruff::rules::StableTestRuleSafeFix::fix()), - ); - } - - if settings.rules.enabled(Rule::StableTestRuleUnsafeFix) { - diagnostics.push( - Diagnostic::new( - crate::rules::ruff::rules::StableTestRuleUnsafeFix, - ruff_text_size::TextRange::default(), - ) - .with_fix(crate::rules::ruff::rules::StableTestRuleUnsafeFix::fix()), - ); - } - - if settings.rules.enabled(Rule::StableTestRuleDisplayOnlyFix) { - diagnostics.push( - Diagnostic::new( - crate::rules::ruff::rules::StableTestRuleDisplayOnlyFix, - ruff_text_size::TextRange::default(), - ) - .with_fix(crate::rules::ruff::rules::StableTestRuleDisplayOnlyFix::fix()), - ); - } - - if settings.rules.enabled(Rule::PreviewTestRule) { - diagnostics.push(Diagnostic::new( - crate::rules::ruff::rules::PreviewTestRule, - ruff_text_size::TextRange::default(), - )); - } - - if settings.rules.enabled(Rule::NurseryTestRule) { - diagnostics.push(Diagnostic::new( - crate::rules::ruff::rules::NurseryTestRule, - ruff_text_size::TextRange::default(), - )); + for test_rule in TEST_RULES { + if settings.rules.enabled(*test_rule) { + let diagnostic = match test_rule { + Rule::StableTestRule => { + test_rules::StableTestRule::diagnostic(&locator, &indexer) + } + Rule::StableTestRuleSafeFix => { + test_rules::StableTestRuleSafeFix::diagnostic(&locator, &indexer) + } + Rule::StableTestRuleUnsafeFix => { + test_rules::StableTestRuleUnsafeFix::diagnostic(&locator, &indexer) + } + Rule::StableTestRuleDisplayOnlyFix => { + test_rules::StableTestRuleDisplayOnlyFix::diagnostic(&locator, &indexer) + } + Rule::NurseryTestRule => { + test_rules::NurseryTestRule::diagnostic(&locator, &indexer) + } + Rule::PreviewTestRule => { + test_rules::PreviewTestRule::diagnostic(&locator, &indexer) + } + _ => unreachable!("All test rules must have an implementation"), + }; + if let Some(diagnostic) = diagnostic { + diagnostics.push(diagnostic); + } + } } } @@ -520,11 +494,6 @@ pub fn lint_fix<'a>( // Track the number of fixed errors across iterations. let mut fixed = FxHashMap::default(); - // Track the applied fixes when using test rules - // So we can filter them out after "fixing" them - #[cfg(feature = "test-rules")] - let mut applied_test_fixes = FxHashSet::default(); - // As an escape hatch, bail after 100 iterations. let mut iterations = 0; @@ -586,39 +555,16 @@ pub fn lint_fix<'a>( } } - #[cfg(not(feature = "test-rules"))] - let diagnostics = &result.data.0; - - // HACK(zanieb): We cannot raise violations for "fixed" test diagnostics - // or we will never converge since they raise unconditionally - // so we filter them out here - #[cfg(feature = "test-rules")] - let filtered_diagnostics = result - .data - .0 - .iter() - .filter(|diagnostic| !applied_test_fixes.contains(&diagnostic.kind.rule())) - .cloned() - .collect::>(); - #[cfg(feature = "test-rules")] - let diagnostics = filtered_diagnostics.as_slice(); - // Apply fix. if let Some(FixResult { code: fixed_contents, fixes: applied, source_map, - }) = fix_file(diagnostics, &locator, unsafe_fixes) + }) = fix_file(&result.data.0, &locator, unsafe_fixes) { if iterations < MAX_ITERATIONS { // Count the number of fixed errors. for (rule, count) in applied { - // Track which test rule fixes are applied - #[cfg(feature = "test-rules")] - if TEST_RULES.contains(&rule) { - applied_test_fixes.insert(rule); - } - *fixed.entry(rule).or_default() += count; } @@ -631,7 +577,7 @@ pub fn lint_fix<'a>( continue; } - report_failed_to_converge_error(path, transformed.source_code(), diagnostics); + report_failed_to_converge_error(path, transformed.source_code(), &result.data.0); } return Ok(FixerResult { diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index f4c23b28112d89..4d245b5da844a8 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -48,7 +48,7 @@ mod sort_dunder_all; mod sort_dunder_slots; mod static_key_dict_comprehension; #[cfg(feature = "test-rules")] -mod test_rules; +pub(crate) mod test_rules; mod unnecessary_dict_comprehension_for_iterable; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; diff --git a/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs b/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs index 36835d59121824..5a78526f663dc6 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs @@ -1,18 +1,50 @@ -use ruff_diagnostics::{Edit, Fix, FixAvailability, Violation}; +/// Fake rules for testing Ruff's behavior +/// +/// All of these rules should be assigned to the RUF9XX codes. +/// +/// Implementing a new test rule involves: +/// +/// - Writing an empty struct for the rule +/// - Adding to the rule registry +/// - Adding to the `TEST_RULES` constant +/// - Implementing `Violation` for the rule +/// - Implementing `TestRule` for the rule +/// - Adding a match arm in `linter::check_path` +/// +/// Rules that provide a fix _must_ not raise unconditionally or the linter +/// will not converge. +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_index::Indexer; +use ruff_source_file::Locator; use ruff_text_size::TextSize; use crate::registry::Rule; +/// Check if a comment exists anywhere in a the given file +fn comment_exists(text: &str, locator: &Locator, indexer: &Indexer) -> bool { + for range in indexer.comment_ranges() { + let comment_text = locator.slice(range); + if text.trim_end() == comment_text { + return true; + } + } + false +} + pub(crate) const TEST_RULES: &[Rule] = &[ Rule::StableTestRule, Rule::StableTestRuleSafeFix, Rule::StableTestRuleUnsafeFix, Rule::StableTestRuleDisplayOnlyFix, - Rule::NurseryTestRule, Rule::PreviewTestRule, + Rule::NurseryTestRule, ]; +pub(crate) trait TestRule { + fn diagnostic(locator: &Locator, indexer: &Indexer) -> Option; +} + /// ## What it does /// Fake rule for testing. /// @@ -40,6 +72,15 @@ impl Violation for StableTestRule { } } +impl TestRule for StableTestRule { + fn diagnostic(_locator: &Locator, _indexer: &Indexer) -> Option { + Some(Diagnostic::new( + StableTestRule, + ruff_text_size::TextRange::default(), + )) + } +} + /// ## What it does /// Fake rule for testing. /// @@ -67,12 +108,20 @@ impl Violation for StableTestRuleSafeFix { } } -impl StableTestRuleSafeFix { - pub(crate) fn fix() -> Fix { - Fix::safe_edit(Edit::insertion( - "# safe insertion\n".to_string(), - TextSize::new(0), - )) +impl TestRule for StableTestRuleSafeFix { + fn diagnostic(locator: &Locator, indexer: &Indexer) -> Option { + let comment = format!("# fix from stable-test-rule-safe-fix\n"); + if comment_exists(&comment, locator, indexer) { + None + } else { + Some( + Diagnostic::new(StableTestRuleSafeFix, ruff_text_size::TextRange::default()) + .with_fix(Fix::safe_edit(Edit::insertion( + comment.to_string(), + TextSize::new(0), + ))), + ) + } } } @@ -103,12 +152,23 @@ impl Violation for StableTestRuleUnsafeFix { } } -impl StableTestRuleUnsafeFix { - pub(crate) fn fix() -> Fix { - Fix::unsafe_edit(Edit::insertion( - "# unsafe insertion\n".to_string(), - TextSize::new(0), - )) +impl TestRule for StableTestRuleUnsafeFix { + fn diagnostic(locator: &Locator, indexer: &Indexer) -> Option { + let comment = format!("# fix from stable-test-rule-unsafe-fix\n"); + if comment_exists(&comment, locator, indexer) { + None + } else { + Some( + Diagnostic::new( + StableTestRuleUnsafeFix, + ruff_text_size::TextRange::default(), + ) + .with_fix(Fix::unsafe_edit(Edit::insertion( + comment.to_string(), + TextSize::new(0), + ))), + ) + } } } @@ -139,12 +199,23 @@ impl Violation for StableTestRuleDisplayOnlyFix { } } -impl StableTestRuleDisplayOnlyFix { - pub(crate) fn fix() -> Fix { - Fix::display_only_edit(Edit::insertion( - "# display only insertion\n".to_string(), - TextSize::new(0), - )) +impl TestRule for StableTestRuleDisplayOnlyFix { + fn diagnostic(locator: &Locator, indexer: &Indexer) -> Option { + let comment = format!("# fix from stable-test-rule-display-only-fix\n"); + if comment_exists(&comment, locator, indexer) { + None + } else { + Some( + Diagnostic::new( + StableTestRuleDisplayOnlyFix, + ruff_text_size::TextRange::default(), + ) + .with_fix(Fix::display_only_edit(Edit::insertion( + comment.to_string(), + TextSize::new(0), + ))), + ) + } } } @@ -175,6 +246,14 @@ impl Violation for PreviewTestRule { } } +impl TestRule for PreviewTestRule { + fn diagnostic(_locator: &Locator, _indexer: &Indexer) -> Option { + Some(Diagnostic::new( + PreviewTestRule, + ruff_text_size::TextRange::default(), + )) + } +} /// ## What it does /// Fake rule for testing. /// @@ -201,3 +280,12 @@ impl Violation for NurseryTestRule { format!("Hey this is a nursery test rule.") } } + +impl TestRule for NurseryTestRule { + fn diagnostic(_locator: &Locator, _indexer: &Indexer) -> Option { + Some(Diagnostic::new( + NurseryTestRule, + ruff_text_size::TextRange::default(), + )) + } +} From d05452f6e342743ce571be177d6581ba6db5c72c Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:23:42 -0600 Subject: [PATCH 07/17] Fix move of import --- crates/ruff_linter/src/linter.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 92ad8bbf0a3850..4f30cd1c906faa 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -6,16 +6,6 @@ use anyhow::{anyhow, Result}; use colored::Colorize; use itertools::Itertools; use log::error; -use ruff_diagnostics::Diagnostic; -use ruff_notebook::Notebook; -use ruff_python_ast::imports::ImportMap; -use ruff_python_ast::{PySourceType, Suite}; -use ruff_python_codegen::Stylist; -use ruff_python_index::Indexer; -use ruff_python_parser::lexer::LexResult; -use ruff_python_parser::{AsMode, ParseError}; -use ruff_source_file::{Locator, SourceFileBuilder}; -use ruff_text_size::Ranged; use rustc_hash::FxHashMap; use crate::checkers::ast::check_ast; @@ -38,6 +28,16 @@ use crate::settings::types::UnsafeFixes; use crate::settings::{flags, LinterSettings}; use crate::source_kind::SourceKind; use crate::{directives, fs}; +use ruff_diagnostics::Diagnostic; +use ruff_notebook::Notebook; +use ruff_python_ast::imports::ImportMap; +use ruff_python_ast::{PySourceType, Suite}; +use ruff_python_codegen::Stylist; +use ruff_python_index::Indexer; +use ruff_python_parser::lexer::LexResult; +use ruff_python_parser::{AsMode, ParseError}; +use ruff_source_file::{Locator, SourceFileBuilder}; +use ruff_text_size::Ranged; /// A [`Result`]-like type that returns both data and an error. Used to return /// diagnostics even in the face of parse errors, since many diagnostics can be From 3419e5c6856d7786b40b6224da43db381725da82 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:26:20 -0600 Subject: [PATCH 08/17] Fix `fix_only_unsafe_fixes_available` --- crates/ruff/tests/integration_test.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 354e95e6c24a00..d21b44aa470f26 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -1318,17 +1318,18 @@ fn fix_does_not_apply_display_only_fixes_with_unsafe_fixes_enabled() { #[test] fn fix_only_unsafe_fixes_available() { let mut cmd = RuffCheck::default() - .args(["--select", "RUF901", "--fix"]) + .args(["--select", "RUF902", "--fix"]) .build(); assert_cmd_snapshot!(cmd, @r###" - success: true - exit_code: 0 + success: false + exit_code: 1 ----- stdout ----- - # fix from stable-test-rule-safe-fix ----- stderr ----- - Found 1 error (1 fixed, 0 remaining). + -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix. + Found 1 error. + No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option). "###); } From 072c52d890b7f81889f26cc8af67df876b795d4d Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:27:25 -0600 Subject: [PATCH 09/17] Fix ecosystem ignore --- python/ruff-ecosystem/ruff_ecosystem/projects.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/ruff-ecosystem/ruff_ecosystem/projects.py b/python/ruff-ecosystem/ruff_ecosystem/projects.py index 487ed444125f1c..4ed46565d82d7a 100644 --- a/python/ruff-ecosystem/ruff_ecosystem/projects.py +++ b/python/ruff-ecosystem/ruff_ecosystem/projects.py @@ -207,7 +207,8 @@ def to_ruff_args(self) -> list[str]: "--no-cache", "--exit-zero", # Ignore internal test rules - "--ignore RUF9", + "--ignore", + "RUF9", f"--{'' if self.preview else 'no-'}preview", ] if self.select: From 4a3a02db751533c8a76da727dc44167846f806a1 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:28:27 -0600 Subject: [PATCH 10/17] Fix moved ruff imports --- crates/ruff_linter/src/linter.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 4f30cd1c906faa..46f344cad82539 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -8,6 +8,17 @@ use itertools::Itertools; use log::error; use rustc_hash::FxHashMap; +use ruff_diagnostics::Diagnostic; +use ruff_notebook::Notebook; +use ruff_python_ast::imports::ImportMap; +use ruff_python_ast::{PySourceType, Suite}; +use ruff_python_codegen::Stylist; +use ruff_python_index::Indexer; +use ruff_python_parser::lexer::LexResult; +use ruff_python_parser::{AsMode, ParseError}; +use ruff_source_file::{Locator, SourceFileBuilder}; +use ruff_text_size::Ranged; + use crate::checkers::ast::check_ast; use crate::checkers::filesystem::check_file_path; use crate::checkers::imports::check_imports; @@ -28,16 +39,6 @@ use crate::settings::types::UnsafeFixes; use crate::settings::{flags, LinterSettings}; use crate::source_kind::SourceKind; use crate::{directives, fs}; -use ruff_diagnostics::Diagnostic; -use ruff_notebook::Notebook; -use ruff_python_ast::imports::ImportMap; -use ruff_python_ast::{PySourceType, Suite}; -use ruff_python_codegen::Stylist; -use ruff_python_index::Indexer; -use ruff_python_parser::lexer::LexResult; -use ruff_python_parser::{AsMode, ParseError}; -use ruff_source_file::{Locator, SourceFileBuilder}; -use ruff_text_size::Ranged; /// A [`Result`]-like type that returns both data and an error. Used to return /// diagnostics even in the face of parse errors, since many diagnostics can be From 6d20a71085b57a28667546dc4d61b3e3014b17ad Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:31:26 -0600 Subject: [PATCH 11/17] `continue` --- crates/ruff_linter/src/linter.rs | 47 ++++++++++++++++---------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 46f344cad82539..9bed76e5cb5a03 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -220,31 +220,30 @@ pub fn check_path( #[cfg(feature = "test-rules")] { for test_rule in TEST_RULES { - if settings.rules.enabled(*test_rule) { - let diagnostic = match test_rule { - Rule::StableTestRule => { - test_rules::StableTestRule::diagnostic(&locator, &indexer) - } - Rule::StableTestRuleSafeFix => { - test_rules::StableTestRuleSafeFix::diagnostic(&locator, &indexer) - } - Rule::StableTestRuleUnsafeFix => { - test_rules::StableTestRuleUnsafeFix::diagnostic(&locator, &indexer) - } - Rule::StableTestRuleDisplayOnlyFix => { - test_rules::StableTestRuleDisplayOnlyFix::diagnostic(&locator, &indexer) - } - Rule::NurseryTestRule => { - test_rules::NurseryTestRule::diagnostic(&locator, &indexer) - } - Rule::PreviewTestRule => { - test_rules::PreviewTestRule::diagnostic(&locator, &indexer) - } - _ => unreachable!("All test rules must have an implementation"), - }; - if let Some(diagnostic) = diagnostic { - diagnostics.push(diagnostic); + if !settings.rules.enabled(*test_rule) { + continue; + } + let diagnostic = match test_rule { + Rule::StableTestRule => test_rules::StableTestRule::diagnostic(&locator, &indexer), + Rule::StableTestRuleSafeFix => { + test_rules::StableTestRuleSafeFix::diagnostic(&locator, &indexer) + } + Rule::StableTestRuleUnsafeFix => { + test_rules::StableTestRuleUnsafeFix::diagnostic(&locator, &indexer) + } + Rule::StableTestRuleDisplayOnlyFix => { + test_rules::StableTestRuleDisplayOnlyFix::diagnostic(&locator, &indexer) + } + Rule::NurseryTestRule => { + test_rules::NurseryTestRule::diagnostic(&locator, &indexer) + } + Rule::PreviewTestRule => { + test_rules::PreviewTestRule::diagnostic(&locator, &indexer) } + _ => unreachable!("All test rules must have an implementation"), + }; + if let Some(diagnostic) = diagnostic { + diagnostics.push(diagnostic); } } } From 052edbdfb5eb934cf689361e541abf9c5b2f10af Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 16:35:00 -0600 Subject: [PATCH 12/17] Clippy --- crates/ruff_linter/src/linter.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 9bed76e5cb5a03..003edc4446198c 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -224,22 +224,18 @@ pub fn check_path( continue; } let diagnostic = match test_rule { - Rule::StableTestRule => test_rules::StableTestRule::diagnostic(&locator, &indexer), + Rule::StableTestRule => test_rules::StableTestRule::diagnostic(locator, indexer), Rule::StableTestRuleSafeFix => { - test_rules::StableTestRuleSafeFix::diagnostic(&locator, &indexer) + test_rules::StableTestRuleSafeFix::diagnostic(locator, indexer) } Rule::StableTestRuleUnsafeFix => { - test_rules::StableTestRuleUnsafeFix::diagnostic(&locator, &indexer) + test_rules::StableTestRuleUnsafeFix::diagnostic(locator, indexer) } Rule::StableTestRuleDisplayOnlyFix => { - test_rules::StableTestRuleDisplayOnlyFix::diagnostic(&locator, &indexer) - } - Rule::NurseryTestRule => { - test_rules::NurseryTestRule::diagnostic(&locator, &indexer) - } - Rule::PreviewTestRule => { - test_rules::PreviewTestRule::diagnostic(&locator, &indexer) + test_rules::StableTestRuleDisplayOnlyFix::diagnostic(locator, indexer) } + Rule::NurseryTestRule => test_rules::NurseryTestRule::diagnostic(locator, indexer), + Rule::PreviewTestRule => test_rules::PreviewTestRule::diagnostic(locator, indexer), _ => unreachable!("All test rules must have an implementation"), }; if let Some(diagnostic) = diagnostic { From 8247b8582b4b80900a0ef7e984a035283b4558b6 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 22:19:03 -0600 Subject: [PATCH 13/17] Update proc macro to preserve attributes that are common across all child codes --- crates/ruff_macros/src/map_codes.rs | 20 ++++++----- crates/ruff_macros/src/rule_code_prefix.rs | 40 ++++++++++++++++------ 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index 3a68af8e25258b..e81fac4cdd33e6 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -8,7 +8,7 @@ use syn::{ Ident, ItemFn, LitStr, Pat, Path, Stmt, Token, }; -use crate::rule_code_prefix::{get_prefix_ident, if_all_same}; +use crate::rule_code_prefix::{get_prefix_ident, intersection_all}; /// A rule entry in the big match statement such a /// `(Pycodestyle, "E112") => (RuleGroup::Nursery, rules::pycodestyle::rules::logical_lines::NoIndentedBlock),` @@ -142,12 +142,13 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { for (prefix, rules) in &rules_by_prefix { let prefix_ident = get_prefix_ident(prefix); - let attr = match if_all_same(rules.iter().map(|(.., attrs)| attrs)) { - Some(attr) => quote!(#(#attr)*), - None => quote!(), + let attrs = intersection_all(rules.iter().map(|(.., attrs)| attrs.as_slice())); + let attrs = match attrs.as_slice() { + [] => quote!(), + [..] => quote!(#(#attrs)*), }; all_codes.push(quote! { - #attr Self::#linter(#linter::#prefix_ident) + #attrs Self::#linter(#linter::#prefix_ident) }); } @@ -159,12 +160,13 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { quote!(#(#attrs)* Rule::#rule_name) }); let prefix_ident = get_prefix_ident(&prefix); - let attr = match if_all_same(rules.iter().map(|(.., attrs)| attrs)) { - Some(attr) => quote!(#(#attr)*), - None => quote!(), + let attrs = intersection_all(rules.iter().map(|(.., attrs)| attrs.as_slice())); + let attrs = match attrs.as_slice() { + [] => quote!(), + [..] => quote!(#(#attrs)*), }; prefix_into_iter_match_arms.extend(quote! { - #attr #linter::#prefix_ident => vec![#(#rule_paths,)*].into_iter(), + #attrs #linter::#prefix_ident => vec![#(#rule_paths,)*].into_iter(), }); } diff --git a/crates/ruff_macros/src/rule_code_prefix.rs b/crates/ruff_macros/src/rule_code_prefix.rs index 89c7543c5b3259..9a3628c8b32a04 100644 --- a/crates/ruff_macros/src/rule_code_prefix.rs +++ b/crates/ruff_macros/src/rule_code_prefix.rs @@ -89,21 +89,39 @@ fn attributes_for_prefix( codes: &BTreeSet, attributes: &BTreeMap, ) -> proc_macro2::TokenStream { - match if_all_same(codes.iter().map(|code| attributes[code])) { - Some(attr) => quote!(#(#attr)*), - None => quote!(), + let attrs = intersection_all(codes.iter().map(|code| attributes[code])); + match attrs.as_slice() { + [] => quote!(), + [..] => quote!(#(#attrs)*), } } -/// If all values in an iterator are the same, return that value. Otherwise, -/// return `None`. -pub(crate) fn if_all_same(iter: impl Iterator) -> Option { - let mut iter = iter.peekable(); - let first = iter.next()?; - if iter.all(|x| x == first) { - Some(first) +/// Collect all the items from an iterable of slices that are present in all slices. +pub(crate) fn intersection_all<'a, T: PartialEq>( + mut slices: impl Iterator, +) -> Vec<&'a T> { + if let Some(slice) = slices.next() { + // Collect all the items in the first slice + let mut intersection = Vec::with_capacity(slice.len()); + for item in slice { + intersection.push(item); + } + // Then remove all of the items that are not present in each of the + // remaining slices + while let Some(slice) = slices.next() { + let mut mismatches = Vec::with_capacity(slice.len()); + for (idx, item) in intersection.iter().enumerate() { + if !slice.contains(item) { + mismatches.push(idx) + } + } + for idx in mismatches { + intersection.remove(idx); + } + } + intersection } else { - None + Vec::new() } } From 2b31bbe95bdfdab7fa10200448015a2d7091228e Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 22:20:03 -0600 Subject: [PATCH 14/17] Remove schema strip --- crates/ruff_linter/src/rule_selector.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/ruff_linter/src/rule_selector.rs b/crates/ruff_linter/src/rule_selector.rs index 4197c4d4478593..2f6e7a5da12ac4 100644 --- a/crates/ruff_linter/src/rule_selector.rs +++ b/crates/ruff_linter/src/rule_selector.rs @@ -289,8 +289,6 @@ mod schema { (!prefix.is_empty()).then(|| prefix.to_string()) })), ) - // Strip the test rules from the schema - .filter(|code| !code.starts_with("RUF9")) .sorted() .map(Value::String) .collect(), From d89a8887a5c0c9f76f31d29f965c16339317038f Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 22:22:04 -0600 Subject: [PATCH 15/17] Clippy --- crates/ruff_macros/src/rule_code_prefix.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_macros/src/rule_code_prefix.rs b/crates/ruff_macros/src/rule_code_prefix.rs index 9a3628c8b32a04..d21ef0ae5a9364 100644 --- a/crates/ruff_macros/src/rule_code_prefix.rs +++ b/crates/ruff_macros/src/rule_code_prefix.rs @@ -108,11 +108,11 @@ pub(crate) fn intersection_all<'a, T: PartialEq>( } // Then remove all of the items that are not present in each of the // remaining slices - while let Some(slice) = slices.next() { + for slice in slices { let mut mismatches = Vec::with_capacity(slice.len()); for (idx, item) in intersection.iter().enumerate() { if !slice.contains(item) { - mismatches.push(idx) + mismatches.push(idx); } } for idx in mismatches { From fafbc251f0ed891e1f0b51c35cb76dc7fcca21d8 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 22:33:02 -0600 Subject: [PATCH 16/17] Fix implementation of `intersection_all` --- crates/ruff_macros/src/rule_code_prefix.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/crates/ruff_macros/src/rule_code_prefix.rs b/crates/ruff_macros/src/rule_code_prefix.rs index d21ef0ae5a9364..04d9f9b1fa42ec 100644 --- a/crates/ruff_macros/src/rule_code_prefix.rs +++ b/crates/ruff_macros/src/rule_code_prefix.rs @@ -106,18 +106,9 @@ pub(crate) fn intersection_all<'a, T: PartialEq>( for item in slice { intersection.push(item); } - // Then remove all of the items that are not present in each of the - // remaining slices + // Then only keep items that are present in each of the remaining slices for slice in slices { - let mut mismatches = Vec::with_capacity(slice.len()); - for (idx, item) in intersection.iter().enumerate() { - if !slice.contains(item) { - mismatches.push(idx); - } - } - for idx in mismatches { - intersection.remove(idx); - } + intersection.retain(|item| slice.contains(item)); } intersection } else { From 8d9b6b5259f917396ab97a6ab42692e3b3b631b9 Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 31 Jan 2024 23:49:07 -0600 Subject: [PATCH 17/17] Separate dev tests in CI --- .github/workflows/ci.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8142b62ef4c0fd..4ca1614154b6cb 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -117,7 +117,10 @@ jobs: tool: cargo-insta - uses: Swatinem/rust-cache@v2 - name: "Run tests" - run: cargo insta test --all --all-features --unreferenced reject + run: cargo insta test --all --exclude ruff_dev --all-features --unreferenced reject + - name: "Run dev tests" + # e.g. generating the schema — these should not run with all features enabled + run: cargo insta test -p ruff_dev --unreferenced reject # Check for broken links in the documentation. - run: cargo doc --all --no-deps env: @@ -146,7 +149,7 @@ jobs: - name: "Run tests" shell: bash # We can't reject unreferenced snapshots on windows because flake8_executable can't run on windows - run: cargo insta test --all --all-features + run: cargo insta test --all --exclude ruff_dev --all-features cargo-test-wasm: name: "cargo test (wasm)"