Implement NOOP_METHOD_CALL lint #80723

rylev · 2021-01-05T15:26:08Z

Implements the beginnings of rust-lang/lang-team#67 - a lint for detecting noop method calls (e.g, calling <&T as Clone>::clone() when T: !Clone).

This PR does not fully realize the vision and has a few limitations that need to be addressed either before merging or in subsequent PRs:

No UFCS support
The warning message is pretty plain
Doesn't work for ToOwned

The implementation uses Instance::resolve which is normally later in the compiler. It seems that there are some invariants that this function relies on that we try our best to respect. For instance, it expects substitutions to have happened, which haven't yet performed, but we check first for needs_subst to ensure we're dealing with a monomorphic type.

Thank you to @davidtwco, @Aaron1011, and @wesleywiser for helping me at various points through out this PR ❤️.

rust-highfive · 2021-01-05T15:26:11Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

davidtwco · 2021-01-05T15:30:50Z

r? @estebank since I helped a tiny bit with this :)

src/test/ui/panic-handler/weak-lang-item.stderr

estebank

Leaving one nitpick to provide a structured suggestion for one-click application on VSCode. It'll also change the output slightly to be a bit more verbose, but hopefully it'll look good.

Otherwise, r=me.

compiler/rustc_lint/src/noop_method_call.rs

src/test/ui/issues/issue-11820.rs

compiler/rustc_lint/src/noop_method_call.rs

rylev · 2021-01-10T11:08:58Z

library/core/tests/iter.rs

