-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Account for selector specificity when merging
extend_unsafe_fixes a…
…nd `override extend_safe_fixes` (#8444) ## Summary Prior to this change `extend_unsafe_fixes` took precedence over `extend_safe_fixes` selectors, so any conflicts were resolved in favour of `extend_unsafe_fixes`. Thanks to that ruff were conservatively assuming that if configs conlict the fix corresponding to selected rule will be treated as unsafe. After this change we take into account Specificity of the selectors. For conflicts between selectors of the same Specificity we will treat the corresponding fixes as unsafe. But if the conflicting selectors are of different specificity the more specific one will win. ## Test Plan Tests were added for the `FixSafetyTable` struct. The `check_extend_unsafe_fixes_conflict_with_extend_safe_fixes_by_specificity` integration test was added to test conflicting rules of different specificity. Fixes #8404 --------- Co-authored-by: Zanie <[email protected]>
- Loading branch information
1 parent
7873ca3
commit 03303a9
Showing
5 changed files
with
251 additions
and
41 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| use std::fmt::Debug; | ||
|
|
||
| use ruff_diagnostics::Applicability; | ||
| use ruff_macros::CacheKey; | ||
| use rustc_hash::FxHashMap; | ||
| use strum::IntoEnumIterator; | ||
|
|
||
| use crate::{ | ||
| registry::{Rule, RuleSet}, | ||
| rule_selector::{PreviewOptions, Specificity}, | ||
| RuleSelector, | ||
| }; | ||
|
|
||
| /// A table to keep track of which rules fixes should have | ||
| /// their safety overridden. | ||
| #[derive(Debug, CacheKey, Default)] | ||
| pub struct FixSafetyTable { | ||
| forced_safe: RuleSet, | ||
| forced_unsafe: RuleSet, | ||
| } | ||
|
|
||
| impl FixSafetyTable { | ||
| pub const fn resolve_applicability( | ||
| &self, | ||
| rule: Rule, | ||
| applicability: Applicability, | ||
| ) -> Applicability { | ||
| match applicability { | ||
| // If applicability is display-only we don't change it | ||
| Applicability::DisplayOnly => applicability, | ||
| Applicability::Safe | Applicability::Unsafe => { | ||
| if self.forced_unsafe.contains(rule) { | ||
| Applicability::Unsafe | ||
| } else if self.forced_safe.contains(rule) { | ||
| Applicability::Safe | ||
| } else { | ||
| applicability | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub const fn is_empty(&self) -> bool { | ||
| self.forced_safe.is_empty() && self.forced_unsafe.is_empty() | ||
| } | ||
|
|
||
| pub fn from_rule_selectors( | ||
| extend_safe_fixes: &[RuleSelector], | ||
| extend_unsafe_fixes: &[RuleSelector], | ||
| preview_options: &PreviewOptions, | ||
| ) -> Self { | ||
| enum Override { | ||
| Safe, | ||
| Unsafe, | ||
| } | ||
|
|
||
| let safety_override_map: FxHashMap<Rule, Override> = { | ||
| Specificity::iter() | ||
| .flat_map(|spec| { | ||
| let safe_overrides = extend_safe_fixes | ||
| .iter() | ||
| .filter(|selector| selector.specificity() == spec) | ||
| .flat_map(|selector| selector.rules(preview_options)) | ||
| .map(|rule| (rule, Override::Safe)); | ||
|
|
||
| let unsafe_overrides = extend_unsafe_fixes | ||
| .iter() | ||
| .filter(|selector| selector.specificity() == spec) | ||
| .flat_map(|selector| selector.rules(preview_options)) | ||
| .map(|rule| (rule, Override::Unsafe)); | ||
|
|
||
| // Unsafe overrides take precedence over safe overrides | ||
| safe_overrides.chain(unsafe_overrides).collect::<Vec<_>>() | ||
| }) | ||
| // More specified selectors take precedence over less specified selectors | ||
| .collect() | ||
| }; | ||
|
|
||
| FixSafetyTable { | ||
| forced_safe: safety_override_map | ||
| .iter() | ||
| .filter_map(|(rule, o)| match o { | ||
| Override::Safe => Some(*rule), | ||
| Override::Unsafe => None, | ||
| }) | ||
| .collect(), | ||
| forced_unsafe: safety_override_map | ||
| .iter() | ||
| .filter_map(|(rule, o)| match o { | ||
| Override::Unsafe => Some(*rule), | ||
| Override::Safe => None, | ||
| }) | ||
| .collect(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_resolve_applicability() { | ||
| let table = FixSafetyTable { | ||
| forced_safe: RuleSet::from_iter([Rule::RedefinedWhileUnused]), | ||
| forced_unsafe: RuleSet::from_iter([Rule::UnusedImport]), | ||
| }; | ||
|
|
||
| for applicability in &[Applicability::Safe, Applicability::Unsafe] { | ||
| assert_eq!( | ||
| table.resolve_applicability(Rule::RedefinedWhileUnused, *applicability), | ||
| Applicability::Safe // It is forced to Safe | ||
| ); | ||
| } | ||
| for applicability in &[Applicability::Safe, Applicability::Unsafe] { | ||
| assert_eq!( | ||
| table.resolve_applicability(Rule::UnusedImport, *applicability), | ||
| Applicability::Unsafe // It is forced to Unsafe | ||
| ); | ||
| } | ||
| for applicability in &[Applicability::Safe, Applicability::Unsafe] { | ||
| assert_eq!( | ||
| table.resolve_applicability(Rule::UndefinedName, *applicability), | ||
| *applicability // Remains unchanged | ||
| ); | ||
| } | ||
|
|
||
| for rule in &[ | ||
| Rule::RedefinedWhileUnused, | ||
| Rule::UnusedImport, | ||
| Rule::UndefinedName, | ||
| ] { | ||
| assert_eq!( | ||
| table.resolve_applicability(*rule, Applicability::DisplayOnly), | ||
| Applicability::DisplayOnly // Display is never changed | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| fn mk_table(safe_fixes: &[&str], unsafe_fixes: &[&str]) -> FixSafetyTable { | ||
| FixSafetyTable::from_rule_selectors( | ||
| &safe_fixes | ||
| .iter() | ||
| .map(|s| s.parse().unwrap()) | ||
| .collect::<Vec<_>>(), | ||
| &unsafe_fixes | ||
| .iter() | ||
| .map(|s| s.parse().unwrap()) | ||
| .collect::<Vec<_>>(), | ||
| &PreviewOptions::default(), | ||
| ) | ||
| } | ||
|
|
||
| fn assert_rules_safety( | ||
| table: &FixSafetyTable, | ||
| assertions: &[(&str, Applicability, Applicability)], | ||
| ) { | ||
| for (code, applicability, expected) in assertions { | ||
| assert_eq!( | ||
| table.resolve_applicability(Rule::from_code(code).unwrap(), *applicability), | ||
| *expected | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_rule_selectors_specificity() { | ||
| use Applicability::{Safe, Unsafe}; | ||
| let table = mk_table(&["UP"], &["ALL", "UP001"]); | ||
|
|
||
| assert_rules_safety( | ||
| &table, | ||
| &[ | ||
| ("E101", Safe, Unsafe), | ||
| ("UP001", Safe, Unsafe), | ||
| ("UP003", Unsafe, Safe), | ||
| ], | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_rule_selectors_unsafe_over_safe() { | ||
| use Applicability::{Safe, Unsafe}; | ||
| let table = mk_table(&["UP"], &["UP"]); | ||
|
|
||
| assert_rules_safety(&table, &[("E101", Safe, Safe), ("UP001", Safe, Unsafe)]); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters