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

New lint: deref coercions #7104

Open
Aaron1011 opened this issue Apr 17, 2021 · 6 comments · May be fixed by #14059
Open

New lint: deref coercions #7104

Aaron1011 opened this issue Apr 17, 2021 · 6 comments · May be fixed by #14059
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-restriction Lint: Belongs in the restriction lint group

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Apr 17, 2021

What it does

Lints on any use of deref coercions - for example, accessing a field or method through a Deref impl

Categories (optional)

  • Kind: Restriction lint

What is the advantage of the recommended code over the original code

Normally, deref coercions are a very useful part of rust, making smart points like Box, Rc, Ref, and RefMut much easier to use. However, this can be undesirable when writing unsafe code, since a non-trivial function call could be completely hidden from view. For example:

  • Unsafe code can temporarily put data structures in an unusual state, which will lead to undefined behavior if dropped (e.g. Vec::set_len). A deref coercion might cause a panic at an unexpected point (due to a panicking user-supplied Deref impl), leading to undefined behavior.
  • When mixing raw pointers and references, it's important to pay attention to the provenance of pointers. A deref coercion can hide the creation of an &self reference behind what looks like a normal field access, leading to a very subtle form of undefined behavior.

Drawbacks

This would be a fairly niche lint - unless you're writing unsafe code, there's little need to use it.

Example

fn main() {
    let a = Box::new(true);
    let b: &bool = &a;
}

Could be written as:

use std::ops::Deref;
fn main() {
    let a = Box::new(true);
    let b: &bool = (&a).deref();
}
@Aaron1011 Aaron1011 added the A-lint Area: New lints label Apr 17, 2021
@camsteffen camsteffen added good-first-issue These issues are a good way to get started with Clippy L-restriction Lint: Belongs in the restriction lint group labels May 3, 2021
@Heidar-An
Copy link

This sounds interesting, and I'd want to do it, any help on how I would do this though (sorry for naive question, this would be my first issue).

  • could I also be assigned to this?

@Centri3
Copy link
Member

Centri3 commented Jul 18, 2023

You can ping rustbot like rustbot claim (plus an @ of course)

In regards to implementing this, I'm not sure. I don't think implicit deref coercion is explicitly in the HIR (as a node) so unless there's a query for it you'd need to figure it out yourself, maybe there's some method to do that in dereference.rs (likely where you'd implement the lint). Perhaps @xFrednet (or others) have any ideas :)?

PS: FnCtxt::deref_steps looks interesting! Same with can_coerce. Haven't tested them so perhaps they won't catch this but it could be a good starting point ^^

@y21
Copy link
Member

y21 commented Jul 18, 2023

Perhaps this could use TypeckResults::expr_adjustments by checking if there are any Adjust::Derefs where the target type is different from the source type (from expr_ty without adjusts) with references removed

@Centri3
Copy link
Member

Centri3 commented Jul 18, 2023

I hadn't thought of that; that sounds perfect for this

@xFrednet
Copy link
Member

TypeckResults::expr_adjustments

You found the correct method 👍

@yegeunyang
Copy link

@rustbot claim

@yegeunyang yegeunyang linked a pull request Jan 22, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-restriction Lint: Belongs in the restriction lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants