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

Update rule selection to respect preview mode #7195

Merged
merged 34 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
9b4aee1
Rename a bunch of "nursery" references to "preview"
zanieb Aug 31, 2023
8fce454
Update ALL rule selector to include all rules then deselect preview r…
zanieb Aug 31, 2023
c527f69
Clarify `include_preview_rules` conditional
zanieb Aug 31, 2023
a954fd6
Fix conditional
zanieb Aug 31, 2023
2e44d66
WIP: Rename rule selector from nursery to preview
zanieb Aug 31, 2023
51bce02
Enable selection of preview rules
charliermarsh Sep 1, 2023
1689190
Make preview part of the selector API
charliermarsh Sep 1, 2023
2c830e2
Fix linter group comment
zanieb Sep 5, 2023
f111189
Add comment to `PerFileIgnore`
zanieb Sep 5, 2023
e6d491a
Restore nursery rule group for backwards compatible selection
zanieb Sep 5, 2023
97b80c0
Restore nursery selector
zanieb Sep 6, 2023
2b0dfd6
Fix macro generated `RuleSelector::from(code)` support for `RuleSelec…
zanieb Sep 6, 2023
359012a
Fix pyproject test
zanieb Sep 6, 2023
b60ce8b
Remove `prefix_to_selector`
zanieb Sep 6, 2023
950bd95
Update FAQ
zanieb Sep 6, 2023
ae80063
Fix benchmark rule selection
zanieb Sep 8, 2023
5ae0075
Clippy
zanieb Sep 6, 2023
a5200ed
Fix typo
zanieb Sep 6, 2023
83d888f
Add support for NURSERY selector (backwards compat)
zanieb Sep 6, 2023
45ceb6a
Disable preview mode during benchmarks
zanieb Sep 8, 2023
cb47f02
Separate test cases for selection
zanieb Sep 6, 2023
2d8f961
Add preview flag to test utility
zanieb Sep 6, 2023
550f787
Add tests for preview / nursery
zanieb Sep 6, 2023
c581006
Add test for select of linter; fix names of existing tests
zanieb Sep 6, 2023
f628e95
Add test cases for selection of NURSERY and PREVIEW groups
zanieb Sep 6, 2023
c092d62
Fix typo
zanieb Sep 6, 2023
32b34fa
Add unreachable code to test cases
zanieb Sep 6, 2023
fbd449a
Fix test case handling of RUF014
zanieb Sep 6, 2023
d94dfd7
Fix preview mode messages
zanieb Sep 7, 2023
e7f095a
Deprecate `RuleSelector::Nursery`
zanieb Sep 7, 2023
b09fbb9
Deprecate `RuleGroup::Nursery`
zanieb Sep 7, 2023
b374802
Fix deprecated usage in test
zanieb Sep 7, 2023
4ea6f17
Improve prefix comments
zanieb Sep 8, 2023
2cab8b7
Fix unresolved merge conflict
zanieb Sep 11, 2023
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
Prev Previous commit
Next Next commit
Enable selection of preview rules
  • Loading branch information
charliermarsh authored and zanieb committed Sep 8, 2023
commit 51bce024d5151cff6f8499089046c0e7cc97bb62
86 changes: 59 additions & 27 deletions crates/ruff/src/rule_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ pub enum RuleSelector {
prefix: RuleCodePrefix,
redirected_from: Option<&'static str>,
},
/// Select an individual rule with a given prefix.
Rule {
prefix: RuleCodePrefix,
redirected_from: Option<&'static str>,
},
}

impl From<Linter> for RuleSelector {
Expand Down Expand Up @@ -61,16 +66,42 @@ impl FromStr for RuleSelector {
return Ok(Self::Linter(linter));
}

Ok(Self::Prefix {
prefix: RuleCodePrefix::parse(&linter, code)
.map_err(|_| ParseError::Unknown(s.to_string()))?,
redirected_from,
})
// Does the selector select a single rule?
let prefix = RuleCodePrefix::parse(&linter, code)
.map_err(|_| ParseError::Unknown(s.to_string()))?;
if is_single_rule_selector(&prefix) {
Ok(Self::Rule {
prefix,
redirected_from,
})
} else {
Ok(Self::Prefix {
prefix,
redirected_from,
})
}
}
}
}
}

/// Returns `true` if the [`RuleCodePrefix`] matches a single rule exactly
/// (e.g., `E225`, as opposed to `E2`).
fn is_single_rule_selector(prefix: &RuleCodePrefix) -> bool {
let mut rules = prefix.rules();

// The selector must match a single rule.
let Some(rule) = rules.next() else {
return false;
};
if rules.next().is_some() {
return false;
}

// The rule must match the selector exactly.
rule.noqa_code().suffix() == prefix.short_code()
}
Copy link
Member

Choose a reason for hiding this comment

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

I bet this could be encoded in the type system somehow, this feels awkward (I know that I wrote it lol). Not asking for a change here since it will likely involve more extensive changes in the macro, etc., just expressing that it feels strange.

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 will open a tracking issue once this is merged


#[derive(Debug, thiserror::Error)]
pub enum ParseError {
#[error("Unknown rule selector: `{0}`")]
Expand All @@ -86,7 +117,7 @@ impl RuleSelector {
RuleSelector::Preview => ("", "PREVIEW"),
RuleSelector::C => ("", "C"),
RuleSelector::T => ("", "T"),
RuleSelector::Prefix { prefix, .. } => {
RuleSelector::Prefix { prefix, .. } | RuleSelector::Rule { prefix, .. } => {
(prefix.linter().common_prefix(), prefix.short_code())
}
RuleSelector::Linter(l) => (l.common_prefix(), ""),
Expand Down Expand Up @@ -137,18 +168,9 @@ impl Visitor<'_> for SelectorVisitor {
}
}

impl From<RuleCodePrefix> for RuleSelector {
fn from(prefix: RuleCodePrefix) -> Self {
Self::Prefix {
prefix,
redirected_from: None,
}
}
}

impl IntoIterator for &RuleSelector {
type IntoIter = RuleSelectorIter;
type Item = Rule;
type IntoIter = RuleSelectorIter;

fn into_iter(self) -> Self::IntoIter {
match self {
Expand All @@ -167,7 +189,9 @@ impl IntoIterator for &RuleSelector {
.chain(Linter::Flake8Print.rules()),
),
RuleSelector::Linter(linter) => RuleSelectorIter::Vec(linter.rules()),
RuleSelector::Prefix { prefix, .. } => RuleSelectorIter::Vec(prefix.clone().rules()),
RuleSelector::Prefix { prefix, .. } | RuleSelector::Rule { prefix, .. } => {
RuleSelectorIter::Vec(prefix.clone().rules())
}
}
}
}
Expand Down Expand Up @@ -270,14 +294,14 @@ impl RuleSelector {
RuleSelector::T => Specificity::LinterGroup,
RuleSelector::C => Specificity::LinterGroup,
RuleSelector::Linter(..) => Specificity::Linter,
RuleSelector::Rule { .. } => Specificity::Rule,
RuleSelector::Prefix { prefix, .. } => {
let prefix: &'static str = prefix.short_code();
match prefix.len() {
1 => Specificity::Code1Char,
2 => Specificity::Code2Chars,
3 => Specificity::Code3Chars,
4 => Specificity::Code4Chars,
5 => Specificity::Code5Chars,
1 => Specificity::Prefix1Char,
2 => Specificity::Prefix2Chars,
3 => Specificity::Prefix3Chars,
4 => Specificity::Prefix4Chars,
_ => panic!("RuleSelector::specificity doesn't yet support codes with so many characters"),
}
}
Expand All @@ -287,14 +311,22 @@ impl RuleSelector {

#[derive(EnumIter, PartialEq, Eq, PartialOrd, Ord, Copy, Clone, Debug)]
pub enum Specificity {
/// The specificity when selecting all rules (e.g., `--select ALL`).
All,
/// The specificity when selecting a linter group (e.g., `--select PL`).
LinterGroup,
/// The specificity when selecting a linter (e.g., `--select PLE` or `--select UP`).
Linter,
Code1Char,
Code2Chars,
Code3Chars,
Code4Chars,
Code5Chars,
/// The specificity when selecting via a rule prefix at one-character depth (e.g., `--select PLE1`).
Prefix1Char,
Copy link
Member

Choose a reason for hiding this comment

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

I got a bit confused with "rule prefix" which made me think of the prefix part of the code i.e., "PLE" in the given example. But I think this is related to the number part, right? Maybe "Suffix1Char"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm well the specificity is that it's a prefix e.g. including "PLE10" but I agree the character count part is confusing since it just refers to the code length.

/// The specificity when selecting via a rule prefix at two-character depth (e.g., `--select PLE12`).
Prefix2Chars,
/// The specificity when selecting via a rule prefix at one-character depth (e.g., `--select PLE120`).
Prefix3Chars,
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
/// The specificity when selecting via a rule prefix at one-character depth (e.g., `--select PLE120`).
Prefix4Chars,
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
/// The specificity when selecting an individual rule (e.g., `--select PLE1205`).
Rule,
}

#[cfg(feature = "clap")]
Expand Down
32 changes: 8 additions & 24 deletions crates/ruff_macros/src/map_codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use syn::{
Ident, ItemFn, LitStr, Pat, Path, Stmt, Token,
};

use crate::rule_code_prefix::{get_prefix_ident, if_all_same, is_preview};
use crate::rule_code_prefix::{get_prefix_ident, if_all_same};

/// A rule entry in the big match statement such a
/// `(Pycodestyle, "E112") => (RuleGroup::Nursery, rules::pycodestyle::rules::logical_lines::NoIndentedBlock),`
Expand Down Expand Up @@ -156,7 +156,7 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result<TokenStream> {

output.extend(quote! {
impl #linter {
pub fn rules(self) -> ::std::vec::IntoIter<Rule> {
pub fn rules(&self) -> ::std::vec::IntoIter<Rule> {
match self { #prefix_into_iter_match_arms }
}
}
Expand All @@ -172,7 +172,7 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result<TokenStream> {
})
}

pub fn rules(self) -> ::std::vec::IntoIter<Rule> {
pub fn rules(&self) -> ::std::vec::IntoIter<Rule> {
match self {
#(RuleCodePrefix::#linter_idents(prefix) => prefix.clone().rules(),)*
}
Expand All @@ -195,26 +195,12 @@ fn rules_by_prefix(
// TODO(charlie): Why do we do this here _and_ in `rule_code_prefix::expand`?
let mut rules_by_prefix = BTreeMap::new();

for (code, rule) in rules {
// Nursery rules have to be explicitly selected, so we ignore them when looking at
// prefix-level selectors (e.g., `--select SIM10`), but add the rule itself under
// its fully-qualified code (e.g., `--select SIM101`).
if is_preview(&rule.group) {
rules_by_prefix.insert(code.clone(), vec![(rule.path.clone(), rule.attrs.clone())]);
continue;
}

for code in rules.keys() {
for i in 1..=code.len() {
let prefix = code[..i].to_string();
let rules: Vec<_> = rules
.iter()
.filter_map(|(code, rule)| {
// Nursery rules have to be explicitly selected, so we ignore them when
// looking at prefixes.
if is_preview(&rule.group) {
return None;
}

if code.starts_with(&prefix) {
Some((rule.path.clone(), rule.attrs.clone()))
} else {
Expand Down Expand Up @@ -336,12 +322,10 @@ fn generate_iter_impl(
let mut linter_rules_match_arms = quote!();
let mut linter_all_rules_match_arms = quote!();
for (linter, map) in linter_to_rules {
let rule_paths = map.values().filter(|rule| !is_preview(&rule.group)).map(
|Rule { attrs, path, .. }| {
let rule_name = path.segments.last().unwrap();
quote!(#(#attrs)* Rule::#rule_name)
},
);
let rule_paths = map.values().map(|Rule { attrs, path, .. }| {
let rule_name = path.segments.last().unwrap();
quote!(#(#attrs)* Rule::#rule_name)
});
linter_rules_match_arms.extend(quote! {
Linter::#linter => vec![#(#rule_paths,)*].into_iter(),
});
Expand Down
27 changes: 4 additions & 23 deletions crates/ruff_macros/src/rule_code_prefix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,14 @@ pub(crate) fn expand<'a>(
let mut prefix_to_codes: BTreeMap<String, BTreeSet<String>> = BTreeMap::default();
let mut code_to_attributes: BTreeMap<String, &[Attribute]> = BTreeMap::default();

for (variant, group, attr) in variants {
for (variant, .., attr) in variants {
let code_str = variant.to_string();
// Nursery rules have to be explicitly selected, so we ignore them when looking at prefixes.
if is_preview(group) {
for i in 1..=code_str.len() {
let prefix = code_str[..i].to_string();
prefix_to_codes
.entry(code_str.clone())
.entry(prefix)
.or_default()
.insert(code_str.clone());
} else {
for i in 1..=code_str.len() {
let prefix = code_str[..i].to_string();
prefix_to_codes
.entry(prefix)
.or_default()
.insert(code_str.clone());
}
}

code_to_attributes.insert(code_str, attr);
Expand Down Expand Up @@ -125,14 +117,3 @@ pub(crate) fn get_prefix_ident(prefix: &str) -> Ident {
};
Ident::new(&prefix, Span::call_site())
}

/// Returns true if the given group is the "preview" group.
pub(crate) fn is_preview(group: &Path) -> bool {
let group = group
.segments
.iter()
.map(|segment| segment.ident.to_string())
.collect::<Vec<String>>()
.join("::");
group == "RuleGroup::Preview"
}
11 changes: 6 additions & 5 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,11 +442,9 @@ impl Configuration {
pub fn as_rule_table(&self) -> RuleTable {
// The select_set keeps track of which rules have been selected.
let mut select_set: RuleSet = defaults::PREFIXES.iter().flatten().collect();

// The fixable set keeps track of which rules are fixable.
let mut fixable_set: RuleSet = RuleSelector::All
.into_iter()
.chain(&RuleSelector::Preview)
.collect();
let mut fixable_set: RuleSet = RuleSelector::All.into_iter().collect();

// Ignores normally only subtract from the current set of selected
// rules. By that logic the ignore in `select = [], ignore = ["E501"]`
Expand Down Expand Up @@ -486,7 +484,10 @@ impl Configuration {
.filter(|s| s.specificity() == spec)
{
for rule in selector {
if spec == Specificity::All && !include_preview_rules && rule.is_preview() {
if !matches!(spec, Specificity::Rule)
&& !include_preview_rules
&& rule.is_preview()
{
continue;
}
select_map_updates.insert(rule, true);
Expand Down
25 changes: 25 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.