Double-check conditional constness in MIR #132276

compiler-errors · 2024-10-28T17:17:57Z

To prevent any unchecked ~const bounds from leaking through during MIR lowering.

If this check fails, it will eventually just delay a bug, but for now it reports errors. That error reporting may be redundant if we're calling it from code that already doesn't allow ~const (i.e. when the effects and const_trait_impl gates are disabled), but I don't think it's that big of a deal.

edit: This also makes sure that we issue a const stability error if we encounter any function with const conditions when const_trait_impl is not enabled. This ensures that that feature remains airtight.

rustbot · 2024-10-28T17:18:06Z

r? @lcnr

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

compiler-errors · 2024-10-28T17:18:16Z

r? fee1-dead

fee1-dead · 2024-10-29T06:24:35Z

tests/ui/traits/const-traits/cross-crate.stocknc.stderr

+error[E0277]: the trait bound `cross_crate::NonConst: ~const cross_crate::MyTrait` is not satisfied
+  --> $DIR/cross-crate.rs:19:5
+   |
+LL |     NonConst.func();
+   |     ^^^^^^^^^^^^^^^
+


note that when we constify libcore in the future, this change may be leaking out unstable details (the ~const syntax) on stable code

I'll make this fall back to a regular "cannot call in const contexts" error if the const_trait_impl feature gate is not enabled.

bors · 2024-10-30T11:21:04Z

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

compiler-errors · 2024-10-30T16:59:11Z

tests/ui/traits/const-traits/cross-crate.stocknc.stderr

@@ -1,3 +1,15 @@
+error[E0015]: cannot call non-const fn `<cross_crate::NonConst as cross_crate::MyTrait>::func` in constant functions


Duplicated error is NBD imo.

compiler-errors · 2024-10-30T18:31:55Z

compiler/rustc_const_eval/src/check_consts/check.rs

