Implement `~const Destruct` effect goals in the new solver #132329

compiler-errors · 2024-10-29T20:16:05Z

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 · 2024-10-31T13:49:13Z

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

bors · 2024-11-03T23:09:10Z

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

compiler-errors · 2024-11-08T19:27:13Z

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 · 2024-11-08T19:27:16Z

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

compiler-errors · 2024-11-08T20:27:26Z

compiler/rustc_next_trait_solver/src/solve/effect_goals.rs

+        let cx = ecx.cx();
+
+        let self_ty = goal.predicate.self_ty();
+        let (tupled_inputs_and_output, def_id, args) = match self_ty.kind() {


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 · 2024-11-08T20:57:05Z

compiler/rustc_next_trait_solver/src/solve/effect_goals.rs

+
+        let self_ty = goal.predicate.self_ty();
+
+        let const_conditions = match self_ty.kind() {


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

RalfJung · 2024-11-09T08:19:01Z

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 · 2024-11-09T08:20:47Z

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.

compiler/rustc_const_eval/src/check_consts/qualifs.rs

RalfJung · 2024-11-09T08:25:00Z

the NeedsConstDrop check

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

compiler/rustc_const_eval/src/check_consts/qualifs.rs

compiler-errors · 2024-11-12T21:09:36Z

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 · 2024-11-12T21:12:24Z

compiler/rustc_const_eval/src/check_consts/post_drop_elaboration.rs

+                // I know it's not great to be creating a new const checker, but I'd
+                // rather use it so we can deduplicate the error emitting logic that
+                // it contains.
+                Checker::new(self.ccx).check_op_spanned_post(


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.

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?

Yeah, I can deduplicate the logic.

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.

compiler/rustc_const_eval/src/check_consts/qualifs.rs

compiler/rustc_const_eval/src/check_consts/check.rs

RalfJung · 2024-11-14T06:38:24Z

compiler/rustc_const_eval/src/check_consts/post_drop_elaboration.rs

+                // I know it's not great to be creating a new const checker, but I'd
+                // rather use it so we can deduplicate the error emitting logic that
+                // it contains.
+                Checker::new(self.ccx).check_op_spanned_post(


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/rustc_const_eval/src/check_consts/qualifs.rs

RalfJung · 2024-11-14T06:43:25Z

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/rustc_const_eval/src/check_consts/check.rs

compiler/rustc_const_eval/src/check_consts/post_drop_elaboration.rs

Implement `~const Fn` trait goal in the new solver Split out from rust-lang#132329 since this should be easier to review on its own. r? lcnr

Rollup merge of rust-lang#133216 - compiler-errors:const-fn, r=lcnr Implement `~const Fn` trait goal in the new solver Split out from rust-lang#132329 since this should be easier to review on its own. r? lcnr

bors · 2024-11-20T12:39:30Z

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

compiler-errors · 2024-11-21T01:37:09Z

@RalfJung, are you satisfied with the changes to const qualif checking? @lcnr, could you take a look at the trait solver changes?

Implement `~const Fn` trait goal in the new solver Split out from rust-lang/rust#132329 since this should be easier to review on its own. r? lcnr

RalfJung

I have no idea if that new trait query in NeedsNonConstDrop makes any sense, but the rest looks good, modulo minor comments. :)

compiler/rustc_const_eval/src/check_consts/check.rs

RalfJung · 2024-11-21T07:04:43Z

compiler/rustc_const_eval/src/check_consts/check.rs

-                let ty_of_dropped_place = dropped_place.ty(self.body, self.tcx).ty;
-
-                let ty_needs_non_const_drop =
-                    qualifs::NeedsNonConstDrop::in_any_value_of_ty(self.ccx, ty_of_dropped_place);


Seems like previously, we always checked in_any_value_of_ty before checking the qualifs. Any idea why we did that? Your new logic does not do this any more. Might just be a perf thing?

Yeah, I think maybe perf? I'll check perf anyways, but I expect it to not matter.

compiler/rustc_const_eval/src/check_consts/post_drop_elaboration.rs

compiler/rustc_const_eval/src/check_consts/qualifs.rs

bors · 2024-11-21T11:03:51Z

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

…live_drops post-drop-elaboration check

compiler-errors · 2024-11-21T18:12:05Z

@bors try @rust-timer queue

…try> Implement `~const Destruct` effect goals in the new solver 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: ```rust #![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 · 2024-11-21T18:13:23Z

⌛ Trying commit 4f732ba with merge 433af0d5ded13567dc2c496bead363e60bd753e2...

bors · 2024-11-21T19:57:37Z

☀️ Try build successful - checks-actions
Build commit: 433af0d (433af0d5ded13567dc2c496bead363e60bd753e2)

rust-timer · 2024-11-21T21:34:16Z

Finished benchmarking commit (433af0d): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.2%, 3.4%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (primary 3.5%, secondary 2.9%)

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

Binary size

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

Bootstrap: 796.819s -> 795.165s (-0.21%)
Artifact size: 336.01 MiB -> 336.12 MiB (0.03%)

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Oct 29, 2024

compiler-errors force-pushed the fn-and-destruct branch from 861399d to 242dd91 Compare October 31, 2024 17:53

compiler-errors force-pushed the fn-and-destruct branch 2 times, most recently from e0730c9 to 313877d Compare November 8, 2024 19:24

compiler-errors marked this pull request as ready for review November 8, 2024 19:27

rustbot assigned lcnr Nov 8, 2024

compiler-errors commented Nov 8, 2024

View reviewed changes

RalfJung reviewed Nov 9, 2024

View reviewed changes

compiler/rustc_const_eval/src/check_consts/qualifs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 9, 2024

View reviewed changes

compiler/rustc_const_eval/src/check_consts/qualifs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 9, 2024

View reviewed changes

compiler-errors force-pushed the fn-and-destruct branch 2 times, most recently from 8ceb8c2 to 6ab6f74 Compare November 12, 2024 21:09

compiler-errors commented Nov 12, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung reviewed Nov 14, 2024

View reviewed changes

compiler/rustc_const_eval/src/check_consts/qualifs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 14, 2024

View reviewed changes

compiler-errors mentioned this pull request Nov 19, 2024

Tracking Issue for const_destruct #133214

Open

3 tasks

compiler-errors force-pushed the fn-and-destruct branch from 6ab6f74 to 6884fb4 Compare November 19, 2024 17:25

compiler-errors mentioned this pull request Nov 19, 2024

Implement ~const Fn trait goal in the new solver #133216

Merged

RalfJung reviewed Nov 19, 2024

View reviewed changes

compiler/rustc_const_eval/src/check_consts/check.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 19, 2024

View reviewed changes

compiler/rustc_const_eval/src/check_consts/post_drop_elaboration.rs Show resolved Hide resolved

compiler-errors force-pushed the fn-and-destruct branch from 6884fb4 to 882ebe7 Compare November 19, 2024 17:51

compiler-errors force-pushed the fn-and-destruct branch from 882ebe7 to 0dfaf2f Compare November 20, 2024 19:00

compiler-errors changed the title ~~Implement ~const Fn and ~const Destruct effect goals in the new solver~~ Implement ~const Destruct effect goals in the new solver Nov 21, 2024

RalfJung reviewed Nov 21, 2024

View reviewed changes

compiler-errors added 4 commits November 21, 2024 18:11

Implement ~const Destruct in new solver

8a3d095

More comments, reverse polarity of structural check

13214e4

Gate const drop behind const_destruct feature, and fix const_precise_…

39d29cb

…live_drops post-drop-elaboration check

Deduplicate checking drop terminator

4f732ba

compiler-errors force-pushed the fn-and-destruct branch from 0dfaf2f to 4f732ba Compare November 21, 2024 18:11

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 21, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `~const Destruct` effect goals in the new solver #132329

Implement `~const Destruct` effect goals in the new solver #132329

compiler-errors commented Oct 29, 2024 •

edited

Loading

bors commented Oct 31, 2024

bors commented Nov 3, 2024

compiler-errors commented Nov 8, 2024 •

edited

Loading

rustbot commented Nov 8, 2024

compiler-errors Nov 8, 2024

compiler-errors Nov 8, 2024

RalfJung commented Nov 9, 2024

RalfJung commented Nov 9, 2024

RalfJung commented Nov 9, 2024

compiler-errors commented Nov 12, 2024

compiler-errors Nov 12, 2024

RalfJung Nov 14, 2024 •

edited

Loading

compiler-errors Nov 19, 2024

compiler-errors Nov 19, 2024

This comment has been minimized.

RalfJung Nov 14, 2024 •

edited

Loading

RalfJung commented Nov 14, 2024

bors commented Nov 20, 2024

compiler-errors commented Nov 21, 2024

RalfJung left a comment

RalfJung Nov 21, 2024

compiler-errors Nov 21, 2024

bors commented Nov 21, 2024

compiler-errors commented Nov 21, 2024

This comment has been minimized.

bors commented Nov 21, 2024

bors commented Nov 21, 2024

This comment has been minimized.

rust-timer commented Nov 21, 2024


		let self_ty = goal.predicate.self_ty();

		let const_conditions = match self_ty.kind() {

Implement ~const Destruct effect goals in the new solver #132329

Are you sure you want to change the base?

Implement ~const Destruct effect goals in the new solver #132329

Conversation

compiler-errors commented Oct 29, 2024 • edited Loading

bors commented Oct 31, 2024

bors commented Nov 3, 2024

compiler-errors commented Nov 8, 2024 • edited Loading

rustbot commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 9, 2024

RalfJung commented Nov 9, 2024

RalfJung commented Nov 9, 2024

compiler-errors commented Nov 12, 2024

Choose a reason for hiding this comment

RalfJung Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Nov 14, 2024

bors commented Nov 20, 2024

compiler-errors commented Nov 21, 2024

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 21, 2024

compiler-errors commented Nov 21, 2024

This comment has been minimized.

bors commented Nov 21, 2024

bors commented Nov 21, 2024

This comment has been minimized.

rust-timer commented Nov 21, 2024

Overall result: ❌ regressions - please read the text below

Implement `~const Destruct` effect goals in the new solver #132329

Implement `~const Destruct` effect goals in the new solver #132329

compiler-errors commented Oct 29, 2024 •

edited

Loading

compiler-errors commented Nov 8, 2024 •

edited

Loading

RalfJung Nov 14, 2024 •

edited

Loading

RalfJung Nov 14, 2024 •

edited

Loading