const fn stability checking: also check declared language features #129659

RalfJung · 2024-08-27T15:07:58Z

@oli-obk I assume it is just an oversight that this didn't use features().declared()? Or is there a deep reason that this must only check declared_lib_features?

rustbot · 2024-08-27T15:08:06Z

r? @fee1-dead

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

fee1-dead · 2024-08-29T11:21:13Z

The only reason I could think of is that we might allow rustc_const_unstable under a lang feature, but the lang feature is already accepted. So when someone tries to call that const function in const contexts, it would suggest them to enable the feature, but when they enable the feature they'd see a warning that it is already stable?

oli is on leave, so maybe we can get more eyes on this from someone else who is familiar with stability checking

RalfJung · 2024-08-29T11:36:46Z

The only reason I could think of is that we might allow rustc_const_unstable under a lang feature, but the lang feature is already accepted. So when someone tries to call that const function in const contexts, it would suggest them to enable the feature, but when they enable the feature they'd see a warning that it is already stable?

That would equally apply to regular stability, not just const stability, right?
We might want to reject unstable/rustc_const_unstable with an already-stable (or removed) language feature. But that's a separate question.

RalfJung · 2024-08-29T11:37:42Z

The status quo is that the example in #129656 suggests to enable the f128 feature gate which is already enabled. So that's clearly worse than what you describe. ;)

oli-obk

I can't make any statements about the current or expected behavior around (const) stability without putting more thought and time into it than I am currently able to

oli-obk · 2024-08-31T09:47:42Z

tests/ui/consts/min_const_fn/const-fn-lang-feature.rs

@@ -0,0 +1,19 @@
+//! Ensure that we can use a language feature with a `const fn` and things behave as expected.


Explain what "expected" means in detail.

Well, it should work like all feature gates work -- if you enable the feature gate it is actually considered enabled...

RalfJung · 2024-08-31T10:19:24Z

To me this looks like a fairly simple oversight -- the code does tcx.features().declared_lib_features.iter().any(...) to check whether a feature gate is enabled, but everywhere else in the compiler we use tcx.features().declared(...) to do that check.

fee1-dead · 2024-08-31T10:20:22Z

We might want to reject unstable/rustc_const_unstable with an already-stable (or removed) language feature. But that's a separate question.

Let's add that as a test. If this isn't rejected, we should open an issue to track fixing it for both unstable and rustc_const_unstable. If the case for unstable is already fixed but not rustc_const_unstable, I'd want this PR to have that fixed also.

The status quo is that the example in #129656 suggests to enable the f128 feature gate which is already enabled. So that's clearly worse than what you describe. ;)

Well, that requires someone who enables the staged_api feature to specify a lang feature with rustc_const_unstable. It's bad, but I think there's not a lot urgency for making it possible.. Even if the PR using f128_const lands first, we can also just rename the feature afterwards.

RalfJung · 2024-08-31T10:37:53Z

I think there's not a lot urgency

I didn't say that this fix is more urgent than a regular PR. It's just a normal bugfix. :)

Let's add that as a test. If this isn't rejected, we should open an issue to track fixing it for both unstable and rustc_const_unstable. If the case for unstable is already fixed but not rustc_const_unstable, I'd want this PR to have that fixed also.

This is completely orthogonal to the issue this PR is concerned with. Please don't ask the PR to be extended in scope to fix unrelated issues.

This PR is about making rustc_const_unstable consistent with unstable.

I created an issue to track the concern around already-stable lang features: #129814.

fee1-dead · 2024-08-31T11:02:23Z

Let's add that as a test. If this isn't rejected, we should open an issue to track fixing it for both unstable and rustc_const_unstable. If the case for unstable is already fixed but not rustc_const_unstable, I'd want this PR to have that fixed also.

This is completely orthogonal to the issue this PR is concerned with. Please don't ask the PR to be extended in scope to fix unrelated issues.

Maybe I should be the person opening that issue.. I shouldn't be asking for more work when it isn't necessary. Sorry if that nudge was annoying!

On another look, I think there is no deep reason - probably an oversight as you said, so let's merge it.

@bors r+ rollup

bors · 2024-08-31T11:02:26Z

