[ty] Refactor CheckSuppressionContext to use DiagnosticGuard#21587
[ty] Refactor CheckSuppressionContext to use DiagnosticGuard#21587MichaReiser merged 4 commits intomainfrom
CheckSuppressionContext to use DiagnosticGuard#21587Conversation
MichaReiser
added
internal
| buffer | ||
| } | ||
|
|
||
| /// An abstraction for mutating a diagnostic. |
There was a problem hiding this comment.
This is just a move except that DiagnosticGuard now stores a file and sink instead of a &InferContext
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
MichaReiser
force-pushed
the
micha/suppressions-diagnostic-guard
branch
from
95230f4 to
75c3782
Compare
| if let DiagnosticId::Lint(lint_name) = diag.id() { | ||
| diag.set_documentation_url(Some(lint_documentation_url(lint_name))); | ||
| } |
There was a problem hiding this comment.
I moved this logic into DiagnosticGuard::drop, to reduce the code duplication
MichaReiser
force-pushed
the
micha/suppressions-diagnostic-guard
branch
from
75c3782 to
acc9702
Compare
MichaReiser
force-pushed
the
micha/suppressions-diagnostic-guard
branch
from
acc9702 to
b97e848
Compare
MichaReiser
changed the title
CheckSuppressionContext to use DiagnosticGuard"CheckSuppressionContext to use DiagnosticGuard
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
| pub(super) struct DiagnosticGuard<'sink> { | ||
| file: File, | ||
|
|
||
| sink: &'sink std::cell::RefCell<TypeCheckDiagnostics>, |
There was a problem hiding this comment.
What goes wrong when you use &mut TypeCheckDiagnostics here?
It might be worth a brief comment saying why interior mutability is being used here.
There was a problem hiding this comment.
It would panic if you do:
let diag = context.report_lint(...);
let diag2 = context.report_lint(...); // we now borrow `&mut TypeCheckDiagnostics` twice
This seems worse as an API caller shouldn't be aware of the internal ref-cell borrow
There was a problem hiding this comment.
I guess I meant the higher level reason. But I think I now see: you wouldn't be able to have two guards alive at the same time?
There was a problem hiding this comment.
Yes, the higher level reason is that the current approach correctly isolates the RefCell so that callers don't need to know about it (I'll add a comment).
MichaReiser
force-pushed
the
micha/suppressions-diagnostic-guard
branch
from
b97e848 to
a67e6f2
Compare
* main: [ty] Implement `typing.override` (astral-sh#21627) [ty] Avoid expression reinference for diagnostics (astral-sh#21267) [ty] Improve autocomplete suppressions of keywords in variable bindings [ty] Only suggest completions based on text before the cursor Implement goto-definition and find-references for global/nonlocal statements (astral-sh#21616) [ty] Inlay Hint edit follow up (astral-sh#21621) [ty] Implement lsp support for string annotations (astral-sh#21577) [ty] Add 'remove unused ignore comment' code action (astral-sh#21582) [ty] Refactor `CheckSuppressionContext` to use `DiagnosticGuard` (astral-sh#21587) [ty] Improve several "Did you mean?" suggestions (astral-sh#21597) [ty] Add more and update existing projects in `ty_benchmark` (astral-sh#21536) [ty] fix ty playground initialization and vite optimization issues (astral-sh#21471)
2 participants
Summary
This is PR is a first step towards adding a "Remove unused suppression" code action in the editor.
Adding the fix for removing unused suppression requires setting the
Fixon thediagnostic created by
CheckSuppressionContext::report_unchecked. To make this possible,change
report_uncheckedandreport_lintto return aDiagnosticGuard.Doing so requires extracting
DiagnosticGuardfromInferContext.I tried very hard to not having to change
CheckSuppressionContextto store aRefCell<TypeCheckDiagnostic>butI couldn't find a way to do so. I tried a
enum DiagnosticSink { Cell(RefCell<TypeCheckDiagnostics>), Mut(&mut TypeCheckDiagnostic) }with a
emitmethod that takes&self. However, the borrow checker (correctly?), disallowes me to mutate the&mut TypeCheckDiagnosticin that case.I gave up after 30min or so and I sort of like how
check_suppressionstakes theDiagnostics with suppressions and returns onlythe diagnostics once it's done, as the suppressions have now been handled.
Test Plan
cargo test