Detect match statement intended to be tail expression #81458
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I'm not entirely sure this strikes the right balance between user friendliness and code amount :-/ |
This comment has been minimized.
This comment has been minimized.
|
It would be great if someone else could review this, I'm leaving for vacation in a couple of days and won't have enough time. |
|
No problem, enjoy the time off! r? @oli-obk would you have the bandwidth to check this one out? It doesn't need to be approved, just gauging interest/opinion. |
| note: you might have meant to return the `match` expression | ||
| --> $DIR/match-with-different-arm-types-as-stmt-instead-of-expr.rs:27:6 | ||
| | | ||
| LL | fn wrong(c: &str) -> Box<dyn Foo> { | ||
| | ------------ the `match` arms can conform to this return type | ||
| LL | / match c { | ||
| LL | | "baz" => Box::new(Baz), | ||
| LL | | _ => Box::new(Bar), | ||
| LL | | }; | ||
| | | -^ the `match` is a statement because of this semicolon, consider removing it | ||
| | |_____| | ||
| | this could be implicitly returned but it is a statement, not a tail expression |
There was a problem hiding this comment.
This is the new part.
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
match-stmt-remove-semi
branch
from
80249cc
to
5921c49
Compare
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
match-stmt-remove-semi
branch
2 times, most recently
from
fd2a565
to
5afc09f
Compare
estebank
force-pushed
the
match-stmt-remove-semi
branch
from
5afc09f
to
1d24f07
Compare
|
@bors r=oli-obk |
|
📌 Commit 1d24f07 has been approved by |
bors
added
S-waiting-on-bors
…oli-obk Detect match statement intended to be tail expression CC rust-lang#24157
|
☀️ Test successful - checks-actions |
|
It seems like this was a regression, which seems a little surprising, as the PR appears to try to affect only an error path - maybe storing the additional span is causing some overhead though. @estebank @oli-obk any thoughts on whether there's some optimization that can be done here? If not the regression seems not horrible - primarily affecting stress tests - but it is rather large on those. |
| Some(ret_coercion) if self.in_tail_expr => { | ||
| let ret_ty = ret_coercion.borrow().expected_ty(); | ||
| let ret_ty = self.inh.infcx.shallow_resolve(ret_ty); | ||
| self.can_coerce(arm_ty, ret_ty) | ||
| && prior_arm_ty.map_or(true, |t| self.can_coerce(t, ret_ty)) | ||
| // The match arms need to unify for the case of `impl Trait`. | ||
| && !matches!(ret_ty.kind(), ty::Opaque(..)) | ||
| } |
There was a problem hiding this comment.
Doing this on every tail expression could potentially be the cause of the regression
There was a problem hiding this comment.
Opened #82738 to check this.
| let expectation = match expr.kind { | ||
| hir::ExprKind::Match(..) if is_last => IsLast(stmt.span), | ||
| _ => NoExpectation, | ||
| }; |
There was a problem hiding this comment.
If the other piece of code isn't at fault, let's try reverting to the logic you had before (bubbling the information down in the function arguments
Move the check for potentially forgotten `return` in a tail expression of arbitrary expressions into the coercion error branch to avoid computing unncessary coercion checks on successful code. Follow up to rust-lang#81458.
… r=oli-obk Move check only relevant in error case out of critical path Move the check for potentially forgotten `return` in a tail expression of arbitrary expressions into the coercion error branch to avoid computing unncessary coercion checks on successful code. Follow up to rust-lang#81458.
CC #24157