Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 85.29%. The percentage of expected errors that received a diagnostic held steady at 78.70%. The number of fully passing files held steady at 64/133. |
Memory usage reportMemory usage unchanged ✅ |
|
|
MichaReiser
requested review from
Gankra,
carljm,
dcreager and
sharkdp
as code owners
crates/ty_project/src/lib.rs
Outdated
| Ok(result) => Ok(result), | ||
| Err(error) => { | ||
| match error.payload.downcast_ref::<salsa::Cancelled>() { | ||
| Some(salsa::Cancelled::PropagatedPanic) | None => { |
There was a problem hiding this comment.
Is it possible for PropagatedPanic to happen just because another thread was cancelled normally, while our thread was blocked on it? I.e. could this change mean that a normal file-written revision bump during parallel check ends up causing a panicked diagnostic in a thread which was blocked on another thread when the change occurred?
I'm not totally sure but looking at where Salsa throws PropagatedPanic it seems like this might be a problem?
There was a problem hiding this comment.
Nice catch! I'll add a db.unwind_if_cancelled check above to ensure we don't take this path if there's a pending write.
* main: [ty] Avoid eager TypedDict diagnostics in `TypedDict | dict` unions (#24151) `F507`: Fix false negative for non-tuple RHS in `%`-formatting (#24142) [ty] Update `SpecializationBuilder` hook to get both lower/upper bounds (#23848) Fix `%foo?` parsing in IPython assignment expressions (#24152) `E501`/`W505`/formatter: Exclude nested pragma comments from line width calculation (#24071) [ty] Fix Salsa panic propagation (#24141) [ty] Support `type:ignore[ty:code]` suppressions (#24096) [ty] Support narrowing for extended walrus targets (#24129)
3 participants
Summary
This PR fixes probably the most likely case why users saw astral-sh/ty#1565 in their IDE.
I added handling to convert panics to diagnostics in #17631 to
check_file_impl. However, this was beforecheck_file_implbecame a Salsa query. We have to be a bit more careful, now thatcheck_file_implis a Salsa query.untracked_readensures that thecheck_file_implreruns after every change. This is more often than necessary, but it is the best we can do here without knowing the exact dependencies that were collected up to whencheck_file_impl's dependency panicked.check_files_implis unlikely to be what we want because it means the function still runs to completion even when the query was cancelled. Instead, we want to propagate a cancellation so that itsdbhandle gets released as quickly as possible to unblock any pending mutation. However, we do need some special handling for Salsa's propagating panic to avoid regressing [red-knot] Fix CLI hang when a dependent query panics #17631. For a queryArunning on threadathat depends on queryBrunning on threadb. IfBpanics, Salsa throws the original panic on threadbbut throws aCancelled::PropagatingPanicpanic on threada. Threadb's panic is the more useful one because it contains the actual panic information. We already convertb's panic to aDiagnostic, but we silently ignore anyCancelled::PropagatingPanic. This PR also creates a propagating panic to a diagnostic. While these panics don't contain any useful information, they at least indicate to a user thatAwas only partially checked. They should have a second diagnostic forBthat contains the full panic information (unlessBis a query that didn't run as part ofproject.check, e.g.hover).Test plan
I used @AlexWaygood's panda-stubs reproducer and I was no longer able to reproduce the Salsa panic.
I plan on doing some CLI
--watchtesting tomorrow morning but this should block code review.