@@ -3509,7 +3509,7 @@ pub fn extend_for_unit() {
 #[test]
 fn test_intersperse() {
    let xs = ["a", "", "b", "c"];
-    let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();


@Aaron1011 We've already talked about this, but I am worried about calling this call a "noop" might make some users confused. The method call has the effect of dereferencing the double reference. The specific method itself does nothing but the mere act of making a method call does have an effect. This distinction is subtle, and I'm worried this may lead to situations like this one where we recommend removing the method call which actually breaks the code.

cc @estebank

I think that what could be done is check the type of expr and see if it is different to the type of the method receiver (check if one is and the other is not a borrow). The "correct" check would be to see if this is coming from a impl Clone &T, but that seems like it would be harder to implement than the proposed heuristic.

i tried to do this, but I was not ever able to resolve the return type of the method to something that could be equated to the receiver. I'm not sure how to resolve the return type of the method (which is Self) to a type that can be equated with the receiver.

What happens if you do cx.typeck_results().expr_ty(expr);? Doesn't that give you the resulting type of (in this case) x.clone()? If so, then you can compare (probably for direct equality would work) against x's type (which you already have in receiver_ty) and emit the lint of !=. Could you try that out? Otherwise we can merge without that and I can build on this. If all of this works, then we could have a MachineApplicable structured suggestion (either inline or tool only) to remove the call entirely with a high degree of confidence.

@estebank I already tried cx.typeck_results().expr_ty(expr); and it returns Self as the type in the case of Clone::clone which is not equal to the receiver type unfortunately. If you have any additional ideas, I'd be happy to try them!

Can you try calling expr_ty_adjusted (for both the receiver and expr)?

estebank

I love all the .clone() calls being removed from the repo thanks to the new lint <3

estebank · 2021-01-11T18:37:39Z

library/core/tests/iter.rs

@@ -3509,7 +3509,7 @@ pub fn extend_for_unit() {
 #[test]
 fn test_intersperse() {
    let xs = ["a", "", "b", "c"];
-    let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();


I think that what could be done is check the type of expr and see if it is different to the type of the method receiver (check if one is and the other is not a borrow). The "correct" check would be to see if this is coming from a impl Clone &T, but that seems like it would be harder to implement than the proposed heuristic.

compiler/rustc_lint/src/noop_method_call.rs

src/test/ui/lint/noop-method-call.stderr

compiler/rustc_lint/src/noop_method_call.rs

estebank · 2021-01-13T02:40:02Z

Ok, so I took a few looks and this is weird as all hell, but I think I got something that works correctly (but is ugly and brittle):

https://github.com/rust-lang/rust/compare/master...estebank:noop-lint-pass?expand=1

rylev · 2021-01-13T11:12:43Z

@estebank I don't love it because it hard codes even more specifics about the types we currently care about which will make it harder to extend, but if you think this is right move for now, I'm happy to cherry pick your commit into my branch. What do you say?

estebank · 2021-01-13T18:40:26Z

@rylev I prefer accuracy even if it adds tech debt, as long as it is called out.

I added another commit to my branch doing some code clean up to my preference and tweaked the comments. Feel free to disregard those if you disagree with it.

rylev · 2021-01-14T10:53:29Z

@estebank I've cherry-picked your changes. This looks good to me. Thanks for helping me get this over the finish line!

bors · 2021-03-04T04:48:56Z

⌛ Testing commit 25637b2 with merge 3dc624b76ef132e8e82730a067cd0d769df3837b...

camelid · 2021-03-04T04:52:26Z

If this fails like #80527 (comment) did, the tree should probably be closed until the ESLint issue is fixed.

rust-log-analyzer · 2021-03-04T05:04:52Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Removing intermediate container c74708fed8c0
 ---> 33cfa5ff01fa
Step 5/10 : RUN npm install es-check -g
 ---> Running in da09019f4908
/node-v14.4.0-linux-x64/bin/es-check -> /node-v14.4.0-linux-x64/lib/node_modules/es-check/index.js

> [email protected] postinstall /node-v14.4.0-linux-x64/lib/node_modules/es-check/node_modules/spawn-sync
> node postinstall
+ [email protected]
added 95 packages from 44 contributors in 3.898s
Removing intermediate container da09019f4908
 ---> c094750eb74b
---
Cloning into 'rust-toolstate'...
<Nothing changed>
+ es-check es5 ../src/librustdoc/html/static/main.js ../src/librustdoc/html/static/settings.js ../src/librustdoc/html/static/source-script.js ../src/librustdoc/html/static/storage.js

Cannot read property 'includes' of undefined

bors · 2021-03-04T05:07:27Z

💔 Test failed - checks-actions

camelid · 2021-03-04T05:24:59Z

Unrelated failure happened again: @bors treeclosed=100

camelid · 2021-03-04T05:26:27Z

Actually not sure if only T-infra is supposed to close the tree... sorry if I'm not supposed to that! @bors treeclosed-

JohnTitor · 2021-03-04T06:45:15Z

@bors retry

…k-Simulacrum Pin es-check version to prevent unrelated CI failures es-check v5.2.1 causes a lot of unrelated CI failures on mingw-check, e.g. rust-lang#80723 (comment). es-check v5.2.2 fixes it but let's pin its version to prevent further failures.

Implement NOOP_METHOD_CALL lint Implements the beginnings of rust-lang/lang-team#67 - a lint for detecting noop method calls (e.g, calling `<&T as Clone>::clone()` when `T: !Clone`). This PR does not fully realize the vision and has a few limitations that need to be addressed either before merging or in subsequent PRs: * [ ] No UFCS support * [ ] The warning message is pretty plain * [ ] Doesn't work for `ToOwned` The implementation uses [`Instance::resolve`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/instance/struct.Instance.html#method.resolve) which is normally later in the compiler. It seems that there are some invariants that this function relies on that we try our best to respect. For instance, it expects substitutions to have happened, which haven't yet performed, but we check first for `needs_subst` to ensure we're dealing with a monomorphic type. Thank you to `@davidtwco,` `@Aaron1011,` and `@wesleywiser` for helping me at various points through out this PR ❤️.

Implement NOOP_METHOD_CALL lint Implements the beginnings of rust-lang/lang-team#67 - a lint for detecting noop method calls (e.g, calling `<&T as Clone>::clone()` when `T: !Clone`). This PR does not fully realize the vision and has a few limitations that need to be addressed either before merging or in subsequent PRs: * [ ] No UFCS support * [ ] The warning message is pretty plain * [ ] Doesn't work for `ToOwned` The implementation uses [`Instance::resolve`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/instance/struct.Instance.html#method.resolve) which is normally later in the compiler. It seems that there are some invariants that this function relies on that we try our best to respect. For instance, it expects substitutions to have happened, which haven't yet performed, but we check first for `needs_subst` to ensure we're dealing with a monomorphic type. Thank you to ``@davidtwco,`` ``@Aaron1011,`` and ``@wesleywiser`` for helping me at various points through out this PR ❤️.

Rollup of 10 pull requests Successful merges: - rust-lang#80723 (Implement NOOP_METHOD_CALL lint) - rust-lang#80763 (resolve: Reduce scope of `pub_use_of_private_extern_crate` deprecation lint) - rust-lang#81136 (Improved IO Bytes Size Hint) - rust-lang#81939 (Add suggestion `.collect()` for iterators in iterators) - rust-lang#82289 (Fix underflow in specialized ZipImpl::size_hint) - rust-lang#82728 (Avoid unnecessary Vec construction in BufReader) - rust-lang#82764 (Add {BTreeMap,HashMap}::try_insert) - rust-lang#82770 (Add assert_matches macro.) - rust-lang#82773 (Add diagnostic item to `Default` trait) - rust-lang#82787 (Remove unused code from main.js) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned davidtwco Jan 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2021

rust-highfive assigned estebank and unassigned davidtwco Jan 5, 2021

This comment has been minimized.

Sign in to view

rylev force-pushed the noop-lint-pass branch from 0291029 to e29cefd Compare January 6, 2021 11:33

This comment has been minimized.

Sign in to view

rylev commented Jan 7, 2021

View reviewed changes

src/test/ui/panic-handler/weak-lang-item.stderr Outdated Show resolved Hide resolved

estebank approved these changes Jan 8, 2021

View reviewed changes

compiler/rustc_lint/src/noop_method_call.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/noop_method_call.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Aaron1011 reviewed Jan 8, 2021

View reviewed changes

src/test/ui/issues/issue-11820.rs Outdated Show resolved Hide resolved

Aaron1011 reviewed Jan 9, 2021

View reviewed changes

compiler/rustc_lint/src/noop_method_call.rs Outdated Show resolved Hide resolved

rylev commented Jan 10, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

estebank reviewed Jan 11, 2021

View reviewed changes

compiler/rustc_lint/src/noop_method_call.rs Outdated Show resolved Hide resolved

estebank reviewed Jan 11, 2021

View reviewed changes

src/test/ui/lint/noop-method-call.stderr Outdated Show resolved Hide resolved

compiler/rustc_lint/src/noop_method_call.rs Outdated Show resolved Hide resolved

Dylan-DPC-zz mentioned this pull request Mar 4, 2021

Rollup of 11 pull requests #82737

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 4, 2021

JohnTitor mentioned this pull request Mar 4, 2021

Pin es-check version to prevent unrelated CI failures #82747

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2021

Dylan-DPC-zz mentioned this pull request Mar 5, 2021

Rollup of 9 pull requests #82785

Closed

This was referenced Mar 5, 2021

Rollup of 15 pull requests #82794

Closed

Rollup of 10 pull requests #82795

Merged

bors merged commit e6a6df5 into rust-lang:master Mar 5, 2021

rustbot added this to the 1.52.0 milestone Mar 5, 2021

Aaron1011 mentioned this pull request Mar 13, 2021

Tracking Issue for NOOP_METHOD_CALL lint #83099

Open

4 tasks

rylev deleted the noop-lint-pass branch July 5, 2021 09:24

GoldsteinE mentioned this pull request Mar 16, 2022

ICE on stable when trying to imitate trivial_bounds #94999

Closed

oli-obk mentioned this pull request May 30, 2023

make noop_method_call warn by default #111916

Merged

kalil-pelissier mentioned this pull request Jan 26, 2024

dragonball: fix noop-method-call warning kata-containers/kata-containers#8932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement NOOP_METHOD_CALL lint #80723

Implement NOOP_METHOD_CALL lint #80723

rylev commented Jan 5, 2021 •

edited

Loading

rust-highfive commented Jan 5, 2021

davidtwco commented Jan 5, 2021

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.

estebank left a comment

This comment has been minimized.

rylev Jan 10, 2021

estebank Jan 11, 2021

rylev Jan 12, 2021

estebank Jan 12, 2021 •

edited

Loading

rylev Jan 12, 2021

estebank Jan 12, 2021

This comment has been minimized.

estebank left a comment

estebank Jan 11, 2021

estebank commented Jan 13, 2021

rylev commented Jan 13, 2021

estebank commented Jan 13, 2021

rylev commented Jan 14, 2021

bors commented Mar 4, 2021

camelid commented Mar 4, 2021

rust-log-analyzer commented Mar 4, 2021

bors commented Mar 4, 2021

camelid commented Mar 4, 2021

camelid commented Mar 4, 2021

JohnTitor commented Mar 4, 2021

Implement NOOP_METHOD_CALL lint #80723

Implement NOOP_METHOD_CALL lint #80723

Conversation

rylev commented Jan 5, 2021 • edited Loading

rust-highfive commented Jan 5, 2021

davidtwco commented Jan 5, 2021

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.

estebank left a comment

Choose a reason for hiding this comment

This comment has been minimized.

rylev Jan 10, 2021

Choose a reason for hiding this comment

estebank Jan 11, 2021

Choose a reason for hiding this comment

rylev Jan 12, 2021

Choose a reason for hiding this comment

estebank Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

rylev Jan 12, 2021

Choose a reason for hiding this comment

estebank Jan 12, 2021

Choose a reason for hiding this comment

This comment has been minimized.

estebank left a comment

Choose a reason for hiding this comment

estebank Jan 11, 2021

Choose a reason for hiding this comment

estebank commented Jan 13, 2021

rylev commented Jan 13, 2021

estebank commented Jan 13, 2021

rylev commented Jan 14, 2021

bors commented Mar 4, 2021

camelid commented Mar 4, 2021

rust-log-analyzer commented Mar 4, 2021

bors commented Mar 4, 2021

camelid commented Mar 4, 2021

camelid commented Mar 4, 2021

JohnTitor commented Mar 4, 2021

rylev commented Jan 5, 2021 •

edited

Loading

estebank Jan 12, 2021 •

edited

Loading