-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix reversed current/expected type #5070
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
Conversation
@@ -142,7 +142,7 @@ pub fn check_pat_variant(pcx: pat_ctxt, pat: @ast::pat, path: @ast::path, | |||
// Check that the type of the value being matched is a subtype of | |||
// the type of the pattern. | |||
let pat_ty = fcx.node_ty(pat.id); | |||
demand::suptype(fcx, pat.span, pat_ty, expected); | |||
demand::suptype(fcx, pat.span, expected, pat_ty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is wrong to swap the order of these two types. The pat_ty should be be the supertype. If the problem is the error message is coming out incorrect, there is another way to solve it, though it's mildly more involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think in this case which one is "expected" and not is a matter of perspective. I kind of expect the pattern to be the "expected" type in the print-out, which is precisely how it works now (before this change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. I think "found" type should be the type of the expression highlighted, e.g. let a: int = 'a' highlights 'a' and says expected 'int' but found 'char'. In this case the pattern is highlighted, so either pattern should be "found" type, or highlighting should change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Well, I am somewhat indifferent. It's easy enough to change where the highlight is placed, and it's easy enough to change which type is expected. But this specific change is semantically incorrect. Let me know which way you'd rather go and I'll advise how best to get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. I got what I was wrong. I am making new patch for this.
I understands:
- it should not test suptype in reversed direction.
- but highlighted type and error message is mismatched.
Is it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, highlighting should stay, since the pattern can be far away from the matched expression. So how does one change which type is "expected" in the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about b218765f ? Could I replace this commit with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close but it is also incorrect. I left a comment in that comment. I think the general idea of adding demand::subtype
and demand::suptype
is fine, that's more-or-less what I was going to suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've fixed it like 7cdc800
@nikomatsakis I pushed new version with new variable name. But I didn't remove XXX because I couldn't resolve that case: https://gist.github.com/youknowone/5040645 |
@youknowone I don't understand, what is the output from that case and what is the expected output? In any case, if you want to keep the XXX in there, you need to open a bug and change it to me FIXME(#1234), where |
Fix some reversed type of arm pattern and type of search pattern in error message.
@nikomatsakis I was stupid. I fixed it too now. |
@youknowone thanks very much for this. I just marked it r+. |
It is reversed that type of arm pattern and type of search pattern in error message.
thanks! |
Improve `suspicious_map`documentation Fixes rust-lang#4793 changelog: none
It is reversed that type of arm pattern and type of search pattern
in error message.