Create Unknown rule diagnostics with a source range by MichaReiser · Pull Request #15648 · astral-sh/ruff

MichaReiser · 2025-01-21T15:59:44Z

Summary

Track the source range of values deserialized from the TOML configuration and use the source range to create unknown-rule diagnostics that point to the right location in the configuration file.

────────────┬───────────────────────────────────────────────────────────────────
    0     0 │ success: false
    1     1 │ exit_code: 1
    2     2 │ ----- stdout -----
    3       │-warning[unknown-rule] Unknown lint rule `division-by-zer`
          3 │+warning[unknown-rule] <temp_dir>/pyproject.toml:3:1 Unknown lint rule `division-by-zer`
    4     4 │ 
    5     5 │ ----- stderr -----

This PR makes use of toml's spanned functionality to get a text range for every deserialized value in combination with the ValueSource that I introduced in previ.

Toml's spanned feature has one significant drawback: It doesn't support flatten or untagged, at least when using the derived deserializer. The toml-span crate explains why in more detail.

I considered using toml-span instead, but it requires you to write all Deserializers manually. I ultimately decided to go with toml's span because:

We can always migrate to toml-span if we have to use flatten or similar (either for the entire configuration or only the problematic values)
Writing deserializer's manually brings the risk that the JSON schema and Deserializer diverge
We want to avoid flatten as much as possible because it leads to very unhelpful error messages when the configuration can't be deserialized because of an unknown field or invalid type.
It's simply easier :)

I do expect that using this approach will require some hackery to support e.g. a string | vec<string> value but this has been solved by cargo-vet.

This PR is inspired by mozilla/cargo-vet#230

Part of astral-sh/ty#219

Test Plan

See the changes in the CLI test

github-actions · 2025-01-21T16:09:39Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser · 2025-01-22T16:36:51Z

crates/red_knot_project/src/metadata.rs

        let name = project
            .and_then(|project| project.name.as_ref())
-            .map(|name| Name::new(&**name))
+            .map(|name| Name::new(&***name))


How long before it becomes &********* 😆

all I see is &hunter2

MichaReiser · 2025-01-22T16:38:54Z

crates/red_knot_python_semantic/src/lint.rs

    ///
    /// If a rule isn't present in this map, then it should be considered disabled.
-    lints: FxHashMap<LintId, Severity>,
+    lints: FxHashMap<LintId, (Severity, LintSource)>,


