Skip to content

[red-knot] Add infrastructure to declare lints#14873

Merged
MichaReiser merged 3 commits intomainfrom
micha/declare-lint
Dec 10, 2024
Merged

[red-knot] Add infrastructure to declare lints#14873
MichaReiser merged 3 commits intomainfrom
micha/declare-lint

Conversation

@MichaReiser
Copy link
Copy Markdown
Member

@MichaReiser MichaReiser commented Dec 9, 2024

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:

declare_lint! {
    /// ## What it does
    /// Checks for references to names that are not defined.
    ///
    /// ## Why is this bad?
    /// Using an undefined variable will raise a `NameError` at runtime.
    ///
    /// ## Example
    ///
    /// ```python
    /// print(x)  # NameError: name 'x' is not defined
    /// ```
    pub(crate) static UNRESOLVED_REFERENCE = {
        summary: "detects references to names that are not defined",
        status: LintStatus::preview("1.0.0"),
        default_level: Level::Warn,
    }
}

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 Violation

I understand that the main motivation for defining message on Violation in 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 the Violation struct.

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:

  • Create a helper function. We already do this in red knot with the add_xy methods
  • Create a custom Diagnostic implementation that tailors the entire diagnostic and pre-codes e.g. the message

Avoiding an extra field on the Violation also removes the need to allocate intermediate strings as it is commonly the place in Ruff. Instead, Red Knot can use a borrowed string with format_args

Test Plan

cargo test

@MichaReiser MichaReiser changed the base branch from main to micha/diagnostic-id December 9, 2024 14:09
@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Dec 9, 2024
@MichaReiser MichaReiser marked this pull request as ready for review December 9, 2024 14:31
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 9, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Copy Markdown
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Comment thread crates/red_knot_python_semantic/src/lint.rs Outdated
Comment thread crates/red_knot_python_semantic/src/lint.rs Outdated
Comment thread crates/red_knot_python_semantic/src/lint.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/diagnostic.rs Outdated
Comment thread crates/red_knot_python_semantic/src/lint.rs Outdated
Base automatically changed from micha/diagnostic-id to main December 10, 2024 15:58
@MichaReiser MichaReiser enabled auto-merge (squash) December 10, 2024 16:10
@MichaReiser MichaReiser merged commit 5fc8e5d into main Dec 10, 2024
@MichaReiser MichaReiser deleted the micha/declare-lint branch December 10, 2024 16:14
dcreager added a commit that referenced this pull request Dec 10, 2024
* main:
  [red-knot] Add infrastructure to declare lints (#14873)
  [red-knot] Typed diagnostic id (#14869)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants