Make unconditional_recursion warning detect recursive drops

[Enselic]

Closes #55388

Also closes #50049 unless we want to keep it for the second example which this PR does not solve, but I think it is better to track that work in #57965.

r? @oli-obk since you are the mentor for #55388

Unresolved questions:

• There are two false positives that must be fixed before merging (see diff). I suspect the best way to solve them is to perform analysis after drop elaboration instead of before, as now, but I have not explored that any further yet. Could that be an option? Answer: Yes, that solved the problem.

@rustbot label +T-compiler +C-enhancement +A-lint

[Enselic]

I'll explore ways to fix the false positives before I send this to review. (Guidance on how to do it still appreciated.)

@rustbot author

Edit: I think I have something that works, I just need to clean up the code

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

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[Enselic]

Moving the check for drop recursion until after drop elaboration indeed fixed the false positives. So I consider this ready for review now.

We probably want to do a perf run? Probably best to wait until after at least an initial review round. Maybe we want to refactor the code to avoid rustc_mir_transform depending on rustc_mir_build? I would be happy to do that. Just let me know.

@rustbot ready

[cjgillot]

mir_drops_elaborated_and_const_checked does not run on all MIR bodies, so moving the lint there will cause it not to run. In particular in check builds on non-generic non-inline functions.

[cjgillot]

That said, I don't see how else run this lint after drop-elaboration.

[oli-obk]

Nominating for lang team discussion. This extends the unconditional_recursion lint to also detect when a Drop impl causes values of the same type getting dropped unconditionally. The one caveat being, that this is only reported in full builds, not in check builds. The existing lint is unaffected.

[Enselic]

(Just so we all are on the same page: that phenomenon is old and not introduced by this PR. See #49292)

[joshtriplett]

@rfcbot merge

[nikomatsakis]

@rfcbot fcp merge

Sounds good, starting FCP!

[nikomatsakis]

@rustbot labels -I-lang-nominated

[pnkfelix]

@rfcbot fcp merge

(see above, I'm just making it happen)

[rfcbot]

Team member @pnkfelix 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.

[pnkfelix]

(once T-lang finishes FCP, we can relabel this back to T-compiler again)

[scottmcm]

Personally, I'm happy for the lint to detect all cases of unconditional recursion, wherever or whenever, without extra lang confirmation. (I see lang as saying "yes, unconditional recursion is worth linting about, and compiler reviewers as sufficient for expanding that to catch more places of the same thing.)

The check/build difference is a shame, but false negatives are normal for lints, so I don't see it as a big issue. (Unlike for errors.)

@rfcbot reviewed

[oli-obk]

I checked niko's and josh's box as both of them tried to fcp merge this PR: #113902 (comment) and #113902 (comment)

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[oli-obk]

@bors r+

[bors]

📌 Commit b4b33df has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit b4b33df with merge 84ec263...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 84ec263 to master...

[rust-timer]

Finished benchmarking commit (84ec263): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

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%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 652.316s -> 650.87s (-0.22%)