Add a ViolationMetadata::rule method - #18234
Conversation
CodSpeed Performance ReportMerging #18234 will improve performances by 21.11%Comparing Summary
Benchmarks breakdown
|
|
|
Those benchmarks are pretty promising, maybe this is worth pursuing. |
|
The speedups look promising. However, I don't think we want or even can store a I think my overall preference would be to reduce the need to access |
Summary -- This is a rough draft/experiment to associate a `Rule` with a `Violation` type for cheaper conversions between the two than the current approach of matching on the stringified names. It also moves `Violation` and `Diagnostic` and some related types out of `ruff_diagnostics` and into the linter to make the macro work. This could definitely be a bad idea, but it seemed fun to try. Some of the module moves may actually be needed for the diagnostic refactor anyway. I'm mostly hoping to see a significant performance gain on codspeed. Test Plan -- Existing tests
31a13d9 to
bc8389a
Compare
|
|
I think this is ready for review. I tried to regroup all of the imports I added to keep the I also changed I also moved the |
ntBre
added
diagnostics
ntBre
changed the title
Violation::RULE constantViolationMetadata::rule method
| pub use rules::pycodestyle::rules::IOError; | ||
|
|
||
| pub use diagnostic::Diagnostic; | ||
| pub(crate) use ruff_diagnostics::{Applicability, Edit, Fix, IsolationLevel, SourceMap}; |
There was a problem hiding this comment.
This is not strictly necessary, it just made rewriting some of the imports automatically a little easier.
There was a problem hiding this comment.
Do we need all the re-exports. E.g. SourceMap is probably less common
There was a problem hiding this comment.
Definitely not, I just got a bit tired/lazy updating imports and wanted to replace all ruff_diagnostics::* imports with crate::* in this crate. It should be easy enough to remove the less common ones like IsolationLevel and SourceMap, though.
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Nice!
The only thing that's unclear to me and I would like to get a short summary on what the next steps are is how you plan on removing rule (and AsRule) from Diagnostic because we won't be able to add it to ruff_db::Diagnostic. IMO, I think that's the next important refactor we shoul do
| } | ||
|
|
||
| impl Diagnostic { | ||
| #[expect(clippy::needless_pass_by_value)] |
There was a problem hiding this comment.
Do we allow needless_pass_by_value because passing by reference would require changing all call sites? It might be worth adding a note why we allow this.
There was a problem hiding this comment.
Yes that's right. I was a bit confused why it wasn't already needless and thought further refactoring (like combining this with Checker::report_diagnostic in most cases) might make it not needless again. I'll add a note!
| pub fix: Option<Fix>, | ||
| pub parent: Option<TextSize>, | ||
|
|
||
| pub(crate) rule: Rule, |
There was a problem hiding this comment.
Our plan is to remove this Diagnostic type with ruff_db::Diagnostic. What's your plan on how to remove rule from here because we can't add it to ruff_db::Diagnostic.
| pub use rules::pycodestyle::rules::IOError; | ||
|
|
||
| pub use diagnostic::Diagnostic; | ||
| pub(crate) use ruff_diagnostics::{Applicability, Edit, Fix, IsolationLevel, SourceMap}; |
There was a problem hiding this comment.
Do we need all the re-exports. E.g. SourceMap is probably less common
| match self { #rule_fixable_match_arms } | ||
| } | ||
| } | ||
|
|
||
| impl AsRule for ruff_diagnostics::Diagnostic { | ||
| impl AsRule for crate::Diagnostic { |
There was a problem hiding this comment.
I think one of the next important refactors is to remove rule (and AsRule) from Diagnostic because that won't work long-term
My plan was to get rid of this old In short, I think most of the need for this field goes away when we combine the diagnostic types, and we should be able to work around the rest if needed. Does that sound right to you? |
Do you have a link to this check. I think that should be fine as long as you have a
It does sound right in that this is what I would hope to be possible. I don't have a clear enough picture to say whether it will be possible. |
(and one unrelated import order refactor)
Oops, I meant ruff/crates/ruff_linter/src/checkers/ast/mod.rs Lines 454 to 457 in 8d5655a I think we can work around the uses that don't pass a
Sounds good. It's not entirely clear to me either, but it feels like it should work! |
|
Why would we have to change |
|
Yes, there are call sites where |
|
Did I really break the mypy primer? I didn't think I touched any ty files, but everything should be up to date and it looks okay on main. I also tried rerunning once. It looks like this is the root cause: |
seems unlikely. |
|
I think it's possible that a recent commit on arviz (arviz-devs/arviz@3205b82? it landed a few hours ago) means that ty now crashes on that project when we didn't previously. There's the same crash being reported on #18333 |
|
if so we should just move the project from |
|
Confirmed locally that ty @ Ruff's |
|
filed #18336 to fix |
Which has now landed -- the primer panic should go away if you rebase! |
|
Amazing, thank you @AlexWaygood! |
|
Hmm, I got a new cascade of errors on this one. I'll try rerunning once to see if that helps. |
I avoided this initially, but it will be cheap enough once #18234 lands
* main: [ty] Support ephemeral uv virtual environments (#18335) Add a `ViolationMetadata::rule` method (#18234) Return `DiagnosticGuard` from `Checker::report_diagnostic` (#18232) [flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (#18337) [ty] Support cancellation and retry in the server (#18273) [ty] Synthetic function-like callables (#18242) [ty] Support publishing diagnostics in the server (#18309) Add Autofix for ISC003 (#18256) [`pyupgrade`]: new rule UP050 (`useless-class-metaclass-type`) (#18334) [pycodestyle] Make `E712` suggestion not assume a context (#18328)
* main: (246 commits) [ty] Simplify signature types, use them in `CallableType` (astral-sh#18344) [ty] Support ephemeral uv virtual environments (astral-sh#18335) Add a `ViolationMetadata::rule` method (astral-sh#18234) Return `DiagnosticGuard` from `Checker::report_diagnostic` (astral-sh#18232) [flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (astral-sh#18337) [ty] Support cancellation and retry in the server (astral-sh#18273) [ty] Synthetic function-like callables (astral-sh#18242) [ty] Support publishing diagnostics in the server (astral-sh#18309) Add Autofix for ISC003 (astral-sh#18256) [`pyupgrade`]: new rule UP050 (`useless-class-metaclass-type`) (astral-sh#18334) [pycodestyle] Make `E712` suggestion not assume a context (astral-sh#18328) put similar dunder-call tests next to each other (astral-sh#18343) [ty] Derive `PartialOrd, Ord` for `KnownInstanceType` (astral-sh#18340) [ty] Simplify `Type::try_bool()` (astral-sh#18342) [ty] Simplify `Type::normalized` slightly (astral-sh#18339) [ty] Move arviz off the list of selected primer projects (astral-sh#18336) [ty] Add --config-file CLI arg (astral-sh#18083) [ty] Tell the user why we inferred a certain Python version when reporting version-specific syntax errors (astral-sh#18295) [ty] Implement implicit inheritance from `Generic[]` for PEP-695 generic classes (astral-sh#18283) [ty] Add hint if async context manager is used in non-async with statement (astral-sh#18299) ...
3 participants
Summary
This PR adds a macro-generated method to retrieve the
Ruleassociated with a givenViolationstruct, which makes it substantially cheaper than parsing from the rule name. The rule is then converted to aNoqaCodefor storage on theMessage(and eventually on the new diagnostic type). TheViolationMetadata::rule_namemethod was now unused, so therulemethod replaces it.Several types had to be moved from the
ruff_diagnosticscrate to theruff_lintercrate to make this work, namely theViolationtraits and the oldDiagnostictype, which had a constructor generic over aViolation.It's actually a fairly small PR, minus the hundreds of import changes. The main changes are in these files:
Test Plan
Existing tests