Skip to content

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

Merged
merged 1 commit into from
Feb 27, 2013
Merged

Conversation

youknowone
Copy link
Contributor

It is reversed that type of arm pattern and type of search pattern
in error message.

@@ -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);
Copy link
Contributor

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.

Copy link
Contributor

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).

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

@youknowone youknowone Feb 23, 2013

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

@pcwalton
Copy link
Contributor

r? @nikomatsakis

@youknowone
Copy link
Contributor Author

@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

@nikomatsakis
Copy link
Contributor

@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 #1234 would be the number of the bug you open

Fix some reversed type of arm pattern and type of search pattern
in error message.
@youknowone
Copy link
Contributor Author

@nikomatsakis I was stupid. I fixed it too now.

@nikomatsakis
Copy link
Contributor

@youknowone thanks very much for this. I just marked it r+.

bors added a commit that referenced this pull request Feb 27, 2013
It is reversed that type of arm pattern and type of search pattern
in error message.
@bors bors closed this Feb 27, 2013
@bors bors merged commit 35baf5b into rust-lang:incoming Feb 27, 2013
@youknowone
Copy link
Contributor Author

thanks!

@youknowone youknowone deleted the struct-match2 branch February 28, 2013 02:16
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2020
Improve `suspicious_map`documentation

Fixes rust-lang#4793

changelog: none
calebcartwright added a commit to calebcartwright/rust that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants