Start of implementation of proposal for E0308 #37388

GuillaumeGomez · 2016-10-25T00:05:45Z

oli-obk · 2016-10-25T09:30:10Z

src/librustc/infer/error_reporting.rs

+                if let Some(impl_infos) = self.tcx.inherent_impls.borrow().get(&found) {
+                    let mut methods = Vec::new();
+                    for impl_ in impl_infos {
+                        methods.append(&mut self.tcx


use .extend and remove the .collect()

I am sure that this can be replaced with flat_map, isn't it? This will reduce allocations, I think. Or just with nested for loop.

KalitaAlexey · 2016-10-25T10:36:06Z

src/librustc/infer/error_reporting.rs

@@ -527,7 +527,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
    {
        let expected_found = match values {
            None => None,
-            Some(values) => match self.values_str(&values) {
+            Some(ref values) => match self.values_str(&values) {


Could somebody explain me why ref is added?

To avoid moving values says rustc.

Oh. I understood. We can write self.values_str(values) because values is reference or am I wrong?

No, I think you're right. I didn't focus on this point when I wrote it. Thanks! :)

@GuillaumeGomez Will you fix this before merge?

Of course I will. This is far from merge anyway. I opened in order to allow @jonathandturner to follow.

nikomatsakis · 2016-10-25T17:59:32Z

cc me

KalitaAlexey · 2016-10-26T04:43:27Z

src/librustc/infer/error_reporting.rs

@@ -565,19 +565,29 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                // look for expected with found id
                self.tcx.populate_inherent_implementations_for_type_if_necessary(found);
                if let Some(impl_infos) = self.tcx.inherent_impls.borrow().get(&found) {
-                    let mut methods = Vec::new();


Can you write like this:

let methods = impl_infos.iter() .flat_map(|impl_| { self.tcx .impl_or_trait_items(*impl_) .iter() .map(|&did| (None, did, self.tcx.impl_or_trait_item(did))) .filter(|&(_, _, ref x)| { self.matches_return_type(x, &expected_ty) }) });

I didn't test it, so it may be impossible.

eddyb · 2016-10-26T22:14:44Z

src/librustc_typeck/check/method/probe.rs

+                                self.matches_return_type(item, &item_ty)
+                            })
+            }
+        };


You should've replaced the original code to a call to impl_or_trait_item - since they're equivalent ;).

GuillaumeGomez · 2016-10-26T22:48:55Z

We now have for the following code:

fn main() {
    let x: usize = String::new();
}

the following error output:

error[E0308]: mismatched types
 --> t.rs:2:20
  |
2 |     let x: usize = String::new();
  |                    ^^^^^^^^^^^^^ expected usize, found struct `std::string::String`
  |
  = note: expected type `usize`
  = note:    found type `std::string::String`
  = help: no safe suggestion found, here are functions which match your needs but be careful:
 - capacity
 - len

error: aborting due to previous error

A few things remain to be done but this is a good start:

I created the safe_suggestion attribute.
I filter potential methods based on some precise parameters (others could be added but like I said, this is a start).
The DiagnosticBuilder is now returned in order to be able to add the suggestions into the help.

What remains to be done:

& propositions (String -> &str for example).
& vs &mut.
Add safe_suggestion attributes on "conversion" methods (the list is quite long...).
Maybe add more filters on the suggestions we have.

eddyb · 2016-10-26T22:54:29Z

I don't like safe_suggestion - I'd rather we do it through (relatively simple) heuristics rather than annotating half of the libraries.

Also, let x: T = y: S; could suggest <T>::method if it's S -> T.
That would require a "search by first argument" mode, too.

GuillaumeGomez · 2016-10-26T22:55:09Z

src/libcollections/string.rs

@@ -1234,6 +1234,7 @@ impl String {
    /// assert_eq!(a.len(), 3);
    /// ```
    #[inline]
+    #[cfg_attr(not(stage0), safe_suggestion)]


Which reminds me that this line should be removed.

bors · 2016-10-27T07:52:36Z

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

sophiajt · 2016-10-29T22:30:18Z

Can you post some examples of what the output currently looks like for the new errors?

GuillaumeGomez · 2016-10-29T22:32:46Z

Sure. So for the following file:

fn test(_x: &mut String) {}
fn test2(_x: &mut i32) {}

fn main() {
    let x: usize = String::new();
    let x: &str = String::new();
    let y = String::new();
    test(&y);
    test2(&y);
}

It prints:

sophiajt · 2016-10-29T22:39:11Z

Cool :) Looks good so far.

sophiajt · 2016-10-29T22:40:02Z

There's a span_suggestion if you wanted to play with that.

arielb1 · 2016-10-30T17:40:59Z

src/librustc_typeck/check/method/probe.rs

+                                               .map(|probe| probe.to_unadjusted_pick())
+                                               .collect();
+
+        if ret.len() < 1 {


Now that PickResult is already a Result<Vec<->, ->, I don't think we need that Option.

You're absolutely right!

sophiajt · 2016-10-31T14:25:01Z

src/librustc_typeck/check/demand.rs

+                if suggestions.len() > 0 {
+                    Some(format!("here are some functions which \
+                                  might fulfill your needs:\n - {}",
+                                 self.get_best_match(&suggestions)))


I was wondering if we should call these methods and show them like:

.len()

.capacity()

Seems actually a better idea than the current output so big 👍 .

sophiajt · 2016-10-31T14:27:29Z

src/test/compile-fail/coerce_suggestions.rs

@@ -0,0 +1,47 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Can we use ui tests instead so we know what the result will look like?

GuillaumeGomez · 2016-11-01T15:02:51Z

@jonathandturner: I moved the new test into ui, let me know if you like the new output.

r? @nikomatsakis

nikomatsakis · 2016-11-08T16:54:37Z

@GuillaumeGomez

Sorry for the delay, been a busy last few days! I'll try to finish up the review now. I just wanted to revisit the method check code one last time with a fresh pair of eyes. =)

BTW, you wrote this earlier:

This is quite limited and would only work for rustc source code and not users' code in the majority of cases.

But I'm not sure what you were responding to? Was it this comment of mine:

I do wonder if we could leverage the as_, to_, and into_ conversions here, which basically signal "safe suggestions" already.

If so, I'm surprised that you think this would only be applicable to rustc! I think maybe what you mean is that it would be helpful for making suggestions from the standard library, but not for detecting cases of safe-conversions that occur in user code? If so, I am not sure I agree, though I suppose that may be possible. Presuming that end-users follow Rust's naming conventions, relying on as etc would seem to have the advantage that existing libraries would "just work" with this suggestion scheme.

nikomatsakis · 2016-11-08T18:20:25Z

@GuillaumeGomez ok, so I've been spending some time re-reading the PR, and I feel like I'm not ready for it to land. I think we want to change the approach somewhat. There are two main concerns.

Concern 1: This makes some reasonable deep changes to the method selection code. In general, I don't think we should be changing the method lookup code quite so much to accommodate selections.

I think a better approach would be to factor out into two phases:

First, we identify the list of candidates with a suitable return type, but without doing the full probe etc. We just get back a list of steps and candidates at each step.
Once we have that list of candidates, we extract out a list of method names.
Then we run the normal method lookup completely unaltered for each method name in turn. We ignore errors and just collect the Ok results to use as suggestions.

I expect this would be more of a "surface edit" to the method lookup code, basically, only tweaking how we generate candidates.

Does that make sense? If not, I can try to prototype to show you what I mean.

Concern 2: I remain uncomfortable with the code that suggests the user add a & or &mut. I tested it locally and it definitely misfires. I think the general rule we shoot for is "first, do no harm" -- meaning, don't give misleading suggestions. Telling people to unilaterally add an &mut doesn't feel helpful to me. This code might be ok if it (at minimum) checked that the type which resulted could possibly be right, but it doesn't right now.

Here is an example interaction with your branch showing you what I mean (though it's the same example I gave before):

lunch-box. cat > ~/tmp/issue-37388.rs
fn foo(x: &i32) {
    bar(x); // don't want a hint here!
}
fn bar(x: &mut i32) {}
fn main() { }

lunch-box. rustc --stage1 ~/tmp/issue-37388.rs
error[E0308]: mismatched types
 --> /home/nmatsakis/tmp/issue-37388.rs:2:9
  |
2 |     bar(x); // don't want a hint here!
  |         ^ types differ in mutability
  |
  = note: expected type `&mut i32`
  = note:    found type `&i32`
  = help: try with `&mut x`

error: aborting due to previous error

The other concern is that the final help is kind of hard to even see!

In general, I'd prefer to factor out this "suggest a &" code into a separate PR, since it seems like a different thing.

GuillaumeGomez · 2016-11-08T20:41:37Z

@nikomatsakis: For your second concern, I think I'll just make this change in another PR, it'll get easier for everyone and I'll try to make something better. However, the only real problem with the current code is for the &mut proposition in bad cases, no? Did you find a case where I proposed a & where I shouldn't?

For your first concern, well, I'm a bit lost.

First, we identify the list of candidates with a suitable return type, but without doing the full probe etc. We just get back a list of steps and candidates at each step.

As far as I understood it, you said: "finding candidates without looking for candidates". So I certainly completely misunderstood and I really need more information. Also, a step actually correspond to only one candidate. Such a thing doesn't seem suitable for this case, right? Or maybe do you prefer to return multiple steps at once?

Once we have that list of candidates, we extract out a list of method names.

Well, since we have methods' information, I guess this part doesn't need much comment and comprehension. 😉

Then we run the normal method lookup completely unaltered for each method name in turn. We ignore errors and just collect the Ok results to use as suggestions.

Now I'm completely lost. Why would we need to run another method lookup since we already have a list of candidates?

So I think most of my questions come from my lack of knowledge but if you could bring a little more information, it'd wonderful!

Meanwhile, I'll remove the check_ref code from here and open another PR and try to work out the &mut issue.

bors · 2016-11-09T23:14:18Z

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

nikomatsakis · 2016-11-22T01:48:43Z

@GuillaumeGomez sorry for dropping off the radar last week. I've not forgotten you =) will try to get you my idea tomorrow.

GuillaumeGomez · 2016-11-22T10:30:36Z

Ok! :)

We now do two phases. First, we gather up the list of candidates with suitable return types and extract their names. Then we filter those to see which are applicable and we return that. It might be nice to do the "filter by return type" as a second step, but this is ok for now.

nikomatsakis · 2016-11-22T21:06:56Z

@GuillaumeGomez see my most recent commits for my proposed approach. The key point is that we do not change any code in the 'non-error' path (or at least, that's the goal).

nikomatsakis · 2016-11-22T21:07:07Z

@GuillaumeGomez (I pushed the commits to your branch)

sophiajt · 2016-11-22T21:07:42Z

@nikomatsakis - do you have a screenshot showing what your version looks like?

nikomatsakis · 2016-11-22T22:32:38Z

@jonathandturner I did not change the appearance of the error messages at all, just the way they are gathered up. I tested it on the one or two UI tests and it seemed to generate the same output. I also agree we can improve the formatting though!

GuillaumeGomez · 2016-11-23T10:40:58Z

@nikomatsakis: Really nice! This is way more elegant like this. I'm all for just keeping your code and merge it as is (in condition that it succeeds tests). Do you want to rebase or do I?

nikomatsakis · 2016-11-29T16:21:24Z

@GuillaumeGomez ok. Perhaps you can rebase, and also remove the code that overlaps with #37658 ?

GuillaumeGomez · 2016-11-29T20:03:30Z

@nikomatsakis: Sure. Give me a bit of time to take care of it. :)

nikomatsakis · 2016-11-29T21:35:23Z

@GuillaumeGomez ok -- I think I'll close this PR for now to clear my queue, feel free to re-open. =)

rust-highfive assigned sophiajt Oct 25, 2016

GuillaumeGomez force-pushed the cast_suggestions branch from c5fc79a to 06386ce Compare October 25, 2016 00:14

GuillaumeGomez mentioned this pull request Oct 25, 2016

Improving type mismatch errors #37384

Closed

oli-obk reviewed Oct 25, 2016

View reviewed changes

KalitaAlexey reviewed Oct 25, 2016

View reviewed changes

KalitaAlexey reviewed Oct 26, 2016

View reviewed changes

eddyb reviewed Oct 26, 2016

View reviewed changes

GuillaumeGomez commented Oct 26, 2016

View reviewed changes

GuillaumeGomez force-pushed the cast_suggestions branch 4 times, most recently from d261e3f to 7f53f51 Compare October 29, 2016 22:09

GuillaumeGomez changed the title ~~[DON'T MERGE] Start of implementation of proposal for E0308~~ Start of implementation of proposal for E0308 Oct 29, 2016

GuillaumeGomez force-pushed the cast_suggestions branch from 7f53f51 to 824292b Compare October 29, 2016 22:29

GuillaumeGomez mentioned this pull request Oct 29, 2016

Creating a safe_suggestion attribute for coerce suggestions #37474

Closed

arielb1 reviewed Oct 30, 2016

View reviewed changes

sophiajt suggested changes Oct 31, 2016

View reviewed changes

GuillaumeGomez force-pushed the cast_suggestions branch 2 times, most recently from 8087837 to d311d6c Compare November 1, 2016 15:02

rust-highfive assigned nikomatsakis and unassigned sophiajt Nov 1, 2016

GuillaumeGomez added 8 commits November 6, 2016 19:53

Return DiagnosticBuilder to add help suggestions

4658688

Move matches return type method

c87aa8e

Fix coercion ICE

19bb4ed

Create check_ref method to allow to check coercion with & types

a2cec1a

Fix invalid probe path

19fb17c

fix tests

9c63817

improve suggestions output and add ui test

82d31c4

splitting methods into smaller ones, add docs, better variable naming

a9defa8

GuillaumeGomez force-pushed the cast_suggestions branch from 6d14755 to a9defa8 Compare November 6, 2016 18:54

killercup mentioned this pull request Nov 8, 2016

Two-level type inference throws unhelpful E0308 #37632

Closed

GuillaumeGomez mentioned this pull request Nov 8, 2016

Ref suggestion #37658

Merged

alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016

nikomatsakis added 3 commits November 22, 2016 10:47

make probe_for generic over final operation

7d513b9

revert a lot of the changes to method probe internals

ef4d5e8

nikomatsakis closed this Nov 29, 2016

		@@ -0,0 +1,47 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

Start of implementation of proposal for E0308 #37388

Start of implementation of proposal for E0308 #37388

Conversation

GuillaumeGomez commented Oct 25, 2016

Choose a reason for hiding this comment

KalitaAlexey Oct 25, 2016 • edited Loading

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

nikomatsakis commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Oct 26, 2016

eddyb commented Oct 26, 2016

Choose a reason for hiding this comment

bors commented Oct 27, 2016

sophiajt commented Oct 29, 2016

GuillaumeGomez commented Oct 29, 2016

sophiajt commented Oct 29, 2016

sophiajt commented Oct 29, 2016

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

GuillaumeGomez commented Nov 1, 2016

nikomatsakis commented Nov 8, 2016

nikomatsakis commented Nov 8, 2016

GuillaumeGomez commented Nov 8, 2016 • edited Loading

bors commented Nov 9, 2016

nikomatsakis commented Nov 22, 2016

GuillaumeGomez commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016

sophiajt commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016

GuillaumeGomez commented Nov 23, 2016

nikomatsakis commented Nov 29, 2016

GuillaumeGomez commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

KalitaAlexey Oct 25, 2016 •

edited

Loading

GuillaumeGomez commented Nov 8, 2016 •

edited

Loading