Implement ~const Destruct effect goal in the new solver

[compiler-errors]

This also fixed a subtle bug/limitation of the NeedsConstDrop check. Specifically, the "Qualif" API basically treats const drops as totally structural, even though dropping something that has an explicit Drop implementation cannot be structurally decomposed. For example:

#![feature(const_trait_impl)]

#[const_trait] trait Foo {
    fn foo();
}

struct Conditional<T: Foo>(T);

impl Foo for () {
    fn foo() {
        println!("uh oh");
    }
}

impl<T> const Drop for Conditional<T> where T: ~const Foo {
    fn drop(&mut self) {
        T::foo();
    }
}

const FOO: () = {
    let _ = Conditional(());
    //~^ This should error.
};

fn main() {}

In this example, when checking if the Conditional(()) rvalue is const-drop, since Conditional has a const destructor, we would previously recurse into the () value and determine it has nothing to drop, which means that it is considered to not need a const drop -- even though dropping Conditional(()) would mean evaluating the destructor which relies on that T: const Foo bound to hold!

This could be fixed alternatively by banning any const conditions on const Drop impls, but that really sucks -- that means that basically no interesting const drop impls could be written. We have the capability to totally and intuitively support the right behavior, which I've implemented here.

[bors]

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

[bors]

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

[compiler-errors]

r? @lcnr

I know you don't typically review this const stuff, but it's touching the new solver so I know you may want to review this.

also cc @RalfJung: are you familiar with the existing implementation of the Qualif system in compiler/rustc_const_eval/src/check_consts/qualifs.rs? I reworked it to fix a bug in the const drop checking implementation, but I wanted to know if you had any strong opinions about the approach.

[rustbot]

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

[compiler-errors]

We're not reusing the helper from structural_traits b/c we specifically want to extract a DefId here too.

I guess alternatively we could pass back an Option<DefId> from that helper and bail if it's None (i.e. a FnPtr), then do the const check afterwards.

[compiler-errors]

I could possibly uplift this to structural traits module too, I guess

[RalfJung]

are you familiar with the existing implementation of the Qualif system in compiler/rustc_const_eval/src/check_consts/qualifs.rs?

I've seen it before but I am not very familiar with it, no...

[RalfJung]

This also fixed a subtle bug/limitation of the NeedsConstDrop check. Specifically, the "Qualif" API basically treats const drops as totally structural, even though dropping something that has an explicit Drop implementation cannot be structurally decomposed

The Qualif API lets you do structural propagation of obligations but it also lets you "overwrite" that. So this sounds like just a plain bug in how the Qualif API is used. Probably because that code was written before const Drop was a thing.

[RalfJung]

the NeedsConstDrop check

There is no such qualif? I assume you mean NeedsNonConstDrop?

[compiler-errors]

Firstly, the last commit is incomplete (needs a new feature gate). I just wanted to get it vibe-checked before fixing it up, since it's kinda tedious.

[compiler-errors]

Notably, I didn't want to recreate all of the logic having to do with conditional gating, so I create a new ConstChecker right here and immediately call check_op. That's kinda gross, I know; I guess I could alternatively abstract out the error reporting part? No strong opinion.

The point is that before this commit, I don't believe we were respecting feature gating or miri-unleashed properly with const_precise_live_drops enabled.

[RalfJung]

Yeah, this entire separate pass is kind of a hack. This seems fine. Actually ideally we'd deduplicate this entire logic (first checking qualifs.needs_drop, then checking qualifs.needs_non_const_drop, then calling check_op_spanned) -- is that reasonably possible?

[compiler-errors]

Yeah, I can deduplicate the logic.

[compiler-errors]

Also, we don't need this _post function. I added it because I wanted to assert we were not registering any secondary diagnostics on this path, but we already delay a span bug if we do.

[RalfJung]

Yeah, this entire separate pass is kind of a hack. This seems fine. Actually ideally we'd deduplicate this entire logic (first checking qualifs.needs_drop, then checking qualifs.needs_non_const_drop, then calling check_op_spanned) -- is that reasonably possible?

[RalfJung]

Firstly, the last commit is incomplete (needs a new feature gate). I just wanted to get it vibe-checked before fixing it up, since it's kinda tedious.

I like the vibes! We'll only really see whether it behaves as expected once the tests are adjusted, but the code matches what I expect. I hope it makes sense to you as well? Definitely let me know if you think I am asking for something weird here. :)

[compiler-errors]

@bors r=lcnr

[bors]

📌 Commit 69a38de has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 69a38de with merge 1dd4a02...

[rust-log-analyzer]

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

💔 Test failed - checks-actions

[compiler-errors]

spurious?

[compiler-errors]

@bors retry

this looks like some spurious miri crash ive seen in the past

[bors]

⌛ Testing commit 69a38de with merge 743003b...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 743003b to master...

[rust-timer]

Finished benchmarking commit (743003b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.0%, 1.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.0%, 1.2%]	2

Max RSS (memory usage)

Results (primary 2.1%, secondary -3.4%)

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

Cycles

Results (secondary -0.4%)

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

Binary size

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

Bootstrap: 794.975s -> 795.253s (0.03%)
Artifact size: 336.16 MiB -> 336.22 MiB (0.02%)