[ty] Don't introduce invalid syntax when autofixing override-of-final-method#21699
[ty] Don't introduce invalid syntax when autofixing override-of-final-method#21699AlexWaygood merged 2 commits intomainfrom
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| .contains(overload.node(db, context.file(), context.module())) | ||
| }); | ||
|
|
||
| let applicability = if should_fix { |
There was a problem hiding this comment.
I'm leaning towards not showing the fix in that case because, I think, the is_only detection uses the wrong parent in that case and the more correct fix is to delete the entire outer statement, or the statement itself.
DisplayOnlyFixes are also never shown anywhere. They're just dead code
There was a problem hiding this comment.
oh, I assumed making it display-only was what you were asking for in #21646 (comment) (we don't have an Applicability::Manual :-)
But sure, I'll just get rid of the fix in these cases.
DisplayOnlyFixes are also never shown anywhere. They're just dead code
What's the purpose of Applicability::DisplayOnly if they aren't shown anywhere? I figured that if/when we show fixes in the CLI rendering of diagnostics, the diff would be helpful for users in telling them what we were asking for (you mentioned in #21646 that you found it confusing that the diagnostic was only omitted on one overload, but that to fix the diagnostic you actually had to remove all overloads).
There was a problem hiding this comment.
The idea was to only show them in the CLI but we never made it that far. There were just too many design decisions.
9f73442 to
d99cfc5
Compare
Summary
This didn't actually turn out to be too hard afterall.
StmtClassDefnode as their immediate parent in the AST (e.g. one or more overloads is inside aStmtIfnode), mark the fix as display-onlypassrather than just deleting the range of the overriding definitionFixes astral-sh/ty#1678
Test Plan
Updated snapshtos