Enable selection of preview rules #7046

charliermarsh · 2023-09-01T15:38:48Z

One proposal for supporting workflows like:

ruff check --select E .  # Only includes stable E rules.
ruff check --select E --preview .  # Includes stable and preview E rules.
ruff check --select E225 .  # Includes E225, even though it's in preview.
ruff check --select E225 --preview .  # Includes E225.
ruff check --select ALL .  # Includes all stable rules.
ruff check --select ALL --preview .  # Includes all stable and preview rules.

The basic idea is to stop filtering out nursery rules in the macro code, and instead filter them out when we resolve the rule set. This alone wouldn't support selecting individual rules (ruff check --select E225 .), so to fix that, we add a new specificity and RuleCodePrefix, for selectors that extract a single rule.

There are a few downsides to this approach:

All selectors will now appear in the JSON Schema and be supported on the CLI, including those that grab single rules (like E225) and those that only include preview rules (like E22 -- all of the rules under that prefix are in preview, but --select E22 without the preview flag won't work). This is not ideal, but... it's also somewhat rare and should be rarer over time.
Maybe others? Thinking...

crates/ruff/src/rule_selector.rs

charliermarsh · 2023-09-01T15:40:54Z

crates/ruff_workspace/src/configuration.rs

-                        if spec == Specificity::All && !include_preview_rules && rule.is_preview() {
+                        if !matches!(spec, Specificity::Rule)
+                            && !include_preview_rules
+                            && rule.is_preview()


Actually, here's another idea...

Instead of matching on specific selector (like --select E225), what if we allow the selector if all rules in that selector are preview? So e.g. --select CPY would work, as would --select CPY001.

Similarly, --select E2 would turn on the E2 rules even though they're in preview. But --select E would not. The idea being that you'd have to explicitly select a preview category here.

I think that @MichaReiser had a really good point about preview mode possibly changing semantic model behavior and that allowing opt-in to specific preview rules without the --preview flag should probably not be allowed. I think if we really want opt-in to specific preview rules, we can provide --ignore PREVIEW to avoid turning them all on and --select RULE can be used to opt-in one at a time, but the --preview flag would still be required and the behavior of other things could change as a consequence.

I'm a bit undecided and realized that part of the struggle comes from that it isn't entirely clear what use cases we want to enable with preview mode. Understanding that would be important in my view because a) what I pointed out isn't a problem, b) requiring --preview is okay, or c) a nursery category is the better solution.

I think it is safer to require --preview for any preview behavior. Once we start allowing you to opt into it without that setting, we create a lot of complexity. We can always expand the behavior later if it seems like it'd be okay, but as a starting point I'd rather be restrictive.

codspeed-hq · 2023-09-01T15:57:10Z

CodSpeed Performance Report

Merging #7046 will degrade performances by 7.11%

_{⚠️ No base runs were found}

_{Falling back to comparing charlie/cli-preview (b9f0ade) with main (fbc9b5a)}

Summary

❌ 8 regressions
✅ 12 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`charlie/cli-preview`	Change
❌	`linter/default-rules[numpy/ctypeslib.py]`	18.7 ms	19.4 ms	-3.27%
❌	`linter/default-rules[numpy/globals.py]`	2.2 ms	2.3 ms	-4.38%
❌	`linter/default-rules[pydantic/types.py]`	39.4 ms	41.6 ms	-5.37%
❌	`linter/default-rules[large/dataset.py]`	93.6 ms	100.7 ms	-7.11%
❌	`linter/all-rules[numpy/ctypeslib.py]`	33.9 ms	35.3 ms	-4.1%
❌	`linter/all-rules[unicode/pypinyin.py]`	16.9 ms	17.3 ms	-2.33%
❌	`linter/default-rules[unicode/pypinyin.py]`	6.4 ms	6.7 ms	-4.92%
❌	`linter/all-rules[pydantic/types.py]`	72.3 ms	74.7 ms	-3.23%

charliermarsh · 2023-09-01T18:49:33Z

crates/ruff/src/rule_selector.rs

        }
    }
+
+    /// Returns rules matching the selector, taking into account whether preview mode is enabled.
+    pub fn rules(&self, preview: PreviewMode) -> impl Iterator<Item = Rule> + '_ {


@zanieb - Here's an alternative API: we remove the into_iter() so that there's no more implicit selection, and now require that the preview flag is passed in to the iteration API.

charliermarsh · 2023-09-01T18:49:54Z

crates/ruff/src/rule_selector.rs

+    /// Returns rules matching the selector, taking into account whether preview mode is enabled.
+    pub fn rules(&self, preview: PreviewMode) -> impl Iterator<Item = Rule> + '_ {
+        self.all_rules().filter(move |rule| {
+            matches!(self, RuleSelector::Rule { .. }) || preview.is_enabled() || !rule.is_preview()


(If we want to avoid enabling preview selection for specific rules, then we'd want to remove RuleSelector::Rule { .. }) here.)

charliermarsh · 2023-09-01T18:50:37Z

crates/flake8_to_ruff/src/plugin.rs

@@ -331,7 +332,7 @@ pub(crate) fn infer_plugins_from_codes(selectors: &HashSet<RuleSelector>) -> Vec
    .filter(|plugin| {
        for selector in selectors {
            if selector
-                .into_iter()
+                .rules(PreviewMode::Disabled)


Fine to use "disabled" here, since we don't support preview in flake8-to-ruff.

charliermarsh · 2023-09-01T18:56:34Z

crates/ruff/src/settings/defaults.rs

+            rules: PREFIXES
+                .iter()
+                .flat_map(|selector| selector.rules(PreviewMode::default()))
+                .collect(),


I think this is maybe only used in tests...?

charliermarsh · 2023-09-01T18:57:01Z

crates/ruff/src/settings/types.rs

@@ -194,7 +194,7 @@ pub struct PerFileIgnore {

 impl PerFileIgnore {
    pub fn new(pattern: String, prefixes: &[RuleSelector], project_root: Option<&Path>) -> Self {
-        let rules: RuleSet = prefixes.iter().flat_map(IntoIterator::into_iter).collect();
+        let rules: RuleSet = prefixes.iter().flat_map(RuleSelector::all_rules).collect();


This is okay (not to use preview) because this is an ignore, so it's okay if it includes rules that will never be enabled. But we should comment as such.

zanieb · 2023-09-06T19:13:39Z

Work continued in #7195

## Summary  Extends work in #7046 (some relevant discussion there) Changes: - All nursery rules are now referred to as preview rules - Documentation for the nursery is updated to describe preview - Adds a "PREVIEW" selector for preview rules - This is primarily to allow `--preview --ignore PREVIEW --extend-select FOO001,BAR200` - Using `--preview` enables preview rules that match selectors Notable decisions: - Preview rules are not selectable by their rule code without enabling preview - Retains the "NURSERY" selector for backwards compatibility - Nursery rules are selectable by their rule code for backwards compatiblity Additional work: - Selection of preview rules without the "--preview" flag should display a warning - Use of deprecated nursery selection behavior should display a warning - Nursery selection should be removed after some time ## Test Plan  Manual confirmation (i.e. we don't have an preview rules yet just nursery rules so I added a preview rule for manual testing) New unit tests --------- Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>

charliermarsh commented Sep 1, 2023

View reviewed changes

crates/ruff/src/rule_selector.rs Outdated Show resolved Hide resolved

charliermarsh commented Sep 1, 2023

View reviewed changes

Enable selection of preview rules

b9f0ade

charliermarsh force-pushed the charlie/cli-preview branch from d5eb107 to b9f0ade Compare September 1, 2023 15:48

charliermarsh requested a review from zanieb September 1, 2023 15:48

Make preview part of the selector API

ffbbb8d

charliermarsh commented Sep 1, 2023

View reviewed changes

zanieb mentioned this pull request Sep 6, 2023

Update rule selection to respect preview mode #7195

Merged

zanieb closed this Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable selection of preview rules #7046

Enable selection of preview rules #7046

charliermarsh commented Sep 1, 2023

charliermarsh Sep 1, 2023 •

edited

Loading

zanieb Sep 1, 2023

MichaReiser Sep 1, 2023

zanieb Sep 1, 2023

codspeed-hq bot commented Sep 1, 2023

charliermarsh Sep 1, 2023

charliermarsh Sep 1, 2023

charliermarsh Sep 1, 2023

charliermarsh Sep 1, 2023

charliermarsh Sep 1, 2023

zanieb commented Sep 6, 2023

Enable selection of preview rules #7046

Enable selection of preview rules #7046

Conversation

charliermarsh commented Sep 1, 2023

charliermarsh Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codspeed-hq bot commented Sep 1, 2023

Merging #7046 will degrade performances by 7.11%

Summary

Benchmarks breakdown

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Sep 6, 2023

charliermarsh Sep 1, 2023 •

edited

Loading