Initial impl of raw_assign_to_drop

[lukaslueg]

Fixes #4294

The lint simply checks all assignments via unsafe pointers where to dereferenced-type has drop-glue.

I'm somewhat unsure about what to call this thing - is it assign_raw_ptr_using_drop, raw_assign_drop, dropped_assign_raw, ... ?

Although some of the tests involve &mut as *mut, the lint does not make efforts to filter out situations where the raw pointer is derived from a known-safe source. The general assumption is that if we have a raw pointer at at all, and assign to the place behind the pointer, then all safety bets are off anyway (otherwise, one could have assigned via &mut to begin with).

changelog: [raw_assign_to_drop]: Initial impl

[rustbot]

r? @Centri3

rustbot has assigned @Centri3.
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

[Centri3]

Looks fine to me beyond the suggestion nit

[Centri3]

FCP: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.3A.20.60raw_assign_to_drop.60

[kpreid]

Right now, this text does not clearly acknowledge that “the value is always initialized (and so the code is correct)” is a possible case. I worry that people will take it as “plain assignments are always bad style”, and possibly rewrite correct code into code that forgets instead of dropping (or add needless oldvalue_is_initialized flags, or just take away “sheesh, Rust has things that are broken by default”). I think it would be significantly better if the text and examples gives a concrete recommendation for the case where the pointer is initialized.

The obvious concrete recommendation to make is “allow/expect the lint”, but in some cases, converting back to safe &mut could be better, allowing the unsafe to be more narrowly scoped — I’m particularly thinking of cases where raw pointers are being used to perform borrow splitting that can’t be checked by the borrow checker, where it makes sense to convert to &mut as soon as the splitting is done, but before the actual writes happen.

[y21]

My initial impression when I read through this and #4294 was that the recommended way to resolve this is to simply always use ptr::write() (+ ptr::drop_in_place if what the user wrote is really what they want - the user can then addditonally justify that dropping is indeed the correct thing to do by writing a safety comment which can be enforced by other lints, and is then no longer hidden behind the = operator). So if the value is initialized, write ptr.drop_in_place(); ptr.write(value);.

This is also how I understood the help messages:

   = help: use `std::ptr::write()` to overwrite a (possibly uninitialized) place
   = help: use `std::ptr::drop_in_place()` to drop the previous value if such value exists

I feel like recommending to allow the lint in the case that the code is correct goes against what the help messages are saying?

[kpreid]

I have two thoughts on this, which are:

1. Verbosity is not necessarily clarity. Splitting a single assignment into separate drop_in_place() and write() calls:

   • Creates a hazard, in that if they end up with some other code inserted in between them, that code is executing with a sort of broken invariant that might be overlooked:

     ptr::drop_in_place(self.ptr);
     ptr::write(self.ptr, self.compute_new_value()); // compute_new_value could see a deinited *self.ptr!

   • Makes it look like something more esoteric than an ordinary Rust assignment to an initialized place in safe code.

   I could get behind “you should convert the pointer to &mut before assigning”, which makes it clear that the invariants of &mut (initialization, exclusive access) apply, but “implement the two parts of assignment yourself just so you’re being explicit” feels like imposing a lot of mental cost for the sake of “code that is doing something else than what this code is doing might be doing something wrong”.

2. If this lint is proposing never assigning through a raw pointer, that feels rather like it is saying “don't use this part of the language at all!” recommendation, which — if Clippy is doing something like that outside of restriction lints, then it feels like something has gone wrong at the language-design level. Rust shouldn’t have (non-deprecated) features that are always wrong to use.

   And perhaps this feature should be deprecated, but in that case, it's time to talk to T-lang, not just add a lint.

[lukaslueg]

Reworked the description to possibly address the points made here.

The example now no longer uses "2-phase replace", and the description hopefully provides better insights into when to use what.

The lint no longer fires when assigning to raw pointers derived from UnsafeCell::get() directly, which we assume to be safely handled; lintcheck went from +10 to +4 due to that.

[lukaslueg]

How about

/// If the code can guarantee that the value being replaced during assignment is initialized
/// in all cases, it is recommended to
/// * silence this warning by adding a `#[expect(raw_assign_to_drop)]`-attribute
/// * make liberal use of SAFETY-comments e.g. around declaration of this unsafe pointer,
///   reminding readers that this unsafe pointer *must* contain an initialized value.

[Centri3]

Based off of the new points in the FCP, I believe my main thoughts are unfortunately that this lint shouldn't be added. I think this comment by llogiq humorously sums it up.

I won't close this yet, and I do believe the problem it's solving is important!, but I don't think a warn-by-default lint is the right way about it. Let us know anything you think!

_{The longer this thread goes on, the less confident I am in my ability to write sound code...}

[Jarcho]

Ping @lukaslueg from triage. Do you plan to return to working on this?

Going by the zulip thread this would have to be a restriction lint. If there were were an alternative to suggest that had identical behaviour things would be different, but that's not a thing that clippy can change.

[lukaslueg]

afaics the discussion reached an impasse:

• It is agreed upon that the situation this lint is aiming for is bad.
• If we can guarantee that *ptr is initialized, the "two-phase assignment" is essentially only there to silence this lint. This obfuscates a perfectly valid one-liner *ptr = ..., with no upside.
• The two-phase assignment may introduce subtleties one might reasonably argue are even worse to reason about; the lint can warn about that, but it can't reason about that.
• There is little this lint can do to avoid noise, as every raw assignment of a T: Drop is susceptible to UB. The advertised remedies are obfuscation, and sprinkling allow everywhere.
• This is also bad.

The two possible ways forward that I can see is to either close this PR; or make it allow-by-default, so people can enable it either periodically as a review guide or on a not-while-you-put-your-feet-under-my-table-young-man basis.

[Jarcho]

That's basically what I took out of the zulip thread. As things currently stand the lint needs to be in the restriction category.

[lukaslueg]

I can move it over and adjust the docs accordingly, if thats worth the effort

[rustbot]

This PR was rebased onto a different master 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.

[lukaslueg]

Rebased. Moved over to restriction, changed the language so its more appropriate for a restriction lint, the "two-phase assignment" is no longer advertised.

The obvious downside is that the only way to escape this allow-by-default lint is to selectively allow each site. One might argue this is acceptable for a restriction lint, whose main purpose is to alert people to the underlying problem.

@rustbot label -S-waiting-on-author +S-waiting-on-review

[github-actions]

Lintcheck changes for a9453af

Lint	Added	Removed	Changed
clippy::raw_assign_to_drop	4	0	0

This comment will be updated if you push new changes

[lukaslueg]

Addendum: One thought is to actually look at the content of the SAFETY-comment, and silence to lint on keywords (think: if there is a unsafe block for the assignment, and there is SAFETY-comment for that block, and the comment contains "initialization", this lint is silenced). Such an approach, which might be applicable for other uses, goes beyond establishing the lint imho.

[Jarcho]

r? Jarcho

[rustbot]

☔ The latest upstream changes (possibly #15979) made this pull request unmergeable. Please resolve the merge conflicts.