Inline DiagnosticKind into other diagnostic types#18074
Merged
Conversation
instead of storing the DiagnosticKind to construct a title later
avoid DiagnosticKind in blocking_process_invocation avoid DiagnosticKind in suspicious_function_call avoid DiagnosticKind in function_call_in_argument_default avoid DiagnosticKind in typing_only_runtime_import avoid DiagnosticKind in unused_arguments avoid DiagnosticKind in replaceable_by_pathlib avoid DiagnosticKind in pandas_vet rules avoid DiagnosticKind in invalid_first_argument_name avoid DiagnosticKind in indentation avoid DiagnosticKind in missing_whitespace_around_operator avoid DiagnosticKind in invalid_string_characters avoid DiagnosticKind in no_method_decoroator avoid DiagnosticKind in use_pep604_annotation avoid DiagnosticKind in ambiguous_unicode_character avoid DiagnosticKind in some tests
Contributor
|
ntBre
added a commit
that referenced
this pull request
May 13, 2025
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
approved these changes
May 14, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This looks good. I only skimmed through and I've one comment related to code size/compile time.
Comment on lines
469
to
476
| impl AsRule for ruff_diagnostics::Diagnostic { | ||
| fn rule(&self) -> Rule { | ||
| match self.name.as_str() { | ||
| #from_impls_for_diagnostic_kind | ||
| _ => unreachable!("invalid rule name: {}", self.name), | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
By how much does this increase the code size, considering that this is a pretty large match?
Should we instead implement TryFrom<&str> for Rule and the AsRule impl for Diagnostic and DiagnosticMessagte both call Rule::try_from(&self.name).or_else(|| panic!("invalid rule name: {}", self.name))
Contributor
Author
There was a problem hiding this comment.
Oh good question. There's even a strum macro for generating a FromStr (and TryFrom<&str>) implementation that I used in one of my drafts, so we can definitely switch to that if you prefer. I guess there's no reason not to do that, unless we're worried about people calling Rule::from_str directly.
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`. In the first commit, I added a generated `Rule::pascal_name` method for converting back to the Pascal-case name currently used by `Diagnostic` and `DiagnosticMessage`, but in the second commit I used the `heck` crate to generalize our existing `kebab_case` macro to convert the `ViolationMetadata::rule_name` output to kebab-case and use kebab-case everywhere. `heck` was already in our dependency graph because it's what `strum` uses 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 `suggestion` is unavailable. Otherwise `Diagnostic::name` is only used for `debug!` logging. Test Plan -- Existing tests
Glyphack
pushed a commit
to Glyphack/ruff
that referenced
this pull request
May 21, 2025
## Summary This PR deletes the `DiagnosticKind` type by inlining its three fields (`name`, `body`, and `suggestion`) into three other diagnostic types: `Diagnostic`, `DiagnosticMessage`, and `CacheMessage`. Instead of deferring to an internal `DiagnosticKind`, both `Diagnostic` and `DiagnosticMessage` now have their own macro-generated `AsRule` implementations. This should make both astral-sh#18051 and another follow-up PR changing the type of `name` on `CacheMessage` easier since its type will be able to change separately from `Diagnostic` and `DiagnosticMessage`. ## Test Plan Existing tests
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR deletes the
DiagnosticKindtype by inlining its three fields (name,body, andsuggestion) into three other diagnostic types:Diagnostic,DiagnosticMessage, andCacheMessage.Instead of deferring to an internal
DiagnosticKind, bothDiagnosticandDiagnosticMessagenow have their own macro-generatedAsRuleimplementations.This should make both #18051 and another follow-up PR changing the type of
nameonCacheMessageeasier since its type will be able to change separately fromDiagnosticandDiagnosticMessage.Test Plan
Existing tests