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

Point at return type always when type mismatch against it #43484

Merged
merged 3 commits into from
Aug 9, 2017

Conversation

estebank
Copy link
Contributor

Before this, the diagnostic errors would only point at the return type
when changing it would be a possible solution to a type error. Add a
label to the return type without a suggestion to change in order to make
the source of the expected type obvious.

Follow up to #42850, fixes #25133, fixes #41897.

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

Before this, the diagnostic errors would only point at the return type
when changing it would be a possible solution to a type error. Add a
label to the return type without a suggestion to change in order to make
the source of the expected type obvious.

Follow up to rust-lang#42850, fixes rust-lang#25133, fixes rust-lang#41897.
@estebank
Copy link
Contributor Author

r? @Mark-Simulacrum as this PR was prompted by this comment.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Very happy to see this, thanks for working on it!

@@ -1,6 +1,8 @@
error[E0308]: mismatched types
--> $DIR/issue-13624.rs:17:5
|
16 | pub fn get_enum_struct_variant() -> () {
| -- expected `()` because of return type
17 | Enum::EnumStructVariant { x: 1, y: 2, z: 3 }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found enum `a::Enum`
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it looks like in the new note we put the empty tuple in backticks, but don't in the old text. This seems odd; any chance it's an easy fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to the original output at @arielb1's request.

@@ -1,6 +1,9 @@
error[E0308]: mismatched types
--> $DIR/equality.rs:25:5
|
21 | fn two(x: bool) -> impl Foo {
| -------- expected `_` because of return type
Copy link
Member

Choose a reason for hiding this comment

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

This note seems odd; presumably we expected i32? I'm not sure, but that seems to be the case here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all suggestions involving impl Trait.

@alexcrichton alexcrichton added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 27, 2017
@aidanhs
Copy link
Member

aidanhs commented Aug 3, 2017

@estebank have you had a chance to have a look at the review comments yet? Just want to make sure this doesn't get lost!

@Mark-Simulacrum
Copy link
Member

Ah, this should also be r? @arielb1 or someone else, I'm not sufficiently familiar with these compiler internals.

@estebank
Copy link
Contributor Author

estebank commented Aug 4, 2017

I am worried about that the current approach isn't 100% accurate. Although this will point out the cause of the expected type most of the time, src/test/ui/impl-trait/equality.stderr shows a case where where type inference caused the expected type to be caused not by the fn's return type, but by the type inference's interaction with if branches of different types. I've come across other examples, but I'm failing to find them now.

I'm hesitant to merge this if it will ever be misleading, but making it work 100% of the time would mean start keeping track of the traceback for all inferred types, something that I've been thinking about doing (but has the potential to make the compiler use a bit more memory).

@@ -664,7 +664,7 @@ impl<'hir> Map<'hir> {
match *node {
NodeExpr(ref expr) => {
match expr.node {
ExprWhile(..) | ExprLoop(..) => true,
ExprWhile(..) | ExprLoop(..) | ExprIf(..) => true,
Copy link
Contributor

@arielb1 arielb1 Aug 7, 2017

Choose a reason for hiding this comment

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

This makes the comment above wrong. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted. I was considering removing all suggestions when involving if blocks due to the accuracy problem mentioned earlier. Given that now I only provide the suggestion if the types are the same, it can be safely reverted.

@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

@estebank

About the expected _ because of return type case: the only time the return type can contain inference variables is because we have an impl Trait with intra-function conflicts. I think we should just disable the error message if the return type contains impl Trait, because that indicates a conflict (there are also cases such as the return type being Option<impl Trait> where we could try to separate conflicts from non-conflicts, so we could instead try to check whether our type can be coerced into the return type if we wanted. I'm fine with it either way).

debug!("suggest_missing_return_type: expected type sty {:?}", ty.sty);
if ty.sty == expected.sty {
let quote = if let TypeVariants::TyTuple(ref slice, _) = expected.sty {
if slice.len() == 0 { // don't use backtics for ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this?

In normal error messages:

fn main() {
    let x: u32 = ();
}

The backticks appear when mentioning the type name, but not when mentioning the type "constructor":


error[E0308]: mismatched types
 --> src/main.rs:2:18
  |
2 |     let x: u32 = ();
  |                  ^^ expected u32, found ()
  |
  = note: expected type `u32`
             found type `()`

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you are going to change that error message too to not have backticks, I'll want to remove this check.

@arielb1
Copy link
Contributor

arielb1 commented Aug 9, 2017

r=me with the backticks back.

@estebank
Copy link
Contributor Author

estebank commented Aug 9, 2017

@bors r=arielb1

@bors
Copy link
Contributor

bors commented Aug 9, 2017

📌 Commit 9bd62a4 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Aug 9, 2017

⌛ Testing commit 9bd62a4 with merge f142499...

bors added a commit that referenced this pull request Aug 9, 2017
Point at return type always when type mismatch against it

Before this, the diagnostic errors would only point at the return type
when changing it would be a possible solution to a type error. Add a
label to the return type without a suggestion to change in order to make
the source of the expected type obvious.

Follow up to #42850, fixes #25133, fixes #41897.
@bors
Copy link
Contributor

bors commented Aug 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing f142499 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
7 participants