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.

Sign up for GitHub

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

Jump to bottom

Support negated patterns in [extend-]per-file-ignores #10852

Merged
merged 3 commits into from Apr 10, 2024
Merged

Support negated patterns in [extend-]per-file-ignores #10852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac8215f
Support negated patterns in [extend-]per-file-ignores
carljm Apr 10, 2024
cffcacc
fix windows test, I hope
carljm Apr 10, 2024
8682f45
switch to using test-only rules
carljm Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 91 additions & 0 deletions crates/ruff/tests/lint.rs
View file
Open in desktop
Expand Up @@ -1168,3 +1168,94 @@ def func():

Ok(())
}

/// Per-file selects via ! negation in per-file-ignores
#[test]
fn negated_per_file_ignores() -> Result<()> {
let tempdir = TempDir::new()?;
let ruff_toml = tempdir.path().join("ruff.toml");
fs::write(
&ruff_toml,
r#"
[lint.per-file-ignores]
"!selected.py" = ["F"]
carljm marked this conversation as resolved.
Show resolved Hide resolved
"#,
)?;
let selected = tempdir.path().join("selected.py");
fs::write(
selected,
r#"
import os
"#,
)?;
let ignored = tempdir.path().join("ignored.py");
fs::write(
ignored,
r#"
import os
"#,
)?;

assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.arg("--config")
.arg(&ruff_toml)
.current_dir(&tempdir)
, @r###"
success: false
exit_code: 1
----- stdout -----
selected.py:2:8: F401 [*] `os` imported but unused
Found 1 error.
[*] 1 fixable with the `--fix` option.

----- stderr -----
"###);
Ok(())
}

