Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warnings for nursery and preview rule selection #7210

Merged
merged 6 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 189 additions & 0 deletions crates/ruff_cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,195 @@ fn nursery_direct() {
-:1:2: E225 Missing whitespace around operator
Found 1 error.

----- stderr -----
warning: Selection of nursery rule `E225` 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
let args = ["--select", "NURSERY"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: CPY001 Missing copyright notice at top of file
-:1:2: E225 Missing whitespace around operator
Found 2 errors.

----- stderr -----
warning: The `NURSERY` selector has been deprecated. Use the `--preview` flag instead.
"###);
}

#[test]
fn nursery_group_selector_preview_enabled() {
// Only nursery rules should be detected e.g. E225 and a warning should be displayed
let args = ["--select", "NURSERY", "--preview"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: CPY001 Missing copyright notice at top of file
-:1:2: E225 Missing whitespace around operator
Found 2 errors.

----- stderr -----
warning: The `NURSERY` selector has been deprecated. Use the `PREVIEW` selector instead.
"###);
}

#[test]
fn preview_enabled_prefix() {
// E741 and E225 (preview) should both be detected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen to this test when E225 gets promoted to stable? (i don't have good answer for testing this)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question, do we choose a new code? Should we have a couple of rules in a "TEST" category that do nothing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking into this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not working... but an idea zanie/rule-warning...zanie/rule-tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit hesitant about adding test-only rules. We would need to filter them out in documentation, schema generation and prevent they can be selected in production (but then, we want them to be selectable in tests).

We could add the category behind a rust feature and make this an integration test (integration tests can require features), but this still suffers from the above-mentioned problems, although it has some more safeguards around it.

To me this is an inherent downside of our statically generated rule schema (instead of deriving a Rule array that has somewhat weaker keys which would allow you to mock out the rules).

I don't have a good solution to propose that doesn't require changing our rule structure or abstracting some of the logic by a trait that then allows overriding whether a rule is preview or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the #[cfg(test)] approach address most of those concerns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also wait and see how much of a burden it is to maintain the tests as-is. I don't like it though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc #[cfg(test)] is only available in the same crate and not across crates (https://users.rust-lang.org/t/what-are-the-rules-for-cfg-test/54122/2), so e.g. ruff cfg(test) rules would not be available in ruff_cli.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let args = ["--select", "E", "--preview"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `I`
-:1:2: E225 Missing whitespace around operator
Found 2 errors.

----- stderr -----
"###);
}

#[test]
fn preview_enabled_all() {
let args = ["--select", "ALL", "--preview"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @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.

----- stderr -----
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.
"###);
}

#[test]
fn preview_enabled_direct() {
// E225 should be detected without warning
let args = ["--select", "E225", "--preview"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:2: E225 Missing whitespace around operator
Found 1 error.

----- stderr -----
"###);
}

#[test]
fn preview_disabled_direct() {
// FURB145 is preview not nursery so selecting should be empty
let args = ["--select", "FURB145"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("a = l[:]\n"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: Selection `FURB145` 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 args = ["--select", "CPY"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: Selection `CPY` has no effect because the `--preview` flag was not included.
"###);
}

#[test]
fn preview_disabled_group_selector() {
// `--select PREVIEW` should warn without the `--preview` flag
let args = ["--select", "PREVIEW"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: Selection `PREVIEW` has no effect because the `--preview` flag was not included.
"###);
}

#[test]
fn preview_enabled_group_selector() {
// `--select PREVIEW` is okay with the `--preview` flag and shouldn't warn
let args = ["--select", "PREVIEW", "--preview"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: CPY001 Missing copyright notice at top of file
-:1:2: E225 Missing whitespace around operator
Found 2 errors.

----- stderr -----
"###);
}

#[test]
fn preview_enabled_group_ignore() {
// `--select E --ignore PREVIEW` should detect E741 and E225, which is in preview but "E" is more specific.
let args = ["--select", "E", "--ignore", "PREVIEW", "--preview"];
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(args)
.pass_stdin("I=42\n"), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `I`
-:1:2: E225 Missing whitespace around operator
Found 2 errors.

----- stderr -----
"###);
}
Expand Down
43 changes: 40 additions & 3 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use ruff::settings::types::{
Version,
};
use ruff::settings::{defaults, resolve_per_file_ignores, AllSettings, CliSettings, Settings};
use ruff::{fs, warn_user_once_by_id, RuleSelector, RUFF_PKG_VERSION};
use ruff::{fs, warn_user, warn_user_once, warn_user_once_by_id, RuleSelector, RUFF_PKG_VERSION};
use ruff_cache::cache_dir;
use rustc_hash::{FxHashMap, FxHashSet};
use shellexpand;
Expand Down Expand Up @@ -460,7 +460,10 @@ impl Configuration {
let mut carryover_ignores: Option<&[RuleSelector]> = None;
let mut carryover_unfixables: Option<&[RuleSelector]> = None;

// Store selectors for displaying warnings
let mut redirects = FxHashMap::default();
let mut deprecated_nursery_selectors = FxHashSet::default();
let mut ignored_preview_selectors = FxHashSet::default();

for selection in &self.rule_selections {
// If a selection only specifies extend-select we cannot directly
Expand Down Expand Up @@ -571,8 +574,7 @@ impl Configuration {
}
}

// We insert redirects into the hashmap so that we
// can warn the users about remapped rule codes.
// Check for selections that require a warning
for selector in selection
.select
.iter()
Expand All @@ -583,6 +585,29 @@ impl Configuration {
.chain(selection.unfixable.iter())
.chain(selection.extend_fixable.iter())
{
#[allow(deprecated)]
if matches!(selector, RuleSelector::Nursery) {
let suggestion = if preview.is_disabled() {
"Use the `--preview` flag instead."
} else {
"Use the `PREVIEW` selector instead."
};
warn_user_once!("The `NURSERY` selector has been deprecated. {suggestion}");
}

if preview.is_disabled() {
if let RuleSelector::Rule { prefix, .. } = selector {
if prefix.rules().any(|rule| rule.is_nursery()) {
deprecated_nursery_selectors.insert(selector);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this like: you selected CPY001, but preview is disabled.


// Check if the selector is empty because preview mode is disabled
if selector.rules(PreviewMode::Disabled).next().is_none() {
ignored_preview_selectors.insert(selector);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool so this is like -- you selected CPY, but preview is disabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I think the exact code would warn too. We could make this "safer" by adding && selector.rules(PreviewMode::Enabled).next().is_some() but I think it's equivalent for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't write an integration test for exact codes yet since we don't have any preview rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is like -- you selected CPY, but preview is disabled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do both warnings show if you select CPY001? (Should they?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the former

}

if let RuleSelector::Prefix {
prefix,
redirected_from: Some(redirect_from),
Expand All @@ -603,6 +628,18 @@ impl Configuration {
);
}

for selection in deprecated_nursery_selectors {
let (prefix, code) = selection.prefix_and_code();
warn_user!("Selection of nursery rule `{prefix}{code}` without the `--preview` flag is deprecated.",);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this doesn't use warn_user_once_by_id, the warnings will probably show up multiple times if we run this configuration resolution multiple times (which we might in a given invocation -- I can't remember exactly). Is it possible to use warn_user_once_by_id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but I'd have to construct an id string like "nursery-{prefix}{code}", is that okay? I was thinking of adding a warn_user_once_by_message utility as an alternative? 🤷‍♀️

Copy link
Member

@charliermarsh charliermarsh Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's okay (to add an ID like that)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think the ID has to be a static string so that may not work...

}
Comment on lines +631 to +634
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redirect warning code has a comment that says this should be in the ruff_cli crate — how would we go about that? I'd like to create a tracking issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote to just remove that TODO -- I can't remember why it's there.


for selection in ignored_preview_selectors {
let (prefix, code) = selection.prefix_and_code();
warn_user!(
"Selection `{prefix}{code}` has no effect because the `--preview` flag was not included.",
);
}

let mut rules = RuleTable::empty();

for rule in select_set {
Expand Down
Loading