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.

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

Make diagnostics clearer for ? operator #75029

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/librustc_infer/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1604,6 +1604,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
trace: TypeTrace<'tcx>,
terr: &TypeError<'tcx>,
) -> DiagnosticBuilder<'tcx> {
use crate::traits::ObligationCauseCode::MatchExpressionArm;
debug!("report_and_explain_type_error(trace={:?}, terr={:?})", trace, terr);

let span = trace.cause.span(self.tcx);
Expand All @@ -1620,7 +1621,25 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
struct_span_err!(self.tcx.sess, span, E0580, "{}", failure_str)
}
FailureCode::Error0308(failure_str) => {
struct_span_err!(self.tcx.sess, span, E0308, "{}", failure_str)
let mut diag = struct_span_err!(self.tcx.sess, span, E0308, "{}", failure_str);
if let MatchExpressionArm(box MatchExpressionArmCause { source, .. }) =
trace.cause.code
{
if let hir::MatchSource::TryDesugar = source {
if let Some((expected_ty, found_ty)) = self.values_str(&trace.values) {
diag.note(&format!(
"`?` operator cannot convert from `{}` to `{}`",
found_ty.content(),
expected_ty.content()
));
diag.help(
"`?` operator unwraps `Result` or `Option` \
and propagates its `Err` or `None`",
);
}
}
}
diag
}
FailureCode::Error0644(failure_str) => {
struct_span_err!(self.tcx.sess, span, E0644, "{}", failure_str)
Expand Down Expand Up @@ -2088,9 +2107,7 @@ impl<'tcx> ObligationCauseExt<'tcx> for ObligationCause<'tcx> {
hir::MatchSource::IfLetDesugar { .. } => {
"`if let` arms have incompatible types"
}
hir::MatchSource::TryDesugar => {
"try expression alternatives have incompatible types"
}
hir::MatchSource::TryDesugar => "`?` operator has incompatible types",
_ => "`match` arms have incompatible types",
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ fn missing_discourses() -> Result<isize, ()> {

fn forbidden_narratives() -> Result<isize, ()> {
missing_discourses()?
//~^ ERROR try expression alternatives have incompatible types
//~^ ERROR: `?` operator has incompatible types
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
error[E0308]: try expression alternatives have incompatible types
error[E0308]: `?` operator has incompatible types
--> $DIR/issue-51632-try-desugar-incompatible-types.rs:8:5
|
LL | missing_discourses()?
| ^^^^^^^^^^^^^^^^^^^^^ expected enum `std::result::Result`, found `isize`
|
= note: `?` operator cannot convert from `isize` to `std::result::Result<isize, ()>`
= help: `?` operator unwraps `Result` or `Option` and propagates its `Err` or `None`
Comment on lines +7 to +8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's verbose?

Copy link
Contributor

@Lonami Lonami Aug 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this last note is not necessary. The clarification with concrete types of the first note should be enough, and people may be using the try_trait feature with custom types where this won't apply.

Though I can see this as The explanation that makes it clear for some people.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I feel the second note is probably unnecessary, no-one is likely to try to use ? without knowing that Result and Option are supported.

= note: expected enum `std::result::Result<isize, ()>`
found type `isize`
help: try removing this `?`
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-59756.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn foo() -> Result<A, B> {

fn bar() -> Result<A, B> {
foo()?
//~^ ERROR try expression alternatives have incompatible types [E0308]
//~^ ERROR `?` operator has incompatible types [E0308]
}

fn main() {}
7 changes: 7 additions & 0 deletions src/test/ui/issues/issue-71309.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
fn foo(x: Result<i32, ()>) -> Result<(), ()> {
let y: u32 = x?;
//~^ ERROR: `?` operator has incompatible types
Ok(())
}

fn main() {}
16 changes: 16 additions & 0 deletions src/test/ui/issues/issue-71309.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0308]: `?` operator has incompatible types
--> $DIR/issue-71309.rs:2:18
|
LL | let y: u32 = x?;
| ^^ expected `u32`, found `i32`
|
= note: `?` operator cannot convert from `i32` to `u32`
= help: `?` operator unwraps `Result` or `Option` and propagates its `Err` or `None`
help: you can convert an `i32` to `u32` and panic if the converted value wouldn't fit
|
LL | let y: u32 = x?.try_into().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^
Comment on lines +11 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion seems to be using the incorrect Span. It should be replacing the ? with the new code.


error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.