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

[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

[RalfJung]

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.

Something like refs_that_immediately_decay_to_a_raw_ptr?
I'm open to suggestions.

[WorldSEnder]

How about roundabout_addr_of or convoluted_addr_of? Meaning it is clear that the intention was obviously to write an addr_of, but it's done in an unnecessarily complicated (UB provoking) way.

I realise that this drops the fact that the current checks only for creating one through a decayed reference, but perhaps there are future variants of the lint (especially thinking of array slicing) that check for other intermediates with the same purpose.

[kpreid]

Surely the code of the standard library itself is the least relevant thing when considering churn, since the lint could be #![allow]ed there (as a much smaller change than this PR, even)? Shouldn’t it say something more like (from unreachable-pub):

Suggested change

	/// This lint is "allow" by default because promoting it would require a
	/// large amount of churn across the standard library to remove the many
	/// unnecessary references it currently creates.
	/// This lint is "allow" by default because it will trigger for a large
	/// amount of existing Rust code.
	/// Eventually it is desired for this to become warn-by-default.

View changes since the review

[kpreid]

This is mixing singular and plural.

Suggested change

	"creating an intermediate reference implies aliasing requirements even when immediately casting to raw pointers"
	"creating an intermediate reference implies aliasing requirements even when immediately cast to a raw pointer"

View changes since the review

[kpreid]

References can coerce to raw pointers, so this example could be simplified:

Suggested change

	/// unsafe { &(*x).0 as *const i32 }
	/// unsafe { &(*x).0 }

I think this is beneficial because it helps show just how tiny the piece of code that undesirably introduces a reference can be.

---

Also, I notice that this function as written is not just dubious but unsound: it is UB if x is null, unaligned, aliased with &mut, or not readable. Most examples of allow-by-default lints are going to be much more “acceptable but not ideal” code, so it might be worth calling out this one in particular as unsound. (On the other hand, that’s a distraction from the typical case where this lint applies.)

View changes since the review

[RalfJung]

Yeah would be better to find an example where the raw pointer version is actually correct.

[kpreid]

This description seems to me to be in an inconsistent middle ground of explaining reference validity: it is mentioning the requirement of alignment, but not others such as being non-null. I think it should focus on the aliasing constraint alone (with a link to the Reference for other validity rules), because most code that runs but has unobvious problems (and thus needs the attention of this lint) is going to be using an aligned, readable, non-null pointer, but neglecting aliasing.

View changes since the review

[kpreid]

For a name, how about raw_borrow_via_ref, meaning “you presumably wanted to achieve a raw borrow, but (unnecessarily) involved a reference”?

[traviscross]

For a name, how about raw_borrow_via_ref, meaning “you presumably wanted to achieve a raw borrow, but (unnecessarily) involved a reference”?

Works for me, in the plural (per RFC 0344): raw_borrows_via_refs.

(And thanks to @RalfJung and @WorldSEnder for the other good suggestions above.)

[Urgau]

Should probably be raw_borrows_via_references, to be consistent with dropping_references, forgetting_references and function_item_references lints.

[traviscross]

Should probably be raw_borrows_via_references, to be consistent with dropping_references, forgetting_references and function_item_references lints.

+1.