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

fix misleading type annotation diagonstics #69456

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

contrun
Copy link
Contributor

@contrun contrun commented Feb 25, 2020

This solves the method call part of issue #69455

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2020
@LeSeulArtichaut
Copy link
Contributor

You may want to add a regression test for this. See the rustc book.

@rust-highfive

This comment has been minimized.

@contrun contrun force-pushed the fix-misleading-compiler-error branch 5 times, most recently from 744fb23 to 6153629 Compare February 26, 2020 00:25
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@contrun contrun force-pushed the fix-misleading-compiler-error branch from 6153629 to 6d7e5e2 Compare March 14, 2020 05:49
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2020
@JohnCSimon
Copy link
Member

Ping from triage:
@contrun any updates on this?

@contrun
Copy link
Contributor Author

contrun commented Apr 6, 2020

@JohnCSimon As I explained in #69455 (comment), the problem is that my PR will eliminate some relevant error message in some cases. The current error messages obviously more informative. I can push a new commit to resolve the conflict and use --bless to change the error message to accommodate my PR. But I don't know if that is desirable.

@contrun contrun force-pushed the fix-misleading-compiler-error branch from 6d7e5e2 to 25ec438 Compare April 6, 2020 15:28
@rust-highfive

This comment has been minimized.

@cramertj
Copy link
Member

cramertj commented Apr 6, 2020

Yes, if you could --bless the output so that we can see what the changes are easily, that would be helpful!

@bors

This comment has been minimized.

@contrun contrun force-pushed the fix-misleading-compiler-error branch from ccbb4e1 to 88c45cf Compare April 8, 2020 02:42
This solves the method call part of issue
rust-lang#69455
I added a `target_span` field so as to pin down the exact location of
the error. We need a dedicated field `found_exact_method_call` to
prioritize situations like the test case `issue-69455.rs`. If we reuse
`found_method_call`, `found_local_pattern` will show up first. We can
not move `found_method_call` up, it is undesirable in various
situations.
@contrun contrun force-pushed the fix-misleading-compiler-error branch from 88c45cf to 3ae974f Compare April 8, 2020 03:53
@contrun
Copy link
Contributor Author

contrun commented Apr 8, 2020

@cramertj It is now ready for review. I added two field in the original struct. They are needed because I can not find any way to it without affecting existing error messages.

I added a target_span field so as to pin down the exact location of
the error. We need a dedicated field found_exact_method_call to
prioritize situations like the test case #69455. If we reuse
found_method_call, found_local_pattern will show up first. This is what rustc currently shows, which is a red herring.
We can not move found_method_call up, it is undesirable in various
situations. I tried it. Many tests failed.

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2020
@crlf0710
Copy link
Member

r? @estebank

| ------^^^^-----------------
| | |
| | cannot infer type for type `u64`
| this method call resolves to `<Self as Test<Rhs>>::Output`
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be amazing if we actually suggested <u64 as Test<u64>>::test(23u64, xs.iter().sum()), but that is beyond the scope of this PR.

@estebank
Copy link
Contributor

The code looks ok to me. @contrun could you add tests for the cases you mention in your original comment would regress? I'm not sure I can see what they would be.

@contrun
Copy link
Contributor Author

contrun commented Apr 24, 2020

@estebank
Did you mean this one?

fn main() {
    let xs: Vec<u64> = vec![1, 2, 3];
    println!("{}", 23u64 + xs.iter().sum());
}

Unfortunately, my current PR does not solve that yet. This is because + is not parsed as a method call. It is parsed as a BinOp. I will submit a separate PR for that.

@estebank
Copy link
Contributor

@contrun it is ok if this PR doesn't fix it, but having the test in the codebase helps us catch when an unrelated change either improves or degrades it. It also makes it easier for the reviewer to see the explicitly ignored cases.

That being said, now I see what the problem is and it shouldn't block this PR.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2020

📌 Commit 3ae974f has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#69456 (fix misleading type annotation diagonstics)
 - rust-lang#71330 (Only run dataflow for const qualification if type-based check would fail)
 - rust-lang#71480 (Improve PanicInfo examples readability)
 - rust-lang#71485 (Add BinaryHeap::retain as suggested in rust-lang#42849)
 - rust-lang#71512 (Remove useless "" args)
 - rust-lang#71527 (Miscellaneous cleanup in `check_consts`)
 - rust-lang#71534 (Avoid unused Option::map results)
 - rust-lang#71535 (Fix typos in docs for keyword "in")

Failed merges:

r? @ghost
@bors bors merged commit 2e2080d into rust-lang:master Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants