Add method disambiguation help for trait implementation #62921

iluuu1994 · 2019-07-23T23:57:47Z

rust-highfive · 2019-07-23T23:57:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

cramertj · 2019-07-23T23:59:52Z

r? @estebank

estebank

Given the nature of the change, you can ignore my nitpicks, but it would be a good follow up task.

r=me once ci is happy

estebank · 2019-07-24T00:14:56Z

src/test/ui/methods/method-ambig-two-traits-from-impls.stderr

+   |     ^^^^^^^^^^^^
+   = help: to disambiguate the method call, write `B::foo(AB{})` instead
+
+error: aborting due to previous error


I think that the ideal output for this would instead be:

error[E0034]: multiple applicable items in scope --> $DIR/method-ambig-two-traits-from-impls.rs:15:11 | LL | AB {}.foo(); | ^^^ multiple `foo` found | note: candidate #1 is defined in an impl of the trait `A` for the type `AB` --> $DIR/method-ambig-two-traits-from-impls2.rs:7:5 | LL | fn foo() {} | ^^^^^^^^ note: candidate #2 is defined in an impl of the trait `B` for the type `AB` --> $DIR/method-ambig-two-traits-from-impls2.rs:11:5 | LL | fn foo() {} | ^^^^^^^^ help: disambiguate the method call to a specific trait: --> $DIR/method-ambig-two-traits-from-impls.rs:15:4 | LL | A::foo(AB {}) | ^^^^^^^^^^^^^ LL | B::foo(AB {}) | ^^^^^^^^^^^^^

The only reason I'm leaving the notes it's because that information is still useful and we can't (yet, at least) interleave suggestions and notes.

src/librustc_typeck/check/method/suggest.rs

estebank · 2019-07-24T00:18:01Z

src/test/ui/methods/method-ambig-two-traits-from-impls2.stderr

+   |     ^^^^^^^^
+   = help: to disambiguate the method call, write `B::foo(...)` instead
+
+error: aborting due to previous error


I think that ideally the output would be closer to

error[E0034]: multiple applicable items in scope --> $DIR/method-ambig-two-traits-from-impls.rs:15:11 | LL | AB {}.foo(); | ^^^ multiple `foo` found | note: candidate #1 is defined in an impl of the trait `A` for the type `AB` --> $DIR/method-ambig-two-traits-from-impls2.rs:7:5 | LL | fn foo() {} | ^^^^^^^^ note: candidate #2 is defined in an impl of the trait `B` for the type `AB` --> $DIR/method-ambig-two-traits-from-impls2.rs:11:5 | LL | fn foo() {} | ^^^^^^^^ help: disambiguate the method call to a specific trait: --> $DIR/method-ambig-two-traits-from-impls.rs:15:4 | LL | A::foo(AB {}) | ^^^^^^^^^^^^^ LL | B::foo(AB {}) | ^^^^^^^^^^^^^

Note that the help here would be created with err.span_suggestions, not err.span_note.

estebank · 2019-07-24T00:20:24Z

src/librustc_typeck/check/method/suggest.rs

-                                                                          |s| s.print_expr(arg)))
-                                              .collect::<Vec<_>>()
-                                              .join(", ")).unwrap_or_else(|| "...".to_owned())));
+                        print_disambiguation_help(err, self.tcx.def_path_str(trait_did));


I see that you're just moving code that was already there. If changing this to use a span_suggestion feels too much work for you to take on, we can merge this PR as is.

feels too much work for you to take on, we can merge this PR as is

It's fine, I'll try to implement your suggestions 🙂

I created #62922 to track that work. If the tests pass on the current state, I'll just approve and then you can implement those suggestions at your own pace. They aren't difficult but might be time consuming. If you need any help with them, do not hesitate to reach out, and there are plenty of examples of how the span_suggestion api is supposed to be used.

Closes rust-lang#51046 Closes rust-lang#40471

iluuu1994 · 2019-07-24T09:40:48Z

src/librustc_typeck/check/method/suggest.rs

+                args.map(|arg| arg
+                    .iter()
+                    .map(|arg| self.tcx.sess.source_map().span_to_snippet(arg.span)
+                        .unwrap_or_else(|_| "...".to_owned()))


@estebank This this unwrap fine? When can this happen?

It shouldn't happen, but it can if the span is coming from a proc macro or an external crate.

iluuu1994 · 2019-07-24T09:44:02Z

I made the changes (except for #62922). Tests are passing 🙂

estebank · 2019-07-24T16:54:04Z

@bors r+ rollup

bors · 2019-07-24T16:54:05Z

📌 Commit be510db has been approved by estebank

…isambiguation, r=estebank Add method disambiguation help for trait implementation Closes rust-lang#51046 Closes rust-lang#40471

@ghost

Rollup of 13 pull requests Successful merges: - rust-lang#60938 (rustdoc: make #[doc(include)] relative to the containing file) - rust-lang#61890 (Fix some sanity checks) - rust-lang#62084 (allow clippy::unreadable_literal in unicode tables) - rust-lang#62261 (Take substs into account in `conservative_is_privately_uninhabited`) - rust-lang#62528 (Add joining slices of slices with a slice separator, not just a single item) - rust-lang#62735 (Turn `#[global_allocator]` into a regular attribute macro) - rust-lang#62801 (Remove support for -Zlower-128bit-ops) - rust-lang#62808 (Revert "Disable stack probing for gnux32.") - rust-lang#62822 (Improve some pointer-related documentation) - rust-lang#62904 (Disable d32 on armv6 hf targets) - rust-lang#62907 (Initialize the MSP430 AsmParser) - rust-lang#62921 (Add method disambiguation help for trait implementation) - rust-lang#62942 (Use match ergonomics in Condvar documentation) Failed merges: r? @ghost

…isambiguation, r=estebank Add method disambiguation help for trait implementation Closes rust-lang#51046 Closes rust-lang#40471

@ghost

Rollup of 15 pull requests Successful merges: - #60066 (Stabilize the type_name intrinsic in core::any) - #60938 (rustdoc: make #[doc(include)] relative to the containing file) - #61884 (Stablize Euclidean Modulo (feature euclidean_division)) - #61890 (Fix some sanity checks) - #62528 (Add joining slices of slices with a slice separator, not just a single item) - #62707 (Add tests for overlapping explicitly dropped locals in generators) - #62735 (Turn `#[global_allocator]` into a regular attribute macro) - #62822 (Improve some pointer-related documentation) - #62887 (Make the parser TokenStream more resilient after mismatched delimiter recovery) - #62921 (Add method disambiguation help for trait implementation) - #62930 (Add test for #51559) - #62942 (Use match ergonomics in Condvar documentation) - #62977 (Fix inconsistent highlight blocks.) - #62978 (Remove `cfg(bootstrap)` code for array implementations) - #62981 (Add note suggesting to borrow a String argument to find) Failed merges: - #62964 (clarify and unify some type test names) r? @ghost

haraldh · 2019-08-01T09:32:04Z

Want to add one more thing:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=982968be09c754ddfa53e34b4247765b

iluuu1994 · 2019-08-01T09:41:34Z

@haraldh Similar cases are already tracked here: #58988

Feel free to leave another comment there with your snippet.

rust-highfive assigned cramertj Jul 23, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2019

rust-highfive assigned estebank and unassigned cramertj Jul 23, 2019

estebank approved these changes Jul 24, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

iluuu1994 added 2 commits July 24, 2019 03:03

Add method disambiguation help for trait implementation

bd8813e

Closes rust-lang#51046 Closes rust-lang#40471

Adjust tests for method disambiguation help

be510db

iluuu1994 force-pushed the improve-help-for-method-disambiguation branch from 8b0b22e to be510db Compare July 24, 2019 01:08

estebank mentioned this pull request Jul 24, 2019

Rework method disambiguation error to use structured suggestions #62922

Closed

iluuu1994 commented Jul 24, 2019

View reviewed changes

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 Jul 24, 2019

Centril mentioned this pull request Jul 24, 2019

Rollup of 13 pull requests #62960

Closed

Centril mentioned this pull request Jul 25, 2019

Rollup of 9 pull requests #62966

Closed

Centril mentioned this pull request Jul 25, 2019

Rollup of 15 pull requests #62990

Merged

bors merged commit be510db into rust-lang:master Jul 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method disambiguation help for trait implementation #62921

Add method disambiguation help for trait implementation #62921

iluuu1994 commented Jul 23, 2019

rust-highfive commented Jul 23, 2019

cramertj commented Jul 23, 2019

estebank left a comment •

edited

Loading

estebank Jul 24, 2019

estebank Jul 24, 2019

estebank Jul 24, 2019

iluuu1994 Jul 24, 2019

estebank Jul 24, 2019

This comment has been minimized.

iluuu1994 Jul 24, 2019

estebank Jul 24, 2019

iluuu1994 commented Jul 24, 2019

estebank commented Jul 24, 2019

bors commented Jul 24, 2019

haraldh commented Aug 1, 2019

iluuu1994 commented Aug 1, 2019

Add method disambiguation help for trait implementation #62921

Add method disambiguation help for trait implementation #62921

Conversation

iluuu1994 commented Jul 23, 2019

rust-highfive commented Jul 23, 2019

cramertj commented Jul 23, 2019

estebank left a comment • edited Loading

Choose a reason for hiding this comment

estebank Jul 24, 2019

Choose a reason for hiding this comment

estebank Jul 24, 2019

Choose a reason for hiding this comment

estebank Jul 24, 2019

Choose a reason for hiding this comment

iluuu1994 Jul 24, 2019

Choose a reason for hiding this comment

estebank Jul 24, 2019

Choose a reason for hiding this comment

This comment has been minimized.

iluuu1994 Jul 24, 2019

Choose a reason for hiding this comment

estebank Jul 24, 2019

Choose a reason for hiding this comment

iluuu1994 commented Jul 24, 2019

estebank commented Jul 24, 2019

bors commented Jul 24, 2019

haraldh commented Aug 1, 2019

iluuu1994 commented Aug 1, 2019

estebank left a comment •

edited

Loading