This is not strictly necessary for what we need today but the intent here is that we show a note in diagnostics similar to clippy, telling users why a check was reported (you enabled this rule in the configuration, you enabled it in the cli, it's enabled by default, etc)

MichaReiser · 2025-01-22T16:40:38Z

crates/red_knot_project/src/metadata/options.rs

 pub struct EnvironmentOptions {
    #[serde(skip_serializing_if = "Option::is_none")]
-    pub python_version: Option<PythonVersion>,
+    pub python_version: Option<RangedValue<PythonVersion>>,


For now, it would be sufficient to only make the key in the Rules table ranged because we don't use the range or source for any other value. I instead opted to make all values Ranged because the overhead should be small and having the range will make it easier to write good diagnostics.

carljm

Cool!

carljm · 2025-01-22T21:40:57Z

crates/red_knot_project/src/metadata.rs

        let name = project
            .and_then(|project| project.name.as_ref())
-            .map(|name| Name::new(&**name))
+            .map(|name| Name::new(&***name))


all I see is &hunter2

carljm · 2025-01-22T23:04:25Z

crates/red_knot_project/src/metadata/value.rs

+    /// The byte range of `value` in `source`.
+    ///
+    /// Can be `None` because not all sources support a range.
+    /// For example, arguments provided on the CLI won't have a range attached.
+    range: Option<TextRange>,


This allows representing the invalid states where a value with CLI source has Some range, or a value with file source has None range. What would be the tradeoffs of instead recording the range as part of the ValueSource? (I realize this would mean splitting the ValueSource type used here from the ValueSource type stored in thread-locals, but that doesn't necessarily seem like a problem).

I considered that but decided against it for two reasons:

It doesn't simplify downstream code because no code depends on the enforced constraint. However, enforcing it here does require more code because we'd need a ValueSourceKind (which is what we set in the thread local) and ValueSource.

We'll need to deserialize a serde::Value when supporting more complex types from the CLI using --config complex.key = "{ sub = [... ] }" and the toml crate can't emit text ranges in that case. I'm not sure if we have a similar case for configurations coming from a file, but maybe when working on the uv integration?

Overall, there are too many unknowns for cases where we might not have a range even if the value comes from a file and there are no "downstream" improvements to enforce the constraint which is why I'd prefer to keep it as is for now.

carljm · 2025-01-22T23:06:09Z

crates/red_knot_project/src/metadata/value.rs

-impl Combine for RelativePathBuf {
-    fn combine(self, _other: Self) -> Self {
-        self
+// The type already has a `into_iter` method thanks to `Deref`.


This comment doesn't make sense to me, because we are defining the into_iter method for the RangedValue type here. Did you mean to say that it already has an iter method?

* main: [red-knot] MDTests: Do not depend on precise public-symbol type inference (#15691) [red-knot] Make `infer.rs` unit tests independent of public symbol inference (#15690) Tidy knot CLI tests (#15685) [red-knot] Port comprehension tests to Markdown (#15688) Create Unknown rule diagnostics with a source range (#15648) [red-knot] Port 'deferred annotations' unit tests to Markdown (#15686) [red-knot] Support custom typeshed Markdown tests (#15683) Don't run the linter ecosystem check on PRs that only touch red-knot crates (#15687) Add `rules` table to configuration (#15645) [red-knot] Make `Diagnostic::file` optional (#15640) [red-knot] Add test for nested attribute access (#15684) [red-knot] Anchor relative paths in configurations (#15634) [`pyupgrade`] Handle multiple base classes for PEP 695 generics (`UP046`) (#15659) [`pyflakes`] Treat arguments passed to the `default=` parameter of `TypeVar` as type expressions (`F821`) (#15679) Upgrade zizmor to the latest version in CI (#15649) [`pyupgrade`] Add rules to use PEP 695 generics in classes and functions (`UP046`, `UP047`) (#15565) [red-knot] Ensure a gradual type can always be assigned to itself (#15675)

MichaReiser added the ty Multi-file analysis & type inference label Jan 21, 2025

MichaReiser force-pushed the micha/ranged-rule-diagnostics branch from e1d941a to 10b8841 Compare January 21, 2025 16:00

MichaReiser commented Jan 22, 2025

View reviewed changes

MichaReiser force-pushed the micha/ranged-rule-diagnostics branch from a6bf876 to 5fa96e4 Compare January 22, 2025 16:59

MichaReiser marked this pull request as ready for review January 22, 2025 16:59

MichaReiser requested review from AlexWaygood, carljm and sharkdp as code owners January 22, 2025 16:59

carljm approved these changes Jan 22, 2025

View reviewed changes

MichaReiser changed the title ~~WIP: Create Unknown rule diagnostics with a source range~~ Create Unknown rule diagnostics with a source range Jan 23, 2025

MichaReiser force-pushed the micha/rule-table branch from 0bf7453 to 3a4b7f0 Compare January 23, 2025 09:50

Base automatically changed from micha/rule-table to main January 23, 2025 09:57

MichaReiser added 2 commits January 23, 2025 10:58

Add RangedValue

fe6f470

Use RangedValue in Options

997bd03

MichaReiser force-pushed the micha/ranged-rule-diagnostics branch from 5fa96e4 to 997bd03 Compare January 23, 2025 10:00

Refine comment

deda0c2

MichaReiser merged commit 05ea77b into main Jan 23, 2025
21 checks passed

MichaReiser deleted the micha/ranged-rule-diagnostics branch January 23, 2025 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Create Unknown rule diagnostics with a source range#15648

Create Unknown rule diagnostics with a source range#15648
MichaReiser merged 3 commits intomainfrom
micha/ranged-rule-diagnostics

MichaReiser commented Jan 21, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jan 21, 2025 •

edited

Loading

Uh oh!

MichaReiser Jan 22, 2025 •

edited

Loading

Uh oh!

carljm Jan 22, 2025

Uh oh!

MichaReiser Jan 22, 2025

Uh oh!

MichaReiser Jan 22, 2025

Uh oh!

carljm left a comment

Uh oh!

carljm Jan 22, 2025

Uh oh!

carljm Jan 22, 2025

Uh oh!

MichaReiser Jan 23, 2025

Uh oh!

carljm Jan 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

MichaReiser commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

carljm Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

carljm Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MichaReiser commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser Jan 22, 2025 •

edited

Loading