Skip to content
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

Implement NOOP_METHOD_CALL lint #80723

Merged
merged 20 commits into from
Mar 5, 2021
Merged

Implement NOOP_METHOD_CALL lint #80723

merged 20 commits into from
Mar 5, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Jan 5, 2021

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
Copy link
Collaborator

r? @davidtwco

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

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

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

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

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
compiler/rustc_lint/src/noop_method_call.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@@ -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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@estebank estebank Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@estebank
Copy link
Contributor

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
Copy link
Member Author

rylev commented Jan 13, 2021

@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
Copy link
Contributor

@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
Copy link
Member Author

rylev commented Jan 14, 2021

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

@bors
Copy link
Contributor

bors commented Mar 4, 2021

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

@camelid
Copy link
Member

camelid commented Mar 4, 2021

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

@rust-log-analyzer
Copy link
Collaborator

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

> spawn-sync@1.0.15 postinstall /node-v14.4.0-linux-x64/lib/node_modules/es-check/node_modules/spawn-sync
> node postinstall
+ es-check@5.2.1
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
Copy link
Contributor

bors commented Mar 4, 2021

💔 Test failed - checks-actions

@bors 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
@camelid
Copy link
Member

camelid commented Mar 4, 2021

Unrelated failure happened again: @bors treeclosed=100

@camelid
Copy link
Member

camelid commented Mar 4, 2021

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

@JohnTitor
Copy link
Member

@bors retry

@bors 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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2021
…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.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 5, 2021
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 ❤️.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 5, 2021
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 ❤️.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2021
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
@bors bors merged commit e6a6df5 into rust-lang:master Mar 5, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 5, 2021
@rylev rylev deleted the noop-lint-pass branch July 5, 2021 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.