rust-lang / compiler-team

A home for compiler team planning documents, meeting minutes, and other such things.
https://rust-lang.github.io/compiler-team/
Apache License 2.0
387 stars 67 forks source link

Add a `NOOP_METHOD_CALL` lint for methods which should never be directly called #375

Closed Aaron1011 closed 3 years ago

Aaron1011 commented 4 years ago

Proposal

Add a new lint NOOP_METHOD_CALL, which fires on calls to methods which are known to do nothing. To start with, we would lint calls to the following methods:

These trait impls are useful in generic code (e.g. you pass an &T to a function expecting a Clone argument), but are pointless when called directly (e.g. &bool::clone(&true)).

Note that we will intentionally not perform any kind of post-monomorphization checks. This lint will only fire on calls that are known to have the proper receiver (&T) at the call site (where the user could just remove the call).

For example

struct Foo;

fn clone_it<T: Clone>(val: T) -> T {
    val.clone() // No warning - we don't know if `T` is `&T`
}

fn main() {
    let val = &Foo;
    val.clone(); // WARNING: noop method call
    clone_it(val);
}

This could be implemented via a new unstable #[noop] attribute for most methods. <&T as ToOwned>::to_owned() goes through a blanket impl, so we might need to hard-code it in some way. In the future, this could be stabilized and made available to user crates in some way.

Background

The immutable reference type &T implements several traits (such as Clone) with a no-op implementation that just returns the original reference. These trait impls are useful for working with generic code (you can pass an &T to a function expecting a Clone argument), but explicitly calling the method (e.g. &bool::clone(&true)) is pointless.

Due to the way that method resolution works, calling (for example) string.clone() on a reference may either invoke clone on the pointee type (if the pointee type implements Clone), or invoke clone on the reference type itself. This can lead to confusing error messages (e.g. https://github.com/rust-lang/rust/issues/78187), where a non-local change (adding/removing a trait impl) can make a normal-looking method call appear to return the wrong type.

Mentors or Reviewers

I'm planning to implement this if the MCP is accepted.

Process

The main points of the Major Change Process is as follows:

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

rustbot commented 4 years ago

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

wesleywiser commented 3 years ago

@rustbot second