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

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

Closed
Aaron1011 opened this issue Nov 2, 2020 · 6 comments
Labels
disposition-merge The FCP starter wants to merge (accept) this final-comment-period The FCP has started, most (if not all) team members are in agreement finished-final-comment-period major-change Major change proposal major-change-accepted Major change proposal that was accepted T-lang to-announce Not yet announced MCP proposals

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Nov 2, 2020

Proposal

Summary and problem statement

Some types have trivial implementations of particular traits - for example, <&T as Clone>::clone and <&T as Borrow>::borrow. These methods are useful in generic contexts, since they allow things like passing a reference to a function with a T: Clone parameter. However, directly calling one of these methods (e.g. (&NonCloneStruct).clone()) is useless. This can also lead to confusing error messages - for example, calling some_ref.to_owned() may return either a &Foo or a Foo, depending on whether the call to to_owned resolves to <&Foo as ToOwned>::to_owned or <Foo as ToOwned::to_owned

Motivation, use-cases, and solution sketches

I propose introducing a lint NOOP_METHOD_CALL (name bikesheddable), which will fire on direct calls to any 'useless' methods. Initially, it will fire on direct calls to the following methods:

  • <&T as Clone>::clone
  • <&T as Borrow>::borrow
  • <&T as Deref>::deref
  • <&T as ToOwned>::to_owned

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);
}

For now, this lint will only work for types and traits in the standard library. In the future, this could be extended to third-party code via some mechanism, allowing crates to mark methods as only being useful in generic contexts.

However, more design work will be required for such a mechanism. Method calls like <&T as ToOwned>::to_owned go through a blanket impl, so applying an attributes to a method in an impl block is not sufficient to cover all use cases. For the standard library, we can simply hard-code the desired paths, or use some other perma-unstable mechanism.

Prioritization

I believe this fits into the 'Targeted ergonomic wins and extensions' lang team priority. Anecdotally, I've seen users on the Rust discord servers accidentally call some of these methods, and get confused by the resulting error messages. A lint would point users in the right direction.

Links and related work

This was initially proposed as the compiler-team MCP rust-lang/compiler-team#375, then reworded and re-opened here.

Initial people involved

I'm planning to implement this

What happens now?

This issue is part of the experimental MCP process described in RFC 2936. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open MCPs in its weekly triage meetings. You should receive feedback within a week or two.

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.

@Aaron1011 Aaron1011 added T-lang major-change Major change proposal labels Nov 2, 2020
@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2020

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.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 9, 2020

@rustbot second

I'm a bit torn on this one. I'd like it if we developed and published clearer criteria for what sort of lints belong in rustc -- I remember that @oli-obk actually took a stab at that. I don't know that this could be a common source of bugs but certainly confusing compilation errors (I've experienced that myself on occasion...). In any case, I'd be ok seeing a PR go up, and I'd expect an FCP on that PR.

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Nov 9, 2020
@nikomatsakis
Copy link
Contributor

Discussed in meeting on 2020-11-10:

  • We realized we might want to evaluate (using crater) how much existing code would be affected, it might want to be "allow" by default if a lot of code is affected (and phased in via edition).
  • We might also want it to be "cargo fixable"
  • Question that was raised:
    • Do we have any mechanism for ensuring it is a function whose return type is the same as the self type?
      • (Answer: No, just don't put the attribute on the wrong place.)
    • Would this trigger also for Clone::clone(&x)?

@Aaron1011
Copy link
Member Author

@nikomatsakis It looks like the FCP didn't complete after 10 days

@nikomatsakis nikomatsakis added disposition-merge The FCP starter wants to merge (accept) this finished-final-comment-period major-change-accepted Major change proposal that was accepted labels Dec 15, 2020
@nikomatsakis
Copy link
Contributor

This FCP completed, I'm going to mark this as accepted and close. The expectation @Aaron1011 is that you'll go ahead and do the work to write the PR and then cc lang team before it is merged.

@rylev
Copy link
Member

rylev commented Dec 16, 2020

I talked with @Aaron1011, and I'm going to try to implement this.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 15, 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 ❤️.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 4, 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 ❤️.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue 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 issue 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 issue 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 ❤️.
eggyal pushed a commit to eggyal/copse that referenced this issue Jan 9, 2023
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 ❤️.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge The FCP starter wants to merge (accept) this final-comment-period The FCP has started, most (if not all) team members are in agreement finished-final-comment-period major-change Major change proposal major-change-accepted Major change proposal that was accepted T-lang to-announce Not yet announced MCP proposals
Projects
None yet
Development

No branches or pull requests

4 participants