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 #375

Closed
1 of 3 tasks
Aaron1011 opened this issue Oct 21, 2020 · 2 comments
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@Aaron1011
Copy link
Member

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:

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

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. rust-lang/rust#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:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

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.

@Aaron1011 Aaron1011 added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Oct 21, 2020
@rustbot
Copy link
Collaborator

rustbot commented Oct 21, 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.

@wesleywiser
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jan 23, 2021
@apiraino apiraino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Feb 4, 2021
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Feb 4, 2021
@apiraino apiraino closed this as completed Feb 4, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants