Add deref suggestion #43870

GuillaumeGomez · 2017-08-14T22:22:23Z

rust-highfive · 2017-08-14T22:22:34Z

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

durka · 2017-08-14T23:11:04Z

src/test/ui/deref-suggestion.stderr

+   |
+   = note: expected type `std::string::String`
+              found type `&std::string::String`
+   = help: try with `*&"aaa".to_owned()`


can't it check if & is already there, or is that too difficult?

Worse, the suggestion isn't even valid.

Compiling playground v0.0.1 (file:///playground) error[E0507]: cannot move out of borrowed content --> src/main.rs:4:9 | 4 | foo(*&"aaa".to_owned()); | ^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content error: aborting due to previous error(s) error: Could not compile `playground`. To learn more, run the command again with --verbose.

@durka: It's pretty simple so I'll do it.
@kennytm: Hum... I guess I'll need to add a check over the dereference...

GuillaumeGomez · 2017-08-15T12:51:21Z

src/librustc_typeck/check/demand.rs

+                        return if src.starts_with("&") {
+                            Some(format!("try with `{}`", &src[1..]))
+                        } else {
+                            Some(format!("try with `*{}`", &src))


For this case, I'd need to check if it's actually possible to deref without error but I don't know how to do it. Anyone has an idea?

@GuillaumeGomez You could check if the target type implements Copy I guess.

That's only one possibility. In the ui test I added, it's not necessary (and the String type doesn't implement the Copy trait). This solution is far from complete.

@GuillaumeGomez That's only because the & is removed altogether, allowing you as the owner to relinquish the ownership (move it into foo()).

The foo(*r_s) suggestion is still wrong, it will trigger E0507 (cannot move out of borrowed content).

That's precisely what I said:

For this case, I'd need to check if it's actually possible to deref without error but I don't know how to do it.

I think the cases are:

type is Copy -> good

value is an index operation -> bad

value is a tuple or struct field op -> OK if root value is not a reference

everything else -> ok

oli-obk · 2017-08-15T15:17:58Z

src/librustc_typeck/check/demand.rs

+                if expected_ty == &checked_ty.ty.sty {
+                    let sp = self.sess().codemap().call_span_if_macro(expr.span);
+                    if let Ok(src) = self.tcx.sess.codemap().span_to_snippet(sp) {
+                        return if src.starts_with("&") {


You should probably destructure expr to figure out if it's an ExprRef. This way you can obtain the inner item directly. Your current code would suggest mut String::new() for foo(&mut String::new()).

Oh damn, excellent point!

oli-obk · 2017-08-15T21:11:25Z

src/librustc_typeck/check/demand.rs

@@ -239,6 +239,19 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                }
                None
            }
+            (expected_ty, &ty::TyRef(_, checked_ty)) => {
+                if expected_ty == &checked_ty.ty.sty {


I think it would be cool if this would infinitely deref, so we'd get suggestions for &&T when T was expected, or even &str when starting out with a String.

I can extract the "real" type through all references. I'll do it next.

using == is a bit strict. We might want to use instead infcx.can_sub(check_ty, expected_ty).

arielb1 · 2017-08-15T21:59:28Z

r? @eddyb - not sure how much we want this.

eddyb · 2017-08-16T08:46:16Z

I do not want to review any more suggestion hacks. r? @nikomatsakis

aidanhs · 2017-08-24T00:05:17Z

I've pinged @nikomatsakis on IRC about this PR.

oli-obk · 2017-08-24T07:27:37Z

src/librustc_typeck/check/demand.rs

+            (expected_ty, &ty::TyRef(_, checked_ty)) => {
+                if expected_ty == &checked_ty.ty.sty {
+                    let sp = self.sess().codemap().call_span_if_macro(expr.span);
+                    if let Ok(src) = self.tcx.sess.codemap().span_to_snippet(sp) {


I think processing the snippets is really fragile. Imo it would be much cleaner to destructure expr and check for ExprAddrOf

Good point!

oli-obk · 2017-08-24T07:28:03Z

src/test/ui/deref-suggestion.stderr

+   |
+   = note: expected type `std::string::String`
+              found type `&std::string::String`
+   = help: try with `*r_s`


The suggestion is still wrong. I think this should only trigger for expressions that produce a temporary or Copy types. Everything else should be handled by the method suggesting code that gets triggered on e.g. let u: usize = ""; producing a suggesting for .len().

nikomatsakis · 2017-08-24T12:45:03Z

src/test/ui/deref-suggestion.stderr

+   |
+   = note: expected type `std::string::String`
+              found type `&std::string::String`
+   = help: try with `*r_s`


so I think that the overall code is fine -- if a bit restrictive -- but this is kind of a goofy suggestion, right? i.e., it's going to lead to a move error. Perhaps we should check if the type is copy before offering it? You can do this via: infcx.type_moves_by_default(self.param_env, ty, span)

I wanted to but was looking for the function to do so. Thanks!

nikomatsakis

Looks like a good start. Sorry for the delay @GuillaumeGomez

arielb1 · 2017-08-29T18:11:09Z

I think @nikomatsakis wants the Copy fix?

GuillaumeGomez · 2017-08-29T19:20:35Z

Yes yes, still in my todo list.

GuillaumeGomez · 2017-09-02T18:44:55Z

src/librustc_typeck/check/demand.rs

+                            }
+                        }
+                        _ => {
+                            if self.infcx.type_moves_by_default(self.param_env,


@nikomatsakis: This call is never returning true so I suppose I'm misusing it. At first, I checked on checked_ty directly but then I thought that the fact it was a reference might make it not work. Then I tried on the lower level (which is the current code) but still can't make it work. Any idea what I'm doing wrong?

I think you want !self.infcx.type_moves_by_default -- that is, this function returns false is the value is Copy (and hence it does not move).

nikomatsakis · 2017-09-05T16:17:44Z

src/librustc_typeck/check/demand.rs

+                            }
+                        }
+                        _ => {
+                            if self.infcx.type_moves_by_default(self.param_env,


I think you want !self.infcx.type_moves_by_default -- that is, this function returns false is the value is Copy (and hence it does not move).

GuillaumeGomez · 2017-09-09T14:06:15Z

Updated.

@nikomatsakis: You were right: it works just as expected now! :)

nikomatsakis · 2017-09-11T21:35:30Z

src/test/ui/deref-suggestion.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+/*fn foo(_: String) {}


Is this meant to be commented out?

nikomatsakis · 2017-09-11T21:35:50Z

src/test/ui/deref-suggestion.rs

+fn main() {
+    let s = String::new();
+    let r_s = &s;
+    foo(r_s);


can you add some comments explaining the purpose of each of these random foo functions?

e.g. "// Check that we do not suggest *r_s when the argument type is not copy".

nikomatsakis · 2017-09-11T21:49:44Z

src/test/ui/deref-suggestion.rs

+    foo(r_s);
+    foo(&"aaa".to_owned());
+    foo(&mut "aaa".to_owned());
+    foo4(&0);


I saw some logic around macros -- can we embed this in a macro and see what happens?

e.g. foo(borrow!(0)) where:

macro_rules! borrow { ($x:expr) => { &$x } }

GuillaumeGomez · 2017-09-12T18:46:15Z

@nikomatsakis: The error with the macro is funny:

error[E0308]: mismatched types
  --> src/test/ui/deref-suggestion.rs:12:20
   |
12 |     ($x:expr) => { &$x }
   |                    ^^^ expected u32, found &{integer}
...
32 |     foo3(borrow!(0));
   |          ---------- in this macro invocation
   |
   = note: expected type `u32`
              found type `&{integer}`
   = help: try with `0`

error: aborting due to 5 previous errors

nikomatsakis · 2017-09-12T20:42:25Z

src/librustc_typeck/check/demand.rs

+                    match expr.node {
+                        hir::ExprAddrOf(_, ref expr) => {
+                            if let Ok(code) = self.tcx.sess.codemap().span_to_snippet(expr.span) {
+                                return Some(format!("try with `{}`", code));


I guess @GuillaumeGomez that this particular macro is triggering this case. Try having the macro produce {&x} for example -- although this is also an interesting data point.

arielb1 · 2017-09-19T20:43:21Z

What's the status of this PR? Are you blocked on something @GuillaumeGomez? Just a friendly ping to make sure this isn't getting lost.

GuillaumeGomez · 2017-09-20T11:18:12Z

I'm still trying to figure out how I can "fix" the problem when there are macros.

oli-obk · 2017-09-20T20:48:57Z

You can only produce the suggestion if span.ctxt().outer().expn_info().is_none(). Around macros it is nigh impossible to create sensible suggestions

nikomatsakis · 2017-09-21T13:24:16Z

Yeah, I also had in mind just disabling around macros. (Thanks @oli-obk, I wasn't sure the best way to check for that.)

GuillaumeGomez · 2017-09-21T17:49:03Z

Ah perfect. I'll update soon then. Thanks @oli-obk!

GuillaumeGomez · 2017-09-21T21:30:56Z

Updated. Now, when the issue is in a macro, the help isn't displayed anymore.

nikomatsakis · 2017-09-22T14:35:00Z

src/test/ui/deref-suggestion.stderr

+           - .escape_default()
+           - .escape_unicode()
+           - .to_lowercase()
+           - .to_uppercase()


Unrelated, but we really have to do something here. These suggestions are pretty silly.

nikomatsakis · 2017-09-22T14:39:18Z

I added some comments explaining the code a bit. Looks good to me now! Thanks @GuillaumeGomez

nikomatsakis · 2017-09-22T14:39:31Z

@bors r+

bors · 2017-09-22T14:39:32Z

📌 Commit 2f52282 has been approved by nikomatsakis

bors · 2017-09-22T17:17:34Z

⌛ Testing commit 2f52282d2267730d6e6ce506ee0dab528de0e33d with merge 11ffb6801bb2aed4986ac17f013153593d7bb3cf...

bors · 2017-09-22T17:24:25Z

💔 Test failed - status-appveyor

kennytm · 2017-09-22T17:34:38Z

src/librustc_typeck/check/demand.rs

+                                return Some(format!("try with `{}`", code));
+                            }
+                        }
+


tidy error: C:\projects\rust\src\librustc_typeck\check\demand.rs:281: trailing whitespace

Ah damn, thanks!

GuillaumeGomez · 2017-09-23T11:17:31Z

I fixed the tidy issue so let's start it over!

@bors: r=nikomatsakis

bors · 2017-09-23T11:17:32Z

📌 Commit 21d4ba2 has been approved by nikomatsakis

bors · 2017-09-23T13:13:21Z

⌛ Testing commit 21d4ba2 with merge a83c3e7...

Add deref suggestion Fixes #34562.

bors · 2017-09-23T16:48:03Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a83c3e7 to master...

rust-highfive assigned pnkfelix Aug 14, 2017

durka reviewed Aug 14, 2017

View reviewed changes

GuillaumeGomez force-pushed the deref-suggestion branch from c18bcc5 to b841a43 Compare August 15, 2017 12:50

GuillaumeGomez commented Aug 15, 2017

View reviewed changes

oli-obk reviewed Aug 15, 2017

View reviewed changes

GuillaumeGomez force-pushed the deref-suggestion branch from b841a43 to 2bfbd86 Compare August 15, 2017 21:43

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2017

rust-highfive assigned nikomatsakis and unassigned pnkfelix Aug 16, 2017

oli-obk requested changes Aug 24, 2017

View reviewed changes

nikomatsakis reviewed Aug 24, 2017

View reviewed changes

nikomatsakis requested changes Aug 24, 2017

View reviewed changes

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 Aug 28, 2017

GuillaumeGomez force-pushed the deref-suggestion branch from 2bfbd86 to b8559b1 Compare September 2, 2017 18:42

GuillaumeGomez commented Sep 2, 2017

View reviewed changes

nikomatsakis requested changes Sep 5, 2017

View reviewed changes

GuillaumeGomez force-pushed the deref-suggestion branch from b8559b1 to 6db9332 Compare September 9, 2017 14:05

nikomatsakis requested changes Sep 11, 2017

View reviewed changes

GuillaumeGomez force-pushed the deref-suggestion branch from 6db9332 to 201a56a Compare September 12, 2017 18:45

nikomatsakis reviewed Sep 12, 2017

View reviewed changes

Add deref suggestion

c30435b

GuillaumeGomez force-pushed the deref-suggestion branch from 201a56a to c30435b Compare September 21, 2017 21:30

nikomatsakis reviewed Sep 22, 2017

View reviewed changes

nikomatsakis approved these changes Sep 22, 2017

View reviewed changes

kennytm reviewed Sep 22, 2017

View reviewed changes

add some comments

21d4ba2

GuillaumeGomez force-pushed the deref-suggestion branch from 2f52282 to 21d4ba2 Compare September 23, 2017 11:16

bors added a commit that referenced this pull request Sep 23, 2017

Auto merge of #43870 - GuillaumeGomez:deref-suggestion, r=nikomatsakis

a83c3e7

Add deref suggestion Fixes #34562.

bors merged commit 21d4ba2 into rust-lang:master Sep 23, 2017

GuillaumeGomez deleted the deref-suggestion branch September 23, 2017 22:38

Add deref suggestion #43870

Add deref suggestion #43870

Uh oh!

Conversation

GuillaumeGomez commented Aug 14, 2017

Uh oh!

rust-highfive commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennytm Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennytm Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Aug 15, 2017

Uh oh!

eddyb commented Aug 16, 2017

Uh oh!

aidanhs commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Aug 29, 2017

Uh oh!

GuillaumeGomez commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Sep 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kennytm Aug 15, 2017 •

edited

Loading

GuillaumeGomez Aug 15, 2017 •

edited

Loading

kennytm Aug 15, 2017 •

edited

Loading

nikomatsakis Aug 24, 2017 •

edited

Loading