Uplift clippy::undropped_manually_drops lint

[Urgau]

This PR aims at uplifting the clippy::undropped_manually_drops lint.

undropped_manually_drops

(deny-by-default)

The undropped_manually_drops lint check for calls to std::mem::drop with a value of std::mem::ManuallyDrop which doesn't drop.

Example

struct S;
drop(std::mem::ManuallyDrop::new(S));

Explanation

ManuallyDrop does not drop it's inner value so calling std::mem::drop will not drop the inner value of the ManuallyDrop either.

---

Mostly followed the instructions for uplifting an clippy lint described here: #99696 (review)

@rustbot label: +I-lang-nominated
r? compiler

---

For Clippy:

changelog: Moves: Uplifted clippy::undropped_manually_drops into rustc

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[asquared31415]

ManuallyDrop doesn't not drop it's inner value

I think this part of the description is incorrect? Should it be "ManuallyDrop does not drop its inner value ..."?

[tmandry]

@rfcbot fcp merge

[rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

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.

[scottmcm]

Didn't I see some lints go by recently that lint for using drop on things that are Copy? Might it make sense for this to be just part of that lint? Something like "promised it won't be needs_drop" instead of Copy?

Or if this is worth adding separately, maybe it would make sense to give it a higher default level as a "this is a specific mistake that people make" deny lint rather than a "this is useless" warn lint?

[Urgau]

Didn't I see some lints go by recently that lint for using drop on things that are Copy?

Yes, it was #109732

Might it make sense for this to be just part of that lint? Something like "promised it won't be needs_drop" instead of Copy?

Well, it uplifted 4 lints, clippy::{drop,forget}_{ref,copy}. They could maybe be merged into one or two lint(s) if desired. I could see a reason for grouping by method but don't think this lint should be part of any of them since undropped_manually_drops detects obvious mistake while the other aren't not at the same level of "obvious mistake" (dropping a ref isn't wrong it's just useless, dropping a ManuallyDrop is useless but also surely a mistake).

Or if this is worth adding separately, maybe it would make sense to give it a higher default level as a "this is a specific mistake that people make" deny lint rather than a "this is useless" warn lint?

Yes, sure. My take on the default level was "does it cause UB? yes -> deny; no -> warn" but since this lint catch's a so obvious mistake that no-one would have ever wrote this intentionally, it could make sense to make it deny-by-default.

[scottmcm]

Yeah, we have a couple of other deny-by-default lints like that. For example,

• unused binding from match pattern: warning
• unused binding from match pattern, but spelled exactly the same as a variant of the type it's matching against: deny (and suggest replacing A => with Foo::A => )

When there's a super-specific suggestion we can give (here to use ManuallyDrop::drop instead of mem::drop) because it's a known pattern, I think deny-by-default makes good sense.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Urgau]

Yeah, we have a couple of other deny-by-default lints like that.

[..]

When there's a super-specific suggestion we can give (here to use ManuallyDrop::drop instead of mem::drop) because it's a known pattern, I think deny-by-default makes good sense.

Make sense, I will change it to deny-by-default.

(btw, the suggestion here is ManuallyDrop::into_inner not ManuallyDrop::drop as the lint only triggers on owned ManuallyDrop, the ref cases and handled by the other lints)

[bors]

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

[nikomatsakis]

@rustbot label -I-lang-nominated

[Urgau]

(delegate still works even though bors didn't comment, just use @bors r=compiler-errors when this is ready to go)

unfortunately it won't work since you have used @rustbot and not @bors (unless I'm missing something)

@rustbot ready

[GuillaumeGomez]

@compiler-errors I think it was supposed to be done using bors and not rustbot. ;)

@bors r=compiler-errors

[bors]

📌 Commit d9d1c76 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit d9d1c76 with merge d7ad9d9...

[compiler-errors]

🤦

Whoops, sorry, I mixed "compiler-errors makes a typo" up with the actual "bors delegate doesn't tell you the delegate succeeded" bug (https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20ignoring.20.60delegate.2B.60.3F) -- thx @GuillaumeGomez

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing d7ad9d9 to master...

[rust-timer]

Finished benchmarking commit (d7ad9d9): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.6%, 0.7%]	3
Regressions ❌ (secondary)	0.5%	[0.3%, 0.6%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.6%, 0.7%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[1.9%, 2.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.29s -> 649.935s (0.41%)

[pnkfelix]

• only primary benchmark to regress was helloworld (3 incr check variants), and not by all that much (relatively speaking)
• secondary regressions were solely to unify-linearly, await-call-tree, token-stream-stress.
• impact seems acceptable (one expects new lint to add some amount of extra work, and I wouldn't be surprised if this is actually noise,. given Misc HIR typeck type mismatch tweaks #112116 above).
• marking as triaged.

@rustbot label: +perf-regression-triaged