Suggest to use self for fake-self from other languages #54694
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
src/test/ui/self/suggest-self.rs
Outdated
|
|
||
| fn baz(&self) -> i32 { | ||
| my.bar() | ||
| //~^ ERROR cannot find value `this` in this scope |
src/librustc_resolve/lib.rs
Outdated
| span, | ||
| "do you mean", | ||
| "self".to_string(), | ||
| Applicability::MachineApplicable, |
There was a problem hiding this comment.
Hm, is this truly machine applicable in the sense that I can run cargo fix, it will apply this, and we can guarantee this fixes the code?
There was a problem hiding this comment.
I would keep this as MaybeIncorrect unless we added a check to see if the binding is available in self to begin with (which I don't see in this PR and shouldn't necessarily block it).
| |s| Ident::from_str(*s) | ||
| ).collect(); | ||
| if fake_self.contains(&item_str) | ||
| && this.self_value_is_available(path[0].span, span) { |
There was a problem hiding this comment.
Can we ever get here in case that this or my are defiend? If so, maybe we should add a check here to only suggest this when they are undefined and the author actually meant what we call self?
A test case to consider adding would be
struct Foo { x: i32 };
impl Foo {
fn foo(&mut self) -> i32 {
let this = 13;
// ... a hundred complicated lines of code ...
this.x
}
}There was a problem hiding this comment.
yes, it's covered. I tested this locally(but deleted this lines from test case):
let this = self;
this.xit works fine and doesn't trigger the err.
I'll recover this as you point here.
There was a problem hiding this comment.
This is pretty neat! Could you quickly draft out how hard it'd be to see if self.foo actually exists when the user wrote this.foo? Not only that, if that is the case then it'd be a great idea to actually accept the code (while emitting the error) so that we avoid knock down effects from the incorrect code, although it seems that the scope is already marked as TyErr, so the output already looks good enough.
r=me with the test that @killercup pointed out fixed.
src/librustc_resolve/lib.rs
Outdated
| span, | ||
| "do you mean", | ||
| "self".to_string(), | ||
| Applicability::MachineApplicable, |
There was a problem hiding this comment.
I would keep this as MaybeIncorrect unless we added a check to see if the binding is available in self to begin with (which I don't see in this PR and shouldn't necessarily block it).
| x: 2 | ||
| }; | ||
| let this = a; | ||
| this.x |
There was a problem hiding this comment.
@estebank the testcases this1 this2 passed, should it be marked as Applicable?
oh, no, as you pointed out, if it's UnApplicable with this.fo for struct T { foo: i32 }.
There was a problem hiding this comment.
@estebank I guess it won't be hard as there alread has methods like check_field
There was a problem hiding this comment.
That's great! In that case, could you make a follow up PR incorporating that change? At that point, if the check fails it might need to be a note instead of a suggestion, given that the likelihood of the suggestion (even MaybeIncorrect) plummets... I'm not sure.
There was a problem hiding this comment.
yep, added an entry to TODO.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-bors
|
@estebank sorry for rebasing PR, I misspelled |
|
@bors r=estebank |
|
📌 Commit 4470b1c has been approved by |
|
☀️ Test successful - status-appveyor, status-travis |
Closes #54019
r? @estebank