#[test]
fn negated_per_file_ignores_absolute() -> Result<()> {
let tempdir = TempDir::new()?;
let ruff_toml = tempdir.path().join("ruff.toml");
fs::write(
&ruff_toml,
r#"
[lint.per-file-ignores]
"!src/**.py" = ["F"]
"#,
)?;
let src_dir = tempdir.path().join("src");
fs::create_dir(&src_dir)?;
let selected = src_dir.join("selected.py");
fs::write(
selected,
r#"
import os
"#,
)?;
let ignored = tempdir.path().join("ignored.py");
fs::write(
ignored,
r#"
import os
"#,
)?;

assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.arg("--config")
.arg(&ruff_toml)
.current_dir(&tempdir)
, @r###"
success: false
exit_code: 1
----- stdout -----
src/selected.py:2:8: F401 [*] `os` imported but unused
Found 1 error.
[*] 1 fixable with the `--fix` option.

----- stderr -----
"###);
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth adding some more tests where there are both regular patterns and negated patterns. And in particular, cases where a negated pattern might try to "override" a previous pattern. e.g.,

"*.py" = ["RUF"]
"!foo.py" = ["RUF"]

Or something like that.

Copy link
Contributor Author

@carljm carljm Apr 10, 2024 •
edited

Choose a reason for hiding this comment

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

Yeah, that's a great point, I'll put up an update with a test for this case, and probably also more clearly document how this works.

(I don't expect to change the implementation as part of this; I think the way it needs to work is how it does now: we go through each pattern, and if the pattern matches (whether that's a positive match or a negated non-match), we add the listed rules as ignored; we never un-ignore rules based on failure to match a pattern. I think that would get too complex to understand pretty quickly, and also un-ignoring doesn't really make sense for an option named per-file-ignores.)

33 changes: 23 additions & 10 deletions crates/ruff_linter/src/fs.rs
View file
Open in desktop
Expand Up @@ -9,24 +9,37 @@ use crate::registry::RuleSet;
/// Create a set with codes matching the pattern/code pairs.
pub(crate) fn ignores_from_path(
path: &Path,
pattern_code_pairs: &[(GlobMatcher, GlobMatcher, RuleSet)],
pattern_code_pairs: &[(GlobMatcher, GlobMatcher, bool, RuleSet)],
) -> RuleSet {
let file_name = path.file_name().expect("Unable to parse filename");
pattern_code_pairs
.iter()
.filter_map(|(absolute, basename, rules)| {
.filter_map(|(absolute, basename, negated, rules)| {
if basename.is_match(file_name) {
debug!(
"Adding per-file ignores for {:?} due to basename match on {:?}: {:?}",
path,
basename.glob().regex(),
rules
);
Some(rules)
if *negated { None } else {
debug!(
"Adding per-file ignores for {:?} due to basename match on {:?}: {:?}",
path,
basename.glob().regex(),
rules
);
Some(rules)
}
} else if absolute.is_match(path) {
if *negated { None } else {
debug!(
"Adding per-file ignores for {:?} due to absolute match on {:?}: {:?}",
path,
absolute.glob().regex(),
rules
);
Some(rules)
}
} else if *negated {
debug!(
"Adding per-file ignores for {:?} due to absolute match on {:?}: {:?}",
"Adding per-file ignores for {:?} due to negated pattern matching neither {:?} nor {:?}: {:?}",
path,
basename.glob().regex(),
absolute.glob().regex(),
rules
);
Expand Down
27 changes: 21 additions & 6 deletions crates/ruff_linter/src/settings/types.rs
View file
Open in desktop
Expand Up @@ -296,13 +296,22 @@ impl CacheKey for FilePatternSet {
pub struct PerFileIgnore {
pub(crate) basename: String,
pub(crate) absolute: PathBuf,
pub(crate) negated: bool,
pub(crate) rules: RuleSet,
}

impl PerFileIgnore {
pub fn new(pattern: String, prefixes: &[RuleSelector], project_root: Option<&Path>) -> Self {
pub fn new(
mut pattern: String,
prefixes: &[RuleSelector],
project_root: Option<&Path>,
) -> Self {
// Rules in preview are included here even if preview mode is disabled; it's safe to ignore disabled rules
let rules: RuleSet = prefixes.iter().flat_map(RuleSelector::all_rules).collect();
let negated = pattern.starts_with('!');
if negated {
pattern.drain(..1);
}
Comment on lines +312 to +314
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might look a little odd, but it's zero-allocation, unlike the version using .strip_prefix().

Perf probably doesn't matter much here in config-parsing; if you'd prefer the version that takes a non-mut String and uses .strip_prefix(), I can switch to that, though the code actually ends up longer than this.

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 cool with this.

Copy link
Member

Choose a reason for hiding this comment

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

\cc @BurntSushi may have interesting things to say here :)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine personally. I agree that saving an alloc probably doesn't matter here (I assume these ultimately get converted to a glob matcher, and that is an enormous amount of work compared to saving an alloc) so I'd write whatever is clear. And this seems clear to me. :)

let path = Path::new(&pattern);
let absolute = match project_root {
Some(project_root) => fs::normalize_path_to(path, project_root),
Expand All @@ -312,6 +321,7 @@ impl PerFileIgnore {
Self {
basename: pattern,
absolute,
negated,
rules,
}
}
Expand Down Expand Up @@ -593,7 +603,7 @@ pub type IdentifierPattern = glob::Pattern;
#[derive(Debug, Clone, CacheKey, Default)]
pub struct PerFileIgnores {
// Ordered as (absolute path matcher, basename matcher, rules)
ignores: Vec<(GlobMatcher, GlobMatcher, RuleSet)>,
ignores: Vec<(GlobMatcher, GlobMatcher, bool, RuleSet)>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be a named struct (CompiledPerFileIgnore? PerFileIgnoreMatcher?) with named fields rather than a tuple, but for ease of review I didn't make that change here; will push it as a separate PR (unless a reviewer suggests I shouldn't.)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

}

impl PerFileIgnores {
Expand All @@ -609,7 +619,12 @@ impl PerFileIgnores {
// Construct basename matcher.
let basename = Glob::new(&per_file_ignore.basename)?.compile_matcher();

Ok((absolute, basename, per_file_ignore.rules))
Ok((
absolute,
basename,
per_file_ignore.negated,
per_file_ignore.rules,
))
})
.collect();
Ok(Self { ignores: ignores? })
Expand All @@ -622,10 +637,10 @@ impl Display for PerFileIgnores {
write!(f, "{{}}")?;
} else {
writeln!(f, "{{")?;
for (absolute, basename, rules) in &self.ignores {
for (absolute, basename, negated, rules) in &self.ignores {
writeln!(
f,
"\t{{ absolute = {absolute:#?}, basename = {basename:#?}, rules = {rules} }},"
"\t{{ absolute = {absolute:#?}, basename = {basename:#?}, negated = {negated:#?}, rules = {rules} }},"
)?;
}
write!(f, "}}")?;
Expand All @@ -635,7 +650,7 @@ impl Display for PerFileIgnores {
}

impl Deref for PerFileIgnores {
type Target = Vec<(GlobMatcher, GlobMatcher, RuleSet)>;
type Target = Vec<(GlobMatcher, GlobMatcher, bool, RuleSet)>;

fn deref(&self) -> &Self::Target {
&self.ignores
Expand Down
5 changes: 4 additions & 1 deletion crates/ruff_workspace/src/options.rs
View file
Open in desktop
Expand Up @@ -905,7 +905,8 @@ pub struct LintCommonOptions {

// Tables are required to go last.
/// A list of mappings from file pattern to rule codes or prefixes to
/// exclude, when considering any matching files.
/// exclude, when considering any matching files. An initial '!' negates
/// the file pattern.
#[option(
default = "{}",
value_type = "dict[str, list[RuleSelector]]",
Expand All @@ -914,6 +915,8 @@ pub struct LintCommonOptions {
# Ignore `E402` (import violations) in all `__init__.py` files, and in `path/to/file.py`.
"__init__.py" = ["E402"]
"path/to/file.py" = ["E402"]
# Ignore `D` rules everywhere except for the `src/` directory.
Copy link

Choose a reason for hiding this comment

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

Is this comment correct? Looks like it ignored F401 not D

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, it should be D below (or the comment should change to F401) \cc @carljm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks, will fix!

"!src/**.py" = ["F401"]
"#
)]
pub per_file_ignores: Option<FxHashMap<String, Vec<RuleSelector>>>,
Expand Down
4 changes: 2 additions & 2 deletions ruff.schema.json
View file
Open in desktop

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