Fix automatic suggestion on `use_self`. #3679

daxpedda · 2019-01-20T14:10:50Z

In an example like this:

impl Example {
    fn fun_1() { }
    fn fun_2() {
        Example::fun_1();
    }
}

Clippy tries to replace Example::fun_1 with Self, loosing ::fun_1 in the process, it should rather try to replace Example with Self.

Question

There may be other paths that need the same treatment, but I'm not sure I understand them fully:

rust-clippy/clippy_lints/src/use_self.rs

Lines 94 to 96 in e648adf

if !is_self_ty {

span_use_self_lint(self.cx, path);

}

rust-clippy/clippy_lints/src/use_self.rs

Lines 225 to 229 in e648adf

    
           } else if let Def::StructCtor(ctor_did, CtorKind::Fn) = path.def { 
        
               if self.item_path.def.opt_def_id() == self.cx.tcx.parent_def_id(ctor_did) { 
        
                   span_use_self_lint(self.cx, path); 
        
               } 
        
           }

detrumi · 2019-01-20T16:19:23Z

clippy_lints/src/use_self.rs

        if path.segments.last().expect(SEGMENTS_MSG).ident.name != SelfUpper.name() {
            if self.item_path.def == path.def {
-                span_use_self_lint(self.cx, path);
+                span_use_self_lint(self.cx, path.segments.first().expect(SEGMENTS_MSG).ident.span);


The first segment might not be the type, but a module instead. So this would replace the module name, instead of the type. Playground example

rust-clippy/clippy_lints/src/use_self.rs

Lines 59 to 63 in 2e0977f

// path segments only include actual path, no methods or fields

let last_path_span = path.segments.last().expect(SEGMENTS_MSG).ident.span;

// `to()` doesn't shorten span, so we shorten it with `until(..)`

// and then include it with `to(..)`

let span = path.span.until(last_path_span).to(last_path_span);

Not sure if putting it in span_use_self_lint is the best idea.
If there is any cleaner way to clip path, any guidance would be appreciated.
I also added tests to check for this, any better suggestions on naming or more tests would be nice.

detrumi · 2019-01-20T16:23:49Z

I think you can do the same thing for those two other paths.
Also, in case this fixes all wrong suggestions in the test file, you can add // run-rustfix to the top of the test file and add the .fixed file.

flip1995

This LGTM, just the span trimming could be improved a bit

And does adding // run-rustfix to the top of the test file work?

clippy_lints/src/use_self.rs

Co-Authored-By: daxpedda <[email protected]>

daxpedda · 2019-01-22T14:17:58Z

And does adding // run-rustfix to the top of the test file work?

Added it now.

oli-obk · 2019-01-22T14:32:34Z

@bors r+

Thanks!

bors · 2019-01-22T14:32:35Z

📌 Commit 42d5a07 has been approved by oli-obk

bors · 2019-01-22T15:20:00Z

⌛ Testing commit 42d5a07 with merge b0b6134...

Fix automatic suggestion on `use_self`. In an example like this: ```rust impl Example { fn fun_1() { } fn fun_2() { Example::fun_1(); } } ``` Clippy tries to replace `Example::fun_1` with `Self`, loosing `::fun_1` in the process, it should rather try to replace `Example` with `Self`. **Question** - There may be other paths that need the same treatment, but I'm not sure I understand them fully: - https://github.com/rust-lang/rust-clippy/blob/e648adf0866a1cea7db6ce2d33ea86e442f25377/clippy_lints/src/use_self.rs#L94-L96 - https://github.com/rust-lang/rust-clippy/blob/e648adf0866a1cea7db6ce2d33ea86e442f25377/clippy_lints/src/use_self.rs#L225-L229

bors · 2019-01-22T15:30:24Z

💔 Test failed - checks-travis

flip1995 · 2019-01-22T15:42:00Z

@bors rollup

Fix automatic suggestion on `use_self`. In an example like this: ```rust impl Example { fn fun_1() { } fn fun_2() { Example::fun_1(); } } ``` Clippy tries to replace `Example::fun_1` with `Self`, loosing `::fun_1` in the process, it should rather try to replace `Example` with `Self`. **Question** - There may be other paths that need the same treatment, but I'm not sure I understand them fully: - https://github.com/rust-lang/rust-clippy/blob/e648adf0866a1cea7db6ce2d33ea86e442f25377/clippy_lints/src/use_self.rs#L94-L96 - https://github.com/rust-lang/rust-clippy/blob/e648adf0866a1cea7db6ce2d33ea86e442f25377/clippy_lints/src/use_self.rs#L225-L229

@ghost

Rollup of 4 pull requests Successful merges: - #3582 (Add assert(true) and assert(false) lints) - #3679 (Fix automatic suggestion on `use_self`.) - #3684 ("make_return" and "blockify" convenience methods, fixes #3683) - #3685 (Rustup) Failed merges: r? @ghost

bors · 2019-01-22T17:46:35Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: "make_return" and "blockify" convenience methods, fixes #3683 #3684

bors · 2019-01-22T17:46:36Z

📌 Commit 42d5a07 has been approved by phansch

bors · 2019-01-22T17:46:41Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: "make_return" and "blockify" convenience methods, fixes #3683 #3684

bors · 2019-01-22T17:46:41Z

📌 Commit 42d5a07 has been approved by phansch

bors · 2019-01-22T19:18:45Z

⌛ Testing commit 42d5a07 with merge a40d8e4...

Fix automatic suggestion on `use_self`. In an example like this: ```rust impl Example { fn fun_1() { } fn fun_2() { Example::fun_1(); } } ``` Clippy tries to replace `Example::fun_1` with `Self`, loosing `::fun_1` in the process, it should rather try to replace `Example` with `Self`. **Question** - There may be other paths that need the same treatment, but I'm not sure I understand them fully: - https://github.com/rust-lang/rust-clippy/blob/e648adf0866a1cea7db6ce2d33ea86e442f25377/clippy_lints/src/use_self.rs#L94-L96 - https://github.com/rust-lang/rust-clippy/blob/e648adf0866a1cea7db6ce2d33ea86e442f25377/clippy_lints/src/use_self.rs#L225-L229

bors · 2019-01-22T19:47:04Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing a40d8e4 to master...

Fix automatic suggestion on use_self.

13b5ea4

detrumi reviewed Jan 20, 2019

View reviewed changes

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jan 21, 2019

Fixed potential mistakes with nesting. Added tests.

2e0977f

flip1995 reviewed Jan 22, 2019

View reviewed changes

clippy_lints/src/use_self.rs Outdated Show resolved Hide resolved

flip1995 and others added 2 commits January 22, 2019 14:43

Improve span shortening.

e70f945

Co-Authored-By: daxpedda <[email protected]>

Added rustfix to the test.

e6f2239

Improving comments.

42d5a07

flip1995 mentioned this pull request Jan 22, 2019

Rollup of 4 pull requests #3686

Closed

bors merged commit 42d5a07 into rust-lang:master Jan 22, 2019

	} else if let Def::StructCtor(ctor_did, CtorKind::Fn) = path.def {
	if self.item_path.def.opt_def_id() == self.cx.tcx.parent_def_id(ctor_did) {
	span_use_self_lint(self.cx, path);
	}
	}

	// path segments only include actual path, no methods or fields
	let last_path_span = path.segments.last().expect(SEGMENTS_MSG).ident.span;
	// `to()` doesn't shorten span, so we shorten it with `until(..)`
	// and then include it with `to(..)`
	let span = path.span.until(last_path_span).to(last_path_span);

Fix automatic suggestion on use_self. #3679

Fix automatic suggestion on use_self. #3679

Uh oh!

Conversation

daxpedda commented Jan 20, 2019

Uh oh!

detrumi Jan 20, 2019

Choose a reason for hiding this comment

Uh oh!

daxpedda Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

detrumi commented Jan 20, 2019

Uh oh!

flip1995 left a comment • 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!

Uh oh!

daxpedda commented Jan 22, 2019

Uh oh!

oli-obk commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

flip1995 commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix automatic suggestion on `use_self`. #3679

Fix automatic suggestion on `use_self`. #3679

flip1995 left a comment •

edited

Loading