Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

? desugaring leaks into error messages when Ok is forgotten on final expr #51632

Closed
nikomatsakis opened this issue Jun 19, 2018 · 1 comment
Closed

? desugaring leaks into error messages when Ok is forgotten on final expr #51632

nikomatsakis opened this issue Jun 19, 2018 · 1 comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Given this example:

fn foo() -> Result<(), ()> {
   bar()? 
}

fn bar() -> Result<(), ()> {
}

fn main() { }

I get this error (playground):

error[E0308]: match arms have incompatible types
 --> src/main.rs:2:4
  |
2 |    bar()? 
  |    ^^^^^^
  |    |
  |    expected enum `std::result::Result`, found ()
  |    match arm with an incompatible type
  |
  = note: expected type `std::result::Result<(), ()>`
             found type `()`

It seems like it should not be talking about match =)
@nikomatsakis nikomatsakis added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 19, 2018
@nikomatsakis
Copy link
Contributor Author

Ideally, I would think it should say something like "missing Ok", but that may be a separate bug -- i.e., maybe we should strive to just produce the same error you would get if you did let x = bar(); x

zackmdavis added a commit to zackmdavis/rust that referenced this issue Jun 30, 2018
@zackmdavis
in which we plug the crack where ?-desugaring leaked into errors
6cc78bf 
Most of the time, it's not a problem that the types of the arm bodies in
a desugared-from-`?` match are different (that is, specifically: in `x?`
where x is a `Result<A, B>`, the `Ok` arm body is an `A`, whereas the
`Err` arm diverges to return a `Result<A, B>`), because they're being
assigned to different places. But in tail position, the types do need to
match, and our error message was explicitly referring to "match arms",
which is confusing when there's no `match` in the sweetly sugared
source.

It is not without some misgivings that we pollute the clarity-of-purpose
of `note_error_origin` with the suggestion to wrap with `Ok` (the other
branches are pointing out the odd-arm-out in the HIR that is the origin
of the error; the new branch that issues the `Ok` suggestion is serving
a different purpose), but it's the natural place to do it given that
we're already matching on `ObligationCauseCode::MatchExpressionArm {
arm_span, source }` there.

Resolves rust-lang#51632.
@zackmdavis zackmdavis mentioned this issue Jun 30, 2018
Merged
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 2, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#51938 - zackmdavis:and_the_case_of_the_leak…
898882b 
…ing_desugar, r=estebank

in which we plug the crack where `?`-desugaring leaked into errors

Most of the time, it's not a problem that the types of the arm bodies in
a desugared-from-`?` match are different (that is, specifically: in `x?`
where x is a `Result<A, B>`, the `Ok` arm body is an `A`, whereas the
`Err` arm diverges to return a `Result<A, B>`), because they're being
assigned to different places. But in tail position, the types do need to
match, and our error message was explicitly referring to "match arms",
which is confusing when there's no `match` in the sweetly sugared
source.

It is not without some misgivings that we pollute the clarity-of-purpose
of `note_error_origin` with the suggestion to wrap with `Ok` (the other
branches are pointing out the odd-arm-out in the HIR that is the origin
of the error; the new branch that issues the `Ok` suggestion is serving
a different purpose), but it's the natural place to do it given that
we're already matching on `ObligationCauseCode::MatchExpressionArm {
arm_span, source }` there.

Resolves rust-lang#51632.
bors added a commit that referenced this issue Jul 5, 2018
@bors
Auto merge of #51938 - zackmdavis:and_the_case_of_the_leaking_desugar…
c865d39 
…, r=estebank

in which we plug the crack where `?`-desugaring leaked into errors

Most of the time, it's not a problem that the types of the arm bodies in
a desugared-from-`?` match are different (that is, specifically: in `x?`
where x is a `Result<A, B>`, the `Ok` arm body is an `A`, whereas the
`Err` arm diverges to return a `Result<A, B>`), because they're being
assigned to different places. But in tail position, the types do need to
match, and our error message was explicitly referring to "match arms",
which is confusing when there's no `match` in the sweetly sugared
source.

It is not without some misgivings that we pollute the clarity-of-purpose
of `note_error_origin` with the suggestion to wrap with `Ok` (the other
branches are pointing out the odd-arm-out in the HIR that is the origin
of the error; the new branch that issues the `Ok` suggestion is serving
a different purpose), but it's the natural place to do it given that
we're already matching on `ObligationCauseCode::MatchExpressionArm {
arm_span, source }` there.

Resolves #51632.
This was referenced Jul 21, 2018
Closed
Closed
@zackmdavis zackmdavis mentioned this issue Oct 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@nikomatsakis