Implement NOOP_METHOD_CALL lint

[rylev]

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]

r? @davidtwco

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

[davidtwco]

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

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.

@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)?

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

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.

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

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

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

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

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

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

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

💔 Test failed - checks-actions

Unrelated failure happened again: @bors treeclosed=100

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

@bors retry