feat: add non-exhaustive-let diagnostic #16303
Conversation
rustbot
added
the
S-waiting-on-review
| 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
rosefromthedead
force-pushed
the
non-exhaustive-let
branch
from
29e3c0a
to
cb8733f
Compare
|
☔ The latest upstream changes (presumably #16420) made this pull request unmergeable. Please resolve the merge conflicts. |
rosefromthedead
force-pushed
the
non-exhaustive-let
branch
from
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. |
rosefromthedead
force-pushed
the
non-exhaustive-let
branch
2 times, most recently
from
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. |
Veykril
force-pushed
the
non-exhaustive-let
branch
from
f6cf381
to
5390e4c
Compare
|
Rebased it for you, thanks! |
|
☀️ Test successful - checks-actions |
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.