-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Apply fix availability and applicability when adding to DiagnosticGuard and remove NoqaCode::rule
#18834
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
Apply fix availability and applicability when adding to DiagnosticGuard and remove NoqaCode::rule
#18834
Changes from all commits
a5a6bee
c814520
b47daff
b2b65e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -28,7 +28,7 @@ use itertools::Itertools; | |||
| use log::debug; | ||||
| use rustc_hash::{FxHashMap, FxHashSet}; | ||||
|
|
||||
| use ruff_diagnostics::IsolationLevel; | ||||
| use ruff_diagnostics::{Applicability, Fix, IsolationLevel}; | ||||
| use ruff_notebook::{CellOffsets, NotebookIndex}; | ||||
| use ruff_python_ast::helpers::{collect_import_from_member, is_docstring_stmt, to_module_path}; | ||||
| use ruff_python_ast::identifier::Identifier; | ||||
|
|
@@ -3115,7 +3115,6 @@ pub(crate) struct LintContext<'a> { | |||
| diagnostics: RefCell<Vec<OldDiagnostic>>, | ||||
| source_file: SourceFile, | ||||
| rules: RuleTable, | ||||
| #[expect(unused, reason = "TODO(brent) use this instead of Checker::settings")] | ||||
| settings: &'a LinterSettings, | ||||
| } | ||||
|
|
||||
|
|
@@ -3152,6 +3151,7 @@ impl<'a> LintContext<'a> { | |||
| DiagnosticGuard { | ||||
| context: self, | ||||
| diagnostic: Some(OldDiagnostic::new(kind, range, &self.source_file)), | ||||
| rule: T::rule(), | ||||
| } | ||||
| } | ||||
|
|
||||
|
|
@@ -3165,10 +3165,12 @@ impl<'a> LintContext<'a> { | |||
| kind: T, | ||||
| range: TextRange, | ||||
| ) -> Option<DiagnosticGuard<'chk, 'a>> { | ||||
| if self.is_rule_enabled(T::rule()) { | ||||
| let rule = T::rule(); | ||||
| if self.is_rule_enabled(rule) { | ||||
| Some(DiagnosticGuard { | ||||
| context: self, | ||||
| diagnostic: Some(OldDiagnostic::new(kind, range, &self.source_file)), | ||||
| rule, | ||||
| }) | ||||
| } else { | ||||
| None | ||||
|
|
@@ -3220,6 +3222,7 @@ pub(crate) struct DiagnosticGuard<'a, 'b> { | |||
| /// | ||||
| /// This is always `Some` until the `Drop` (or `defuse`) call. | ||||
| diagnostic: Option<OldDiagnostic>, | ||||
| rule: Rule, | ||||
| } | ||||
|
|
||||
| impl DiagnosticGuard<'_, '_> { | ||||
|
|
@@ -3232,6 +3235,50 @@ impl DiagnosticGuard<'_, '_> { | |||
| } | ||||
| } | ||||
|
|
||||
| impl DiagnosticGuard<'_, '_> { | ||||
| fn resolve_applicability(&self, fix: &Fix) -> Applicability { | ||||
| self.context | ||||
| .settings | ||||
| .fix_safety | ||||
| .resolve_applicability(self.rule, fix.applicability()) | ||||
| } | ||||
|
|
||||
| /// Set the [`Fix`] used to fix the diagnostic. | ||||
| #[inline] | ||||
| pub(crate) fn set_fix(&mut self, fix: Fix) { | ||||
| if !self.context.rules.should_fix(self.rule) { | ||||
| self.fix = None; | ||||
| return; | ||||
| } | ||||
|
Comment on lines
+3249
to
+3252
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this match Ruff's current behavior? Does that mean, we won't even show fixes when the fix is disabled. This seems wrong to me. I'm not suggesting we should necessarily change this in this PR but we should at least open an issue for what I consider a bug. Instead, Ruff should keep the fix (but mark it in some way) and we then skip it when applying automatic fixes (and we don't show it in the LSP although I think even that would be fine). Which makes me wonder. Should
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this does seem to match the current behavior in both the playground and in VS Code, with the caveat that I couldn't get a [lint]
unfixable = ["F401"]to work in VS Code, only setting it in my I think the LSP filters out fixes with display-only applicability here: ruff/crates/ruff_server/src/lint.rs Line 247 in b2b65e7
But if we change that to if !self.context.rules.should_fix(self.rule) {
self.fix = Some(fix.with_applicability(Applicability::DisplayOnly));
return;
}we can show display-only fixes in the editor, even if they're disabled. That definitely makes sense to me.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's create a follow up issue for this. I think it requires some design work on how we want to handle them in the LSP. |
||||
| let applicability = self.resolve_applicability(&fix); | ||||
| self.fix = Some(fix.with_applicability(applicability)); | ||||
| } | ||||
|
|
||||
| /// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`. | ||||
| /// Otherwise, log the error. | ||||
| #[inline] | ||||
| pub(crate) fn try_set_fix(&mut self, func: impl FnOnce() -> anyhow::Result<Fix>) { | ||||
| match func() { | ||||
| Ok(fix) => self.set_fix(fix), | ||||
| Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err), | ||||
| } | ||||
| } | ||||
|
|
||||
| /// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`. | ||||
| /// Otherwise, log the error. | ||||
| #[inline] | ||||
| pub(crate) fn try_set_optional_fix( | ||||
| &mut self, | ||||
| func: impl FnOnce() -> anyhow::Result<Option<Fix>>, | ||||
| ) { | ||||
| match func() { | ||||
| Ok(None) => {} | ||||
| Ok(Some(fix)) => self.set_fix(fix), | ||||
| Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err), | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| impl std::ops::Deref for DiagnosticGuard<'_, '_> { | ||||
| type Target = OldDiagnostic; | ||||
|
|
||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now seems to be the only reason why we need
::strum_macros::EnumStringwhich requires a fair amount of code just to make this work.I think we can get something comparable without going through rule by building an ad-hoc interner by rule id:
The deserialization can then lookup the rule in
rulesand clone the value.This will also allow us to remove the
SerializeandDeserializederive fromRule(And I think we can remove
Ord,PartialOrd, andCacheKeytooUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the type of the
IndexMaphere? I think we still need to get back to theRulebecause it's needed to reconstruct theOldDiagnosticinOldDiagnostic::lint.Another option here could be
Rule::from_code(&msg.noqa_code()?.to_string()).ok()?). And then we could store theRuleas itsrepr(u16)in the cache. That should allow us to remove all of the same derives? (Although it does add back one derive:strum_macros::FromReprfor the conversion in the other direction)Patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see. It's because of the
&'static strinLintId. I don't think what I suggested then makes sense. But this does feel unfortunate. It particularly annoys me thatfilter_mapseems a bit like a footgun. It's very easy to filter out too many diagnostics (e.g. if Ruff ever starts using any otherDiagnosticIdother than syntax error). It even becomes more fragile when we makenoqa_code(secondary code) anOptiononruff_db::Diagnostic.I think the options are:
LintNameuse aString. Which doesn't feel great as wellLintId. That's probably the way to go but I'm okay to defer this to another PR.It's unfortunate that we can't remove all
FromReprcalls or is there a way to get to the rule from theLintName? I think that would be better if possibleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we kept only
EnumString, we could serialize theLintNameas aStringhere and then parse back to a rule when deserializing. That should eliminate all of the derives exceptEnumString. Would that be any better?Then we could also replace this
filter_mapwith afilter(|msg| !msg.is_syntax_error()), I think. Although that still doesn't really address using otherDiagnosticIds since I don't think those could be parsed back toRules either.But yeah, we're just using the
Ruleto retrieve the&'static strlint name. Otherwise we could just (de)serialize the lint name and theOption<NoqaCode>(well, probably as anOption<String>) directly.How are we planning to handle this caching in ty? It seems like there would be issues with the
&'static strthere too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by
EnumString.Yes, I think we would have the same issue. I think going through
Rules here is probably fine (that's what I would do in ty).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
::strum_macros::EnumString, which I thought spawned this thread. We're currently usingEnumStringto parse a rule name to aRulewhen serializing. But if the goal is to remove several derive macros, we could instead serialize the string and callname.parse().unwrap()when deserializing. That would remove the need for all of these macros1:ruff/crates/ruff_macros/src/map_codes.rs
Lines 456 to 462 in 291413b
except
EnumString, which was the original target. I may have lost the thread here, though, if you meant something else entirely.Footnotes
We also still need
IntoStaticStr, which we use for theDisplayimplementation, but that's a bit beside the point. ↩There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, sorry. That's on me. My main goal was to avoid needing to go back to
Rule.I don't think I feel very strongly about this. Removing
EnumStringwould definitely be nice and I could see other ways to avoid thename.parse()overhead (... bring back my interner ;)).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I think I'll merge this for now then. I think there will definitely be an interner in our future! 😄