-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use static diagnostic names and cache Rules
#18079
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
Use static diagnostic names and cache Rules
#18079
Conversation
Summary -- This PR is stacked on #18074 and uses the separation between `CacheMessage` and the other diagnostic types added there to store a `Rule` directly on `CacheMessage` and convert it back to a `&'static str` for use by `Diagnostic` and `DiagnosticMessage`. I think what we really want is to use only the kebab-case names everywhere, but I don't think we can accomplish that with `Diagnostic::new` living in the `ruff_diagnostics` crate, and the proc macros being called in `ruff_linter`. `Diagnostic::new` relies on `ViolationMetadata::rule_name`, which just calls `stringify!(#name)`. Test Plan -- Existing tests
|
MichaReiser
left a comment
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.
Nice speed up.
This is great. I would probably remove the heck dependency again, given that we already had an implementation of kebab_case (also reduces the size of your PR).
My only concern is that name is also used in the GitLab reporter to compute a fingerprint
ruff/crates/ruff_linter/src/message/gitlab.rs
Lines 126 to 134 in 981bd70
| fn fingerprint(message: &Message, project_path: &str, salt: u64) -> u64 { | |
| let mut hasher = DefaultHasher::new(); | |
| salt.hash(&mut hasher); | |
| message.name().hash(&mut hasher); | |
| project_path.hash(&mut hasher); | |
| hasher.finish() | |
| } |
This means, all diagnostics will be changed for gitlab users.
I don't think I'm too concerned by this because the implementation already uses std::hash::DefaultHasher which isn't guaranteed to be stable across rustc versions (or even platforms! we should probably fix this).
|
Oh interesting about the gitlab diagnostic. As soon as I saw it was being hashed, I assumed it was okay. From the gitlab docs, it sounds like they shouldn't be too user-facing and only used to deduplicate entries in the merge request widget, so hopefully it's okay. |
|
Hm, it does look like we marked a previous change to the hash as breaking: #7159. One other benefit of keeping |
This PR is stacked on #18074 and uses the
separation between
CacheMessageand the other diagnostic types added there tostore a
Ruledirectly onCacheMessageand convert it back to a&'static strfor use byDiagnosticandDiagnosticMessage.In the first commit, I added a generated
Rule::pascal_namemethod for convertingback to the Pascal-case name currently used by
DiagnosticandDiagnosticMessage,but in the second commit I used the
heckcrate to generalize our existingkebab_casemacro to convert the
ViolationMetadata::rule_nameoutput to kebab-case and usekebab-case everywhere.
heckwas already in our dependency graph because it's whatstrumuses internally for case conversions.I think that's probably preferable long-term, but it might be a breaking change in
the LSP because the lint name is used as the title for a quick fix, if a
suggestionisunavailable. Otherwise
Diagnostic::nameis only used fordebug!logging.Test Plan
Existing tests