fix: Fix panics on GATs involving const generics #13021
Conversation
crates/hir-ty/src/lower.rs
Outdated
| let idx = generics.param_idx(param_id.into()).expect("matching generics"); | ||
| let idx = match generics.param_idx(param_id.into()) { | ||
| // FIXME: never crash if this involves const generic associated types | ||
| None if generics.is_gat => { |
There was a problem hiding this comment.
let's just do a never!() check here instead of checking is_gat
There was a problem hiding this comment.
This will make the test fail though, is that okay?
There was a problem hiding this comment.
Yeah, I would be fine with that. It's a bit unfortunate that we can't test that it doesn't crash, but I'd rather have the crashing test as a reminder that it's still broken.
There was a problem hiding this comment.
So to prevent the test from failing I used tracing::error! and added a FIXME explaining why it doesn't use never!.
Oops, didn't see your comment in time.
I guess running cargo test --release -- -q would make the test succeed.
This workaround avoids constant crashing of rust analyzer when using GATs with const generics, even when the const generics are only on the `impl` block. The workaround treats GATs as non-existing if either itself or the parent has const generics and removes relevant panicking code-paths.
|
LGTM, but the test of course fails 😅 I'd mark it as ignored or should_panic, but we lint against both of those 😒 @bors delegate+ |
|
✌️ @N3xed can now approve this pull request |
|
Hopefully, the CI failing isn't a big issue. Unfortunately, I'm not familiar enough to tackle the underlying problem since I'm very new to the rust analyzer code base, and just implementing this workaround took me quite a while. Nonetheless thanks for the review. |
fix: Fix panics on GATs involving const generics This workaround avoids constant crashing of rust analyzer when using GATs with const generics, even when the const generics are only on the `impl` block. The workaround treats GATs as non-existing if either itself or the parent has const generics and removes relevant panicking code-paths. ~~Additionally, I've added the `is_gat` field to `utils::Generics` that determines whether missing generics params crashes rust analyzer. Another solution would have been to remove all panics from the relevant code path regardless of whether the generics are on a GAT, but this could change behavior outside of GATs.~~ Fixes #11989, fixes #12193
|
💔 Test failed - checks-actions |
| // in debug mode, we catch the unwind and expect that it panicked. See the | ||
| // [`crate::utils::generics`] function for more information. | ||
| cov_mark::check!(ignore_gats); | ||
| std::panic::catch_unwind(|| { |
There was a problem hiding this comment.
@flodiebold So, to make the test succeed I just catch the unwind here right now, which is pretty much the same as #[shoud_panic]. Or can we merge without passing tests?
|
TBH I think we should probably just get rid of the should_panic lint. Having tests for cases that currently panic can be genuinely useful, like here. But for now let's merge this. @bors r+ |
|
☀️ Test successful - checks-actions |
This workaround avoids constant crashing of rust analyzer when using GATs with const generics,
even when the const generics are only on the
implblock.The workaround treats GATs as non-existing if either itself or the parent has const generics and
removes relevant panicking code-paths.
Fixes #11989, fixes #12193