Skip to content

Add help: subdiagnostics for several Ruff rules that can sometimes appear to disagree with ty#22331

Merged
AlexWaygood merged 1 commit intomainfrom
alex/override-diags
Jan 2, 2026
Merged

Add help: subdiagnostics for several Ruff rules that can sometimes appear to disagree with ty#22331
AlexWaygood merged 1 commit intomainfrom
alex/override-diags

Conversation

@AlexWaygood
Copy link
Member

Summary

Ruff has a bunch of rules that can sometimes appear to disagree with ty. For example, this snippet of code will trigger an FBT001 violation:

# third-party file somewhere...

class Foo:
    def f(self, x: bool): ...

# first-party code...

class Bar(Foo):
    def f(self, x: bool): ...

But the author of this code can't change the definition of Foo.f (because it's third-party code), and they can't change Bar.f or ty will complain about a violation of the Liskov Substitution Principle.

We've already added documentation notes to a bunch of these rules in #21644 that tell people to use typing.override in this situation. Ruff will generally ignore this kind of rule if it sees that the method is decorated with @override, because it knows that a user can't easily change an @override-decorated rule without violating the Liskov Substitution Principle. However, we've still received some reports from users who are understandably confused about two Astral tools seeming to give contradictory orders about how the user should rewrite their code.

This PR adds help: subdiagnostics to several Ruff rules. The subdiagnostics also point the user towards typing.override; having the hint directly presented in the terminal should be more approachable and harder to miss.

Test Plan

Snapshots updated.

@AlexWaygood AlexWaygood added the diagnostics Related to reporting of diagnostics. label Jan 1, 2026
@astral-sh-bot
Copy link

astral-sh-bot bot commented Jan 1, 2026

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood
Copy link
Member Author

(There are a couple more rules that can cause issues with a type checker's Liskov enforcement. I basically just added help: subdiagnostics to the ones where it was easy to do so.)

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks!

Comment on lines +141 to +142
"Consider adding `@typing.override` if this method \
overrides a method from a superclass",
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of prefer this phrasing over the LSP version, but I don't feel strongly. I'm guessing we can't call the helper function here because the current scope isn't set?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm guessing we can't call the helper function here because the current scope isn't set?

That wasn't really my rationale for doing something slightly different here. The reasons why I did something slightly different here is that this violation is telling you to rename the function rather than change its signature, and renaming the function isn't going to cause a Liskov violation. It's just not necessarily what you want to do if the function is deliberately overriding something from a subclass 😄

},
function_def.identifier(),
);
if is_subject_to_liskov_substitution_principle(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the helper here? Oh, I see it's an FBT helper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right — I could put the helper somewhere more central, but wasn't totally sure where a good place would be :/

@AlexWaygood AlexWaygood merged commit d0f841b into main Jan 2, 2026
40 checks passed
@AlexWaygood AlexWaygood deleted the alex/override-diags branch January 2, 2026 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants