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

[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.

[RalfJung]

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

[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.

[Urgau]

Impl looks good to me.

View changes since this review