[flake8_import_conventions] Avoid false positives for NFKC-normalized __debug__ import aliases in ICN001#19411
Conversation
|
| return; | ||
| }; | ||
|
|
||
| let normalized_alias = expected_alias.nfkc().collect::<String>(); |
There was a problem hiding this comment.
It would be better to do this normalization when loading the settings. We otherwise pay the cost for every import statement
There was a problem hiding this comment.
I'm not sure if I've worked with settings before, how would it be implemented? Via these?
crates\ruff_workspace\src\options.rscrates\ruff_linter\src\rules\flake8_import_conventions\settings.rs
There was a problem hiding this comment.
Ideally, we'd validate the settings when loading the configuration. The idea is to rename the into_settings method to try_into_settings and change its return type to Result, similar to
ruff/crates/ruff_workspace/src/options.rs
Lines 1416 to 1428 in 8c0743d
ntBre
added
bug
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
crates/ruff_workspace/src/options.rs
Outdated
| let normalized_aliases: FxHashMap<String, String> = aliases | ||
| .into_iter() | ||
| .map(|(module, alias)| { | ||
| let normalized_alias = alias.nfkc().collect::<String>(); | ||
| (module, normalized_alias) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
Oh. I don't think we need to use try_into_settings if we only do the normalization here.
What I had in mind is to error if the user provides an alias that normalizes to __debug__. Or are there cases where we use aliases where __debug__ should be allowed?
There was a problem hiding this comment.
I don't think __debug__ is ever valid as an alias. Is this the right idea?
let mut normalized_aliases: FxHashMap<String, String> = FxHashMap::default();
for (module, alias) in aliases {
let normalized_alias = alias.nfkc().collect::<String>();
if normalized_alias == "__debug__" {
anyhow::bail!(
"Invalid alias for module '{module}': alias normalizes to '__debug__', which is not allowed."
);
}
normalized_aliases.insert(module, normalized_alias);
}There was a problem hiding this comment.
Yes, I think that would be the ideal user experience (telling them that the configuration is invalid).
An alternative is to use warn_user_once to log a warning instead (and not adding the alias)
I think either's fine
| if expected_alias == "__debug__" { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this check still required, given that we now error in Flake8ImportConventionsOptions
| #[test] | ||
| fn nfkc_debug_alias() -> Result<()> { | ||
| let mut aliases = default_aliases(); | ||
| aliases.extend(FxHashMap::from_iter([ | ||
| ("collections".to_string(), "__debug__".to_string()), | ||
| ("collections.abc".to_string(), "__debug__".to_string()), | ||
| ])); | ||
| let diagnostics = test_path( | ||
| Path::new("flake8_import_conventions/nfkc_debug_alias.py"), | ||
| &LinterSettings { | ||
| flake8_import_conventions: super::settings::Settings { | ||
| aliases, | ||
| banned_aliases: FxHashMap::default(), | ||
| banned_from: FxHashSet::default(), | ||
| }, | ||
| ..LinterSettings::for_rule(Rule::UnconventionalImportAlias) | ||
| }, | ||
| )?; | ||
| assert!( | ||
| diagnostics.is_empty(), | ||
| "No diagnostics should be emitted for NFKC '__debug__' aliases" | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I don't think it's possible to implement a fix in the linter. We can implement this as a unit test next to Flake8ImportConventionsOptions or as a CLI test (see ruff/tests)
…ve redundant linter test
3 participants
Summary
Fixes #19118