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 lint for catching clone() auto(de)ref confusion. #11818

Closed
Kimundi opened this issue Jan 26, 2014 · 7 comments
Closed

Add a lint for catching clone() auto(de)ref confusion. #11818

Kimundi opened this issue Jan 26, 2014 · 7 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut.

Comments

@Kimundi
Copy link
Member

Kimundi commented Jan 26, 2014

Due to auto(de)ref, there are situations where calling clone() on a &T either clones the T, or the &T, depending on whether T has a Clone implementation or not.

This leads to confusing error messages in case of a missing Clone impls, especially in combination with #[deriving(Clone)].

Example

struct Foo; 
let x: Foo = (&Foo).clone();

Fails with

error: mismatched types: expected `main::Foo` but found `&main::Foo` (expected struct main::Foo but found &-ptr)

While this works:

struct Foo; 
impl Clone for Foo { fn clone(&self) -> Foo { Foo } }; 
let x: Foo = (&Foo).clone();

Lint

The proposed lint should catch situations where there are type errors arising from a clone() or deep_clone() call returning &T where T is expected, and recommend implementing Clone or DeepClone for T.

@Kimundi
Copy link
Member Author

Kimundi commented Jan 26, 2014

Something similar could also get implemented for other traits where this is a problem, like Eq, Ord, etc.

@huonw
Copy link
Member

huonw commented Jan 26, 2014

A lint won't help, lints run much after type checking.

I'm going to have another go at fixing deriving properly, so it generates sane error messages, since making compiler generated code have sane error messages via a lint is emphatically the wrong thing to do.

@thestinger
Copy link
Contributor

I don't think this is a solution. It's in no way a problem specific to Clone or a reasonable subset of traits.

@glaebhoerl
Copy link
Contributor

Is #11820 related to this?

Having different methods selected depending on whether or not a particular trait impl exists is exactly the sort of thing which should not happen. The required impl should be unambiguous and in its absence, an error should be reported. I don't have a thorough understanding of this issue. Can someone give me one, or where/how could I get one? Is there a fundamental conflict with auto(de)ref, or is it more a function of the method lookup algorithm?

@Kimundi
Copy link
Member Author

Kimundi commented Jan 28, 2014

@glaebhoerl: It is a side effect of having auto(de)ref on method call at all, and a generic impl<'a, T: Clone> Clone for &'a T { ... }.

In other words, its the method lookup algorithm doing what it should do: Pick the nearest (in terms of references to add/remove) resolving method.

@steveklabnik
Copy link
Member

Triage: nothing has changed. I share skepticism that this is super useful.

@steveklabnik
Copy link
Member

Since new lints have a big impact on users of rustc, the policy is that they should go through the RFC process like other user-facing changes. As such, I'm going to give this one a close, but if anyone comes across this ticket and wants this lint, consider adding it to clippy and/or writing up an RFC. Thanks!

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 1, 2023
[`redundant_guards`]: catch `is_empty`, `starts_with` and `ends_with` on slices and `str`s

Fixes rust-lang#11807

Few things worth mentioning:
- Taking `snippet`s is now done at callsite, instead of passing a span and doing it in `emit_redundant_guards`. This is because we now need custom suggestion strings in certain places, like `""` for `str::is_empty`.
- This now uses `snippet` instead of `snippet_with_applicability`. I don't think this really makes any difference for `MaybeIncorrect`, though?
- This could also lint byte strings, as they're of type `&[u8; N]`, but that can be ugly so I decided to leave it out for now

changelog: [`redundant_guards`]: catch `str::is_empty`, `slice::is_empty`, `slice::starts_with` and `slice::ends_with`
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.
Projects
None yet
Development

No branches or pull requests

5 participants