📌 Commit c298417 has been approved by fee1-dead

It is now in the queue for this repository.

fee1-dead · 2024-08-31T11:06:03Z

The actual reason that this used declared_lib_features instead of .declared is probably that we just didn't consider the potential use for specifying a lang feature in rustc_const_unstable. I guess I was trying too hard to find any deep reasons for this.

…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#129659 (const fn stability checking: also check declared language features) - rust-lang#129711 (Expand NLL MIR dumps) - rust-lang#129730 (f32 docs: define 'arithmetic' operations) - rust-lang#129733 (Subtree update of `rust-analyzer`) - rust-lang#129749 (llvm-wrapper: adapt for LLVM API changes) - rust-lang#129757 (Add a test for trait solver overflow in MIR inliner cycle detection) - rust-lang#129760 (Make the "detect-old-time" UI test more representative) - rust-lang#129767 (Remove `#[macro_use] extern crate tracing`, round 4) - rust-lang#129774 (Remove `#[macro_use] extern crate tracing` from rustdoc and rustfmt) - rust-lang#129785 (Miri subtree update) - rust-lang#129791 (mark joboet as on vacation) - rust-lang#129812 (interpret, codegen: tweak some comments and checks regarding Box with custom allocator) Failed merges: - rust-lang#129777 (Add `unreachable_pub`, round 4) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129659 - RalfJung:const-fn-lang-feat, r=fee1-dead const fn stability checking: also check declared language features Fixes rust-lang#129656 `@oli-obk` I assume it is just an oversight that this didn't use `features().declared()`? Or is there a deep reason that this must only check `declared_lib_features`?

rustbot assigned fee1-dead Aug 27, 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 Aug 27, 2024

beetrees mentioned this pull request Aug 28, 2024

Fix using lang features in #[rustc_const_unstable] #129679

Closed

oli-obk reviewed Aug 31, 2024

View reviewed changes

const fn stability checking: also check declared language features

c298417

RalfJung force-pushed the const-fn-lang-feat branch from 33076f5 to c298417 Compare August 31, 2024 10:14

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2024

matthiaskrgr mentioned this pull request Aug 31, 2024

Rollup of 12 pull requests #129817

Merged

bors merged commit ea5bb99 into rust-lang:master Aug 31, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 31, 2024

RalfJung deleted the const-fn-lang-feat branch August 31, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

const fn stability checking: also check declared language features #129659

const fn stability checking: also check declared language features #129659

RalfJung commented Aug 27, 2024

rustbot commented Aug 27, 2024

fee1-dead commented Aug 29, 2024 •

edited

Loading

RalfJung commented Aug 29, 2024

RalfJung commented Aug 29, 2024

oli-obk left a comment

oli-obk Aug 31, 2024

RalfJung Aug 31, 2024

RalfJung commented Aug 31, 2024

fee1-dead commented Aug 31, 2024

RalfJung commented Aug 31, 2024 •

edited

Loading

fee1-dead commented Aug 31, 2024

bors commented Aug 31, 2024

fee1-dead commented Aug 31, 2024 •

edited

Loading

		@@ -0,0 +1,19 @@
		//! Ensure that we can use a language feature with a `const fn` and things behave as expected.

const fn stability checking: also check declared language features #129659

const fn stability checking: also check declared language features #129659

Conversation

RalfJung commented Aug 27, 2024

rustbot commented Aug 27, 2024

fee1-dead commented Aug 29, 2024 • edited Loading

RalfJung commented Aug 29, 2024

RalfJung commented Aug 29, 2024

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Aug 31, 2024

Choose a reason for hiding this comment

RalfJung Aug 31, 2024

Choose a reason for hiding this comment

RalfJung commented Aug 31, 2024

fee1-dead commented Aug 31, 2024

RalfJung commented Aug 31, 2024 • edited Loading

fee1-dead commented Aug 31, 2024

bors commented Aug 31, 2024

fee1-dead commented Aug 31, 2024 • edited Loading

fee1-dead commented Aug 29, 2024 •

edited

Loading

RalfJung commented Aug 31, 2024 •

edited

Loading

fee1-dead commented Aug 31, 2024 •

edited

Loading