type error method suggestions use whitelisted identity-like conversions #46461

zackmdavis · 2017-12-03T08:30:14Z

Previously, on a type mismatch (and if this wasn't preëmpted by a
higher-priority suggestion), we would look for argumentless methods
returning the expected type, and list them in a help note. This had two
major shortcomings: firstly, a lot of the suggestions didn't really make
sense (if you used a &str where a String was expected,
.to_ascii_uppercase() is probably not the solution you were hoping
for). Secondly, we weren't generating suggestions from the most useful
traits! We address the first problem with an internal
#[rustc_conversion_suggestion] attribute meant to mark methods that keep
the "same value" in the relevant sense, just converting the type. We
address the second problem by making FnCtxt.probe_for_return_type pass
the ProbeScope::AllTraits to probe_op: this would seem to be safe
because grep reveals no other callers of probe_for_return_type.

Also, structured suggestions are pretty and good for RLS and friends.

Unfortunately, the trait probing is still not all one would hope for: at a
minimum, we don't know how to rule out into() in cases where it wouldn't
actually work, and we don't know how to rule in .to_owned() where it
would. Issues #46459 and #46460 have been filed and are ref'd in a FIXME.

This is hoped to resolve #42929, #44672, and #45777.

zackmdavis · 2017-12-03T08:30:24Z

r? @arielb1

rust-highfive · 2017-12-03T08:30:24Z

r? @michaelwoerister

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

estebank · 2017-12-07T00:03:22Z

src/librustc_typeck/check/demand.rs

+                // "identity-like" conversion methods to be suggested here.
+                //
+                // FIXME (#46459 and #46460): ideally
+                // `std::convert::Into::into` and `std::borrow:ToOwned` would


Could into() be suggested if methods.len() == 0 with a note (not a suggestion) as a temporary hack? (Not saying that it'd be a great idea, just thinking out loud. In text.)

estebank · 2017-12-07T00:04:34Z

src/librustc_typeck/check/method/probe.rs

@@ -183,7 +183,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
               scope_expr_id);
        let method_names =
            self.probe_op(span, mode, None, Some(return_type), IsSuggestion(true),
-                          self_ty, scope_expr_id, ProbeScope::TraitsInScope,
+                          self_ty, scope_expr_id, ProbeScope::AllTraits,


Should we do this if and only if TraitsInScope doesn't yield any suggestions?

With no other callers, I feel OK about changing this.

estebank · 2017-12-07T00:05:05Z

src/test/ui/did_you_mean/recursion_limit_deref.stderr

@@ -19,5 +19,5 @@ error[E0308]: mismatched types
   = note: expected type `&Bottom`
              found type `&Top`

-error: aborting due to 3 previous errors
+error: aborting due to 51 previous errors


Oh boy, what happened here!? Given how much of an improvement this is, I wouldn't block on this, but it is worrisome if this PR is generating this many non-emitted diagnostics.

I would actually consider this blocking (changing behavior isn't always wrong, but changing behavior that no one understands is pretty bad).

It turns out that we don't emit exactly-equal (by hash) diagnostics, the method-probing code uses Autoderef, and Autoderef doesn't know when it's already set the recursion-limit error.

... or rather, it's not that Autoderef needs to know when it's already emitted the error, but rather that we instantiate Autoderef many times and hit the limit many times when searching for suggestions.

estebank · 2017-12-07T00:07:08Z

src/test/ui/span/coerce-suggestions.stderr

+36 |     f = box f.as_slice();
+   |              ^^^^^^^^^^^
+36 |     f = box f.to_string();
+   |              ^^^^^^^^^^^^


estebank · 2017-12-07T00:09:21Z

src/libsyntax/feature_gate.rs

    ("wasm_import_memory", Whitelisted, Gated(Stability::Unstable,
                                 "wasm_import_memory",
                                 "wasm_import_memory attribute is currently unstable",
-                                 cfg_fn!(wasm_import_memory))),
+                                              cfg_fn!(wasm_import_memory))),


Fix indentation (either change the indentation of the entire thing or revert to what it was :)

carols10cents · 2017-12-12T18:39:56Z

r? @estebank

because you've already started reviewing :)

It looks like there are some comments for you @zackmdavis ! Do they make sense?

zackmdavis · 2017-12-14T06:32:18Z

Yes, thanks, I haven't forgotten about this; I'm slow to address comments because every time I look at this issue it takes me a long time to get over the existential despair of how hard this codebase is to understand (for me).

bors · 2017-12-15T02:56:02Z

