[red-knot] Add infrastructure to declare lints#14873
Merged
MichaReiser merged 3 commits intomainfrom Dec 10, 2024
Merged
Conversation
270ef11 to
c54c4d0
Compare
Contributor
|
carljm
approved these changes
Dec 10, 2024
sharkdp
reviewed
Dec 10, 2024
5150616 to
b012b24
Compare
c54c4d0 to
3fbb2a3
Compare
c469939 to
68952cd
Compare
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
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 is the second PR out of three that adds support for enabling/disabling lint rules in Red Knot. You may want to take a look at the first PR in this stack to familiarize yourself with the used terminology.
This PR adds a new syntax to define a lint:
A lint has a name and metadata about its status (preview, stable, removed, deprecated), the default diagnostic level (unless the configuration changes), and documentation. I use a macro here to derive the kebab-case name and extract the documentation automatically.
This PR doesn't yet add any mechanism to discover all known lints. This will be added in the next and last PR in this stack.
Documentation
I documented some rules but then decided that it's probably not my best use of time if I document all of them now (it also means that I play catch-up with all of you forever). That's why I left some rules undocumented (marked with TODO)
Where is the best place to define all lints?
I'm not sure. I think what I have in this PR is fine but I also don't love it because most lints are in a single place but not all of them. If you have ideas, let me know.
Why is the message not part of the lint, unlike Ruff's
ViolationI understand that the main motivation for defining
messageonViolationin Ruff is to remove the need to repeat the same message over and over again. I'm not sure if this is an actual problem. Most rules only emit a diagnostic in a single place and they commonly use different messages if they emit diagnostics in different code paths, requiring extra fields on theViolationstruct.That's why I'm not convinced that there's an actual need for it and there are alternatives that can reduce the repetition when creating a diagnostic:
add_xymethodsDiagnosticimplementation that tailors the entire diagnostic and pre-codes e.g. the messageAvoiding an extra field on the
Violationalso removes the need to allocate intermediate strings as it is commonly the place in Ruff. Instead, Red Knot can use a borrowed string withformat_argsTest Plan
cargo test