feat: add non-exhaustive-let diagnostic#16303
Conversation
rustbot
added
the
S-waiting-on-review
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
Nice and simple diagnostic, thanks! Just some small things to fix. Regarding quickfix we should just wait until we switch to rustc's exhaustiveness checker which should help in that regard.
| fn validate_block(&mut self, db: &dyn HirDatabase, expr: &Expr) { | ||
| let Expr::Block { statements, .. } = expr else { return }; | ||
| let body = db.body(self.owner); | ||
| let pattern_arena = Arena::new(); |
There was a problem hiding this comment.
Likewise, lets make this a field so we can re-use the arena from previous iterations (together with the one for general match checking)
There was a problem hiding this comment.
Is it beneficial to reuse an arena when there are no references to the old items? I thought it might be similar logic to reusing a Vec after .clear()ing it, but there is no equivalent method on arenas.
(I'm also having lifetime issues trying to move the arena inside ExprValidator :P)
There was a problem hiding this comment.
Clearing isn't too beneficial for an arena, as that will just drop the allocated chunks which includes capacity, unlike a vec where capacity remains. The lifetime issues I can see, given how the arena is used. So without some lifetime transmuting, not much we can do about so let's keep it this way.
There was a problem hiding this comment.
I think it drops all but the last chunk, which means there is still some good amortizing of capacity compared to making a new one. Also lifetimes should be cleaner with the new code, it might work now.
There was a problem hiding this comment.
Actually it would make sense to intern the lowered patterns if many parts of r-a start to use them
There was a problem hiding this comment.
I'm actually trying to remove the arena entirely, if this turns out ok perf-wise (allocating slices on an arena is actually not great perf-wise because TypedArena allocates into a SmallVec first anyway)
There was a problem hiding this comment.
Yeah, don't worry about reusing the arena, it's going away soon
Veykril
added
S-waiting-on-author
29e3c0a to
cb8733f
Compare
|
☔ The latest upstream changes (presumably #16420) made this pull request unmergeable. Please resolve the merge conflicts. |
cb8733f to
a8da107
Compare
|
Re: the panic I had when I posted this, I never had a solid reproducer, so I can only hope that the pattern analysis update solved that. I'd be happy to mark this diagnostic as experimental if we want to avoid shipping that panic. |
a7f16e6 to
f6cf381
Compare
|
Yeah don't worry about panics in match analysis, they should all be replaced with an |
|
(and by soon I mean #16533) (sorry for breaking your PR again) |
|
☔ The latest upstream changes (presumably #16533) made this pull request unmergeable. Please resolve the merge conflicts. |
f6cf381 to
5390e4c
Compare
|
Rebased it for you, thanks! |
|
☀️ Test successful - checks-actions |
5 participants
I want this to have a quickfix to add an else branch but I couldn't figure out how to do that, so here's the diagnostic on its own. It pretends a
letis a match with one arm, and asks the match checking whether that match would be exhaustive.Previously the pattern was checked based on its own type, but that was causing a panic in
match_check(while processing e.g.crates/hir/src/lib.rs) so I changed it to use the initialiser's type instead, to align with the checking of actual match expressions. I think the panic can still happen, but I hear thatmatch_checkis going to be updated to a new version from rustc, so I'm posting this now in the hopes that the panic will magically go away when that happens.