Unnecessary references lint

[obeis]

View all comments

Close #127724

[rustbot]

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[compiler-errors]

This lint seems really specific to a single kind of expression, and there are plenty of other cases where unnecessary references are created when the user is trying to create a raw pointer. Unless this can be greatly generalized, it doesn't really seem worth adding this.

[compiler-errors]

r? RalfJung

[obeis]

I agree, and my intention is to generalize this lint to cover all unnecessarily created references. Could you please list other cases that you think should be included? I can update this PR to cover them or create an issue to track these cases and mention that they should be added to the unnecessary_refs lint.

[RalfJung]

Sorry, I am swamped. Happy to discuss which cases the lint shoild fire on, but I can't review the implementation. r? compiler

[SparrowLii]

I know little about lints' impls so r? compiler

[RalfJung]

Do we have some people who are our "linting experts"?

[RalfJung]

This lint seems really specific to a single kind of expression, and there are plenty of other cases where unnecessary references are created when the user is trying to create a raw pointer. Unless this can be greatly generalized, it doesn't really seem worth adding this.

Which examples did you have in mind?

A starting point might be to uplift https://rust-lang.github.io/rust-clippy/master/index.html#borrow_as_ptr from cliippy to rustc, as you mention. It is not clear to me what the difference is between that lint and this one.

[traviscross]

Do we have some people who are our "linting experts"?

cc @Urgau

[Urgau]

Happy to take over the review. If @Nadrieril doesn't want to review it of course.

As for the lint it-self, I join @RalfJung that this is lint is currently clippy::borrow_as_ptr, which will need to be dropped from clippy. Look at 1fee1a4 for the changes needed.

As a drive-by, rustc_hir_pretty should not be necessary, a multi-part suggestion should be used instead, with some span manipulation to get the correct spans.

[Nadrieril]

Much appreciated :) r? @Urgau

[obeis]

@RalfJung I agree, let's land this as allow-by-default for now.

[traviscross]

Makes sense to me. Thanks @obeis and @RalfJung.

Lint details

This is an allow-by-default lint:

unnecessary-refs

The unnecessary_refs lint checks for unnecessary references.

Example

#![warn(unnecessary_refs)]

fn via_ref(x: *const (i32, i32)) -> *const i32 {
    unsafe { &(*x).0 as *const i32 }
}

fn main() {
    let x = (0, 1);
    let _r = via_ref(&x);
}

This will produce:

warning: creating an intermediate reference implies aliasing requirements even when immediately casting to raw pointers
 --> lint_example.rs:4:14
  |
4 |     unsafe { &(*x).0 as *const i32 }
  |              ^^^^^^^^^^^^^^^^^^^^^
  |
note: the lint level is defined here
 --> lint_example.rs:1:9
  |
1 | #![warn(unnecessary_refs)]
  |         ^^^^^^^^^^^^^^^^
help: consider using `&raw const` for a safer and more explicit raw pointer
  |
4 -     unsafe { &(*x).0 as *const i32 }
4 +     unsafe { &raw const (*x).0 }
  |

Explanation

Creating unnecessary references is discouraged because it can reduce readability, introduce performance overhead, and lead to undefined behavior if the reference is unaligned or uninitialized. Avoiding them ensures safer and more efficient code.

This lint is proposed at allow-by-default so it can land while we adjust the standard library and gauge effects on the ecosystem.

@rfcbot fcp merge lang

---

In terms of the current implementation, I understand the following to be true.

fn redundant_block(x: u8) {
    let _x = { &x } as *const u8;
}

It doesn't fire for the above, but should.

fn vec_index_deref(x: *mut Vec<u8>) -> *mut u8 {
    unsafe { &mut (*x)[0] as *mut u8 }
}

It fires for this one, but should not, because the place isn't reachable through built-in projections and so a reference is materialized in MIR regardless. After applying the lint's suggestion, the MIR is the same, and the dangerous_implicit_autorefs lint still fires.

fn vec_index(v: &mut Vec<u8>) -> *mut u8 {
    &mut v[0] as *mut u8
}

Similar to the above, the lint fires and applying the suggestion results in code that produces the same MIR. (In this case, though, dangerous_implicit_autorefs doesn't fire.)

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• name-and-scope (Unnecessary references lint #138230 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[ehuss]

I'm having a little bit of difficulty following the conversation and understanding what the current implementation does. I can't find a consolidated description.

Does the current implementation only check for &x as *const _? That is, is it exactly the same as clippy::borrow_as_ptr?

Is there an intention for this lint to cover any situation where a borrow is "unnecessary"? For example, would it also encompass clippy::needless_borrow in the future? I ask because:

• The current description doesn't say what the current limitations are, or what the future intentions are.
• needless_borrow can be a little controversial. (It seems from comments here, even borrow_as_ptr is a little controversial, though seems less so.)
• The lint name can be confusing depending on what the intentions are here.

[RalfJung]

Replacing &mut x as *mut _ by &raw mut x can avoid UB caused by the unnecessary intermediate reference (and similar for shared references). The goal of this lint is to tell people about such unnecessary intermediate references and nudge them towards staying in raw pointer land.

unsafe { &mut (*x)[n] as *mut u8 } (for x: Vec) is an interesting case indeed... the raw pointer version of this is something like x.as_mut_ptr().add(n) except that removes the bounds check. So currently we don't have a good replacement for that instance, and I tend to agree we should then not lint.

This sounds like borrow_as_ptr indeed, though I don't know the details of that lint.

Is there an intention for this lint to cover any situation where a borrow is "unnecessary"?

Not from my side, no. It is specifically about borrows that are unnecessary where removing them actually can make a semantic difference (which I think can only happen if removing the borrow entirely avoids creating any reference, using only raw pointers instead).

[traviscross]

@rfcbot concern name-and-scope

@ehuss asked:

Is there an intention for this lint to cover any situation where a borrow is "unnecessary"? For example, would it also encompass clippy::needless_borrow in the future? I ask because:

• The current description doesn't say what the current limitations are, or what the future intentions are.
• needless_borrow can be a little controversial. (It seems from comments here, even borrow_as_ptr is a little controversial, though seems less so.)
• The lint name can be confusing depending on what the intentions are here.

@RalfJung's reply was:

Is there an intention for this lint to cover any situation where a borrow is "unnecessary"?

Not from my side, no. It is specifically about borrows that are unnecessary where removing them actually can make a semantic difference (which I think can only happen if removing the borrow entirely avoids creating any reference, using only raw pointers instead).

If we mean to limit the scope of the lint in this way, probably the current name is a bit too encompassing. Do we have any other ideas for that? As @ehuss notes, it'd be good to include these intended scope restrictions in the lint documentation as well.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

I'm afraid I don't have the capacity to do code-level review here.

[rust-bors]

☔ The latest upstream changes (presumably #157575) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[Urgau]

Unless I'm missing something, this check doesn't take into account temporaries, which contrary to & don't work with &raw const.

Playground:

error[E0745]: cannot take address of a temporary
 --> src/main.rs:3:24
  |
3 |     let a = &raw const 1;
  |                        ^ temporary value

View changes since the review

[Urgau]

Could you add more tests with temporaries, multiple casts, inner blocks, ...

View changes since the review