☔ The latest upstream changes (presumably #46641) made this pull request unmergeable. Please resolve the merge conflicts.

zackmdavis · 2017-12-27T07:25:41Z

(update a26b63b postdates most recent discussion)

alexcrichton · 2018-01-04T15:42:32Z

ping @estebank, looks like this may be ready for another review!

estebank

@bors r+

estebank · 2018-01-04T17:28:33Z

src/test/ui/suggestions/conversion-methods.stderr

+  --> $DIR/conversion-methods.rs:21:42
+   |
+21 |     let _should_the_glee_glaze: String = 2; // Perhaps surprisingly, we suggest .to_string() here
+   |                                          ^- help: try using a conversion method: `.to_string()`


Going through all of the examples, it seems to me that the output would be slightly easier to read if the help pointed at the main span, and the suggestion text included the main span's snippet:

--> $DIR/conversion-methods.rs:24:46 | 24 | let _in_deaths_stiff_stare: Vec<usize> = &[1, 2, 3]; //~ ERROR mismatched types | ^^^^^^^^^^ | | | expected struct `std::vec::Vec`, found reference | help: try using a conversion method: `&[1, 2, 3].to_vec()`

That being said, I think that can be done in a subsequent PR.

bors · 2018-01-04T17:54:27Z

📌 Commit a26b63b has been approved by estebank

ollie27 · 2018-01-04T17:57:55Z

src/libcore/array.rs

@@ -42,8 +42,10 @@ use slice::{Iter, IterMut};
 /// instead.
 pub unsafe trait FixedSizeArray<T> {
    /// Converts the array to immutable slice
+    #[rustc_conversion_suggestion]


FixedSizeArray is unstable and not in the prelude so it souldn't be suggested.

estebank · 2018-01-04T17:59:50Z

@bors r-, @zackmdavis could you follow @ollie27's comment? While you're at it, could you tweak the span as suggested in the earlier review?

@ollie27 I'd like the suggestion to be maybe there in nightly, but its just easier to take it out in this PR. Thanks for catching that.

bors · 2018-01-04T18:00:46Z

📌 Commit a26b63b has been approved by estebank

estebank · 2018-01-04T18:05:38Z

@bors r-

Previously, on a type mismatch (and if this wasn't preëmpted by a higher-priority suggestion), we would look for argumentless methods returning the expected type, and list them in a `help` note. This had two major shortcomings. Firstly, a lot of the suggestions didn't really make sense (if you used a &str where a String was expected, `.to_ascii_uppercase()` is probably not the solution you were hoping for). Secondly, we weren't generating suggestions from the most useful traits! We address the first problem with an internal `#[rustc_conversion_suggestion]` attribute meant to mark methods that keep the "same value" in the relevant sense, just converting the type. We address the second problem by making `FnCtxt.probe_for_return_type` pass the `ProbeScope::AllTraits` to `probe_op`: this would seem to be safe because grep reveals no other callers of `probe_for_return_type`. Also, structured suggestions are preferred (because they're pretty, but also for RLS and friends). Also also, we make the E0055 autoderef recursion limit error use the one-time-diagnostics set, because we can potentially hit the limit a lot during probing. (Without this, test/ui/did_you_mean/recursion_limit_deref.rs would report "aborting due to 51 errors"). Unfortunately, the trait probing is still not all one would hope for: at a minimum, we don't know how to rule out `into()` in cases where it wouldn't actually work, and we don't know how to rule in `.to_owned()` where it would. Issues rust-lang#46459 and rust-lang#46460 have been filed and are ref'd in a FIXME. This is hoped to resolve rust-lang#42929, rust-lang#44672, and rust-lang#45777.

shepmaster · 2018-01-13T00:53:26Z

Ping from triage, @estebank. It looks like there are new commits for you to review!

estebank · 2018-01-13T01:20:24Z

@bors r+

bors · 2018-01-13T01:20:25Z

📌 Commit aba56dd has been approved by estebank

bors · 2018-01-13T02:15:27Z

⌛ Testing commit aba56dd with merge 6eff103...

…e, r=estebank type error method suggestions use whitelisted identity-like conversions ![method_jamboree_summit](https://user-images.githubusercontent.com/1076988/33523646-e5c43184-d7c0-11e7-98e5-1bff426ade86.png) Previously, on a type mismatch (and if this wasn't preëmpted by a higher-priority suggestion), we would look for argumentless methods returning the expected type, and list them in a `help` note. This had two major shortcomings: firstly, a lot of the suggestions didn't really make sense (if you used a &str where a String was expected, `.to_ascii_uppercase()` is probably not the solution you were hoping for). Secondly, we weren't generating suggestions from the most useful traits! We address the first problem with an internal `#[rustc_conversion_suggestion]` attribute meant to mark methods that keep the "same value" in the relevant sense, just converting the type. We address the second problem by making `FnCtxt.probe_for_return_type` pass the `ProbeScope::AllTraits` to `probe_op`: this would seem to be safe because grep reveals no other callers of `probe_for_return_type`. Also, structured suggestions are pretty and good for RLS and friends. Unfortunately, the trait probing is still not all one would hope for: at a minimum, we don't know how to rule out `into()` in cases where it wouldn't actually work, and we don't know how to rule in `.to_owned()` where it would. Issues #46459 and #46460 have been filed and are ref'd in a FIXME. This is hoped to resolve #42929, #44672, and #45777.

bors · 2018-01-13T05:01:59Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 6eff103 to master...

rust-highfive assigned michaelwoerister Dec 3, 2017

rust-highfive assigned arielb1 and unassigned michaelwoerister Dec 3, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2017

estebank reviewed Dec 7, 2017

View reviewed changes

zackmdavis mentioned this pull request Dec 9, 2017

No to_string method in suggestions to &str rust-lang/rls#618

Closed

rust-highfive assigned estebank and unassigned arielb1 Dec 12, 2017

carols10cents 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 Dec 12, 2017

kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2017

zackmdavis force-pushed the elemental_method_suggestion_jamboree branch 2 times, most recently from 06442d7 to a26b63b Compare December 24, 2017 03:53

alexcrichton 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 Jan 4, 2018

estebank approved these changes Jan 4, 2018

View reviewed changes

ollie27 reviewed Jan 4, 2018

View reviewed changes

zackmdavis force-pushed the elemental_method_suggestion_jamboree branch from a26b63b to aba56dd Compare January 7, 2018 01:17

shepmaster 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 Jan 13, 2018

bors merged commit aba56dd into rust-lang:master Jan 13, 2018

bors mentioned this pull request Jan 13, 2018

Suggest casting on numeric type error #47247

Merged

estebank mentioned this pull request Jan 15, 2018

Do not suggest unwrap on Some(T) where T is expected #42746

Closed

hannobraun mentioned this pull request Jun 12, 2018

Compiler doesn't suggest missing method that is implemented for different type parameter #51518

Open

zackmdavis deleted the elemental_method_suggestion_jamboree branch June 13, 2018 08:35

zackmdavis mentioned this pull request Jul 29, 2018

conversion method suggestion is wrong given struct field init shorthand #52820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

type error method suggestions use whitelisted identity-like conversions #46461

type error method suggestions use whitelisted identity-like conversions #46461

zackmdavis commented Dec 3, 2017

zackmdavis commented Dec 3, 2017

rust-highfive commented Dec 3, 2017

estebank Dec 7, 2017

zackmdavis Dec 24, 2017

estebank Dec 7, 2017

zackmdavis Dec 24, 2017

estebank Dec 7, 2017

zackmdavis Dec 14, 2017

zackmdavis Dec 24, 2017

zackmdavis Dec 24, 2017

estebank Dec 7, 2017

estebank Dec 7, 2017

carols10cents commented Dec 12, 2017

zackmdavis commented Dec 14, 2017

bors commented Dec 15, 2017

zackmdavis commented Dec 27, 2017

alexcrichton commented Jan 4, 2018

estebank left a comment

estebank Jan 4, 2018

bors commented Jan 4, 2018

ollie27 Jan 4, 2018

estebank commented Jan 4, 2018

bors commented Jan 4, 2018

estebank commented Jan 4, 2018

shepmaster commented Jan 13, 2018

estebank commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

type error method suggestions use whitelisted identity-like conversions #46461

type error method suggestions use whitelisted identity-like conversions #46461

Conversation

zackmdavis commented Dec 3, 2017

zackmdavis commented Dec 3, 2017

rust-highfive commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Dec 12, 2017

zackmdavis commented Dec 14, 2017

bors commented Dec 15, 2017

zackmdavis commented Dec 27, 2017

alexcrichton commented Jan 4, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 4, 2018

Choose a reason for hiding this comment

estebank commented Jan 4, 2018

bors commented Jan 4, 2018

estebank commented Jan 4, 2018

shepmaster commented Jan 13, 2018

estebank commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018