+            } else if tcx.features().const_trait_impl() {
+                infcx.err_ctxt().report_fulfillment_errors(errors);
+            } else {
+                self.check_op(ops::FnCallNonConst {


I'm actually leaning towards a different choice here.

If const_trait_impl is not enabled, instead of selecting these const conditions and erroring if they fail, we should probably just reject calling any functions with nontrivial const conditions. I kinda don't want to expose calling, e.g., free functions with ~const bounds on stable.

fee1-dead

lgtm

compiler-errors · 2024-10-31T15:58:27Z

Let's test perf

@bors try @rust-timer queue

fee1-dead · 2024-10-31T15:59:19Z

r=me if perf shows nothing significant

…=<try> Double-check conditional constness in MIR To prevent any unchecked `~const` bounds from leaking through during MIR lowering. If this check fails, it will eventually just delay a bug, but for now it reports errors. That error reporting may be redundant if we're calling it from code that already doesn't allow `~const` (i.e. when the `effects` and `const_trait_impl` gates are disabled), but I don't think it's that big of a deal. edit: This also makes sure that we issue a const stability error if we encounter *any* function with const conditions when `const_trait_impl` is not enabled. This ensures that that feature remains airtight.

bors · 2024-10-31T15:59:42Z

⌛ Trying commit 314b383 with merge 7e97174...

bors · 2024-10-31T16:18:01Z

💔 Test failed - checks-actions

compiler-errors · 2024-10-31T16:20:59Z

AH, yes, rebase...

compiler-errors · 2024-10-31T17:39:21Z

@bors try @rust-timer queue

bors · 2024-10-31T17:40:31Z

⌛ Trying commit 505e083 with merge 95d3a79...

…=<try> Double-check conditional constness in MIR To prevent any unchecked `~const` bounds from leaking through during MIR lowering. If this check fails, it will eventually just delay a bug, but for now it reports errors. That error reporting may be redundant if we're calling it from code that already doesn't allow `~const` (i.e. when the `effects` and `const_trait_impl` gates are disabled), but I don't think it's that big of a deal. edit: This also makes sure that we issue a const stability error if we encounter *any* function with const conditions when `const_trait_impl` is not enabled. This ensures that that feature remains airtight.

bors · 2024-10-31T19:29:26Z

☀️ Try build successful - checks-actions
Build commit: 95d3a79 (95d3a7902bae7929446953c1dca12f5e2465aa63)

rust-timer · 2024-10-31T21:47:39Z

Finished benchmarking commit (95d3a79): comparison URL.

Overall result: ❌ regressions - no action needed

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.

@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)	1.0%	[0.8%, 1.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 2.2%)

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

Cycles

Results (secondary -5.3%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.3%	[-7.1%, -3.5%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 784.059s -> 784.63s (0.07%)
Artifact size: 333.59 MiB -> 333.57 MiB (-0.01%)

compiler-errors · 2024-10-31T21:48:30Z

@bors r=fee1-dead

bors · 2024-10-31T21:48:32Z

📌 Commit 505e083 has been approved by fee1-dead

It is now in the queue for this repository.

bors · 2024-11-01T06:16:03Z

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

To prevent any conditional constness from leaking through during MIR lowering

…check unless const_trait_impl is enabled This will help us make sure that we never leak any conditionally const functions into stable.

compiler-errors · 2024-11-01T16:14:11Z

@bors r=fee1-dead

bors · 2024-11-01T16:14:14Z

📌 Commit 57f2e12 has been approved by fee1-dead

It is now in the queue for this repository.

bors · 2024-11-01T19:08:00Z

⌛ Testing commit 57f2e12 with merge 705cfe0...

bors · 2024-11-01T21:48:07Z

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 705cfe0 to master...

rust-timer · 2024-11-01T23:57:04Z

Finished benchmarking commit (705cfe0): 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 (secondary 1.0%)

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)	3.1%	[2.3%, 4.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

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

Bootstrap: 780.79s -> 780.514s (-0.04%)
Artifact size: 334.96 MiB -> 334.93 MiB (-0.01%)

RalfJung · 2024-11-09T20:46:44Z

compiler/rustc_const_eval/src/check_consts/check.rs

+        // If there are any const conditions on this fn and `const_trait_impl`
+        // is not enabled, simply bail. We shouldn't be able to call conditionally
+        // const functions on stable.
+        if !const_conditions.is_empty() && !tcx.features().const_trait_impl() {


You should never call tcx.features() in this code, as that won't properly check for rustc_allow_const_fn_unstable.

Well, the user still needs to enable the feature gate globally so that we can properly enforce the feature's validity earlier in the compiler.

RalfJung · 2024-11-09T20:47:32Z

compiler/rustc_const_eval/src/check_consts/check.rs

-                if !errors.is_empty() {
-                    infcx.err_ctxt().report_fulfillment_errors(errors);
-                }
+                self.revalidate_conditional_constness(callee, fn_args, call_source, *fn_span);


I am confused, why is this called outside the if let Some(trait_did) = tcx.trait_of_item(callee) {? Can there be non-trait conditional const calls? Given that the const_trait_impl feature gate is used for these calls, I assume the answer is no...

Yes? We can put ~const bounds on free const functions.

Given that the const_trait_impl feature gate is used for these calls

I think you're getting too stuck on the naming of the const_trait_impl feature gate.

The feature gate enables the declaration of const traits, impl const, and the use of ~const trait bounds on conditionally const items, which includes free const function. This is how the gate has been used for the last like... 3 years or whatever.

I don't want to rename the feature gate, since that basically is just churn. the feature gate could be called foo_bar for all it matters.

That's a poorly chosen name. :(

🤷 I'm sorry, but I didn't choose it, and I'm not really interested in doing a rename here. That can be done later, and ideally not when there are like 3 other PRs in flight concerning it.

rustbot assigned lcnr Oct 28, 2024

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. labels Oct 28, 2024

rustbot assigned fee1-dead and unassigned lcnr Oct 28, 2024

fee1-dead reviewed Oct 29, 2024

View reviewed changes

compiler-errors force-pushed the enforce-fx-in-mir branch from 964af99 to 58614fa Compare October 30, 2024 16:57

compiler-errors commented Oct 30, 2024

View reviewed changes

compiler-errors force-pushed the enforce-fx-in-mir branch from 58614fa to 579cef2 Compare October 30, 2024 17:02

compiler-errors commented Oct 30, 2024

View reviewed changes

compiler-errors force-pushed the enforce-fx-in-mir branch 2 times, most recently from 00892c0 to 314b383 Compare October 30, 2024 18:49

fee1-dead approved these changes Oct 31, 2024

View reviewed changes

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 Oct 31, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2024

compiler-errors force-pushed the enforce-fx-in-mir branch from 314b383 to 505e083 Compare October 31, 2024 17:39

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 31, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2024

compiler-errors added 2 commits November 1, 2024 16:03

Double-check conditional constness in MIR

e319838

To prevent any conditional constness from leaking through during MIR lowering

Completely deny calling functions with const conditions in MIR const …

57f2e12

…check unless const_trait_impl is enabled This will help us make sure that we never leak any conditionally const functions into stable.

compiler-errors force-pushed the enforce-fx-in-mir branch from 505e083 to 57f2e12 Compare November 1, 2024 16:13

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 1, 2024

bors merged commit 705cfe0 into rust-lang:master Nov 1, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Nov 1, 2024

bors mentioned this pull request Nov 1, 2024

Use TypingMode throughout the compiler instead of ParamEnv #132460

Merged

RalfJung reviewed Nov 9, 2024

View reviewed changes

		@@ -1,3 +1,15 @@
		error[E0015]: cannot call non-const fn `<cross_crate::NonConst as cross_crate::MyTrait>::func` in constant functions

Double-check conditional constness in MIR #132276

Double-check conditional constness in MIR #132276

Conversation

compiler-errors commented Oct 28, 2024 • edited Loading

rustbot commented Oct 28, 2024

compiler-errors commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 30, 2024

Choose a reason for hiding this comment

compiler-errors Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

fee1-dead left a comment

Choose a reason for hiding this comment

compiler-errors commented Oct 31, 2024

This comment has been minimized.

fee1-dead commented Oct 31, 2024

bors commented Oct 31, 2024

This comment has been minimized.

bors commented Oct 31, 2024

compiler-errors commented Oct 31, 2024

compiler-errors commented Oct 31, 2024

This comment has been minimized.

bors commented Oct 31, 2024

bors commented Oct 31, 2024

This comment has been minimized.

rust-timer commented Oct 31, 2024

Overall result: ❌ regressions - no action needed

compiler-errors commented Oct 31, 2024

bors commented Oct 31, 2024

bors commented Nov 1, 2024

compiler-errors commented Nov 1, 2024

bors commented Nov 1, 2024

bors commented Nov 1, 2024

bors commented Nov 1, 2024

rust-timer commented Nov 1, 2024

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Oct 28, 2024 •

edited

Loading

compiler-errors Oct 30, 2024 •

edited

Loading

compiler-errors Nov 9, 2024 •

edited

Loading