New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest to use self for fake-self from other languages #54694

Merged
merged 4 commits into from Oct 2, 2018

Conversation

Projects
None yet
5 participants
@csmoe
Member

csmoe commented Sep 30, 2018

Closes #54019
r? @estebank

@rust-highfive

This comment was marked as resolved.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 30, 2018

Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:50:33] .................................................................................................... 3300/4314
[00:50:36] .................................................................................................... 3400/4314
[00:50:39] .................................................................................................... 3500/4314
[00:50:43] .................................i.................................................................. 3600/4314
[00:50:47] .............................................................F...................................... 3700/4314
[00:50:54] ...................................................................................................i 3900/4314
[00:50:57] .................................................................................................... 4000/4314
[00:51:01] .................................................................................................... 4100/4314
[00:51:03] .................................................................................................... 4200/4314
[00:51:03] .................................................................................................... 4200/4314
[00:51:06] 
[00:51:06] error: /checkout/src/test/ui/self/suggest-self.rs:27: expected error not found: cannot find value `this` in this scope
[00:51:06] error: 1 unexpected errors found, 1 expected errors not found
[00:51:06] status: exit code: 1
[00:51:06] status: exit code: 1
[00:51:06] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/self/suggest-self.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/self/suggest-self/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/self/suggest-self/auxiliary" "-A" "unused"
[00:51:06]     Error {
[00:51:06]         line_num: 27,
[00:51:06]         kind: Some(
[00:51:06]             Error
[00:51:06]             Error
[00:51:06]         ),
[00:51:06]         msg: "27:9: 27:11: cannot find value `my` in this scope [E0425]"
[00:51:06] ]
[00:51:06] 
[00:51:06] not found errors (from test file): [
[00:51:06]     Error {
[00:51:06]     Error {
[00:51:06]         line_num: 27,
[00:51:06]         kind: Some(
[00:51:06]             Error
[00:51:06]         ),
[00:51:06]         msg: "cannot find value `this` in this scope"
[00:51:06] ]
[00:51:06] 
[00:51:06] thread '[ui] ui/self/suggest-self.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1341:13
[00:51:06] note: Run with `RUST_BACKTRACE=1` for a backtrace.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Collaborator

rust-highfive commented Sep 30, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:50:33] .................................................................................................... 3300/4314
[00:50:36] .................................................................................................... 3400/4314
[00:50:39] .................................................................................................... 3500/4314
[00:50:43] .................................i.................................................................. 3600/4314
[00:50:47] .............................................................F...................................... 3700/4314
[00:50:54] ...................................................................................................i 3900/4314
[00:50:57] .................................................................................................... 4000/4314
[00:51:01] .................................................................................................... 4100/4314
[00:51:03] .................................................................................................... 4200/4314
[00:51:03] .................................................................................................... 4200/4314
[00:51:06] 
[00:51:06] error: /checkout/src/test/ui/self/suggest-self.rs:27: expected error not found: cannot find value `this` in this scope
[00:51:06] error: 1 unexpected errors found, 1 expected errors not found
[00:51:06] status: exit code: 1
[00:51:06] status: exit code: 1
[00:51:06] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/self/suggest-self.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/self/suggest-self/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/self/suggest-self/auxiliary" "-A" "unused"
[00:51:06]     Error {
[00:51:06]         line_num: 27,
[00:51:06]         kind: Some(
[00:51:06]             Error
[00:51:06]             Error
[00:51:06]         ),
[00:51:06]         msg: "27:9: 27:11: cannot find value `my` in this scope [E0425]"
[00:51:06] ]
[00:51:06] 
[00:51:06] not found errors (from test file): [
[00:51:06]     Error {
[00:51:06]     Error {
[00:51:06]         line_num: 27,
[00:51:06]         kind: Some(
[00:51:06]             Error
[00:51:06]         ),
[00:51:06]         msg: "cannot find value `this` in this scope"
[00:51:06] ]
[00:51:06] 
[00:51:06] thread '[ui] ui/self/suggest-self.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1341:13
[00:51:06] note: Run with `RUST_BACKTRACE=1` for a backtrace.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Show outdated Hide outdated src/test/ui/self/suggest-self.rs Outdated
Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
|s| Ident::from_str(*s)
).collect();
if fake_self.contains(&item_str)
&& this.self_value_is_available(path[0].span, span) {

This comment has been minimized.

@killercup

killercup Sep 30, 2018

Member

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
    }
}
@killercup

killercup Sep 30, 2018

Member

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
    }
}

This comment has been minimized.

@csmoe

csmoe Sep 30, 2018

Member

yes, it's covered. I tested this locally(but deleted this lines from test case):

let this = self;
this.x

it works fine and doesn't trigger the err.
I'll recover this as you point here.

@csmoe

csmoe Sep 30, 2018

Member

yes, it's covered. I tested this locally(but deleted this lines from test case):

let this = self;
this.x

it works fine and doesn't trigger the err.
I'll recover this as you point here.

@estebank

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.

Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
x: 2
};
let this = a;
this.x

This comment has been minimized.

@csmoe

csmoe Oct 1, 2018

Member

@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 }.

@csmoe

csmoe Oct 1, 2018

Member

@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 }.

This comment has been minimized.

@csmoe

csmoe Oct 1, 2018

Member

@estebank I guess it won't be hard as there alread has methods like check_field

@csmoe

csmoe Oct 1, 2018

Member

@estebank I guess it won't be hard as there alread has methods like check_field

This comment has been minimized.

@estebank

estebank Oct 1, 2018

Contributor

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.

@estebank

estebank Oct 1, 2018

Contributor

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.

This comment has been minimized.

@csmoe

csmoe Oct 1, 2018

Member

yep, added an entry to TODO.

@csmoe

csmoe Oct 1, 2018

Member

yep, added an entry to TODO.

@csmoe

This comment was marked as resolved.

Show comment
Hide comment
@csmoe
Member

csmoe commented Oct 1, 2018

@bors

This comment was marked as resolved.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

@csmoe: 🔑 Insufficient privileges: Not in reviewers

Contributor

bors commented Oct 1, 2018

@csmoe: 🔑 Insufficient privileges: Not in reviewers

@csmoe

This comment was marked as resolved.

Show comment
Hide comment
@csmoe

csmoe Oct 1, 2018

Member

@bors r=estebank

Member

csmoe commented Oct 1, 2018

@bors r=estebank

@bors

This comment was marked as resolved.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

@csmoe: 🔑 Insufficient privileges: Not in reviewers

Contributor

bors commented Oct 1, 2018

@csmoe: 🔑 Insufficient privileges: Not in reviewers

@rust-lang rust-lang deleted a comment from bors Oct 1, 2018

@estebank

This comment was marked as resolved.

Show comment
Hide comment
@estebank

estebank Oct 1, 2018

Contributor

@bors r+

Contributor

estebank commented Oct 1, 2018

@bors r+

@bors

This comment was marked as resolved.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

📌 Commit a2e7c31 has been approved by estebank

Contributor

bors commented Oct 1, 2018

📌 Commit a2e7c31 has been approved by estebank

@csmoe

This comment has been minimized.

Show comment
Hide comment
@csmoe

csmoe Oct 1, 2018

Member

@estebank sorry for rebasing PR, I misspelled did you mean as do you mean. latest commits fixed that.

Member

csmoe commented Oct 1, 2018

@estebank sorry for rebasing PR, I misspelled did you mean as do you mean. latest commits fixed that.

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Oct 1, 2018

Member

@bors r=estebank

Member

killercup commented Oct 1, 2018

@bors r=estebank

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

📌 Commit 4470b1c has been approved by estebank

Contributor

bors commented Oct 1, 2018

📌 Commit 4470b1c has been approved by estebank

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 2, 2018

Contributor

⌛️ Testing commit 4470b1c with merge 2d1065b...

Contributor

bors commented Oct 2, 2018

⌛️ Testing commit 4470b1c with merge 2d1065b...

bors added a commit that referenced this pull request Oct 2, 2018

Auto merge of #54694 - csmoe:self_this, r=estebank
Suggest to use self for fake-self from other languages

Closes #54019
r? @estebank
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 2, 2018

Contributor

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

Contributor

bors commented Oct 2, 2018

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

@bors bors merged commit 4470b1c into rust-lang:master Oct 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@csmoe csmoe deleted the csmoe:self_this branch Oct 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment