From 9760983353e2600058b5b39d7c97a8bcbe21c583 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 13 Nov 2024 11:58:27 +0100 Subject: [PATCH] check_consts: fix error requesting feature gate when that gate is not actually needed --- .../src/check_consts/check.rs | 33 +++++++++++++------ .../rustc_const_eval/src/check_consts/ops.rs | 15 +++++++-- ...cursive_const_stab_unstable_if_unmarked.rs | 2 +- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_const_eval/src/check_consts/check.rs b/compiler/rustc_const_eval/src/check_consts/check.rs index c3efca28c688a..ffe32acb31656 100644 --- a/compiler/rustc_const_eval/src/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/check_consts/check.rs @@ -272,9 +272,18 @@ impl<'mir, 'tcx> Checker<'mir, 'tcx> { /// context. pub fn check_op_spanned>(&mut self, op: O, span: Span) { let gate = match op.status_in_item(self.ccx) { - Status::Unstable { gate, safe_to_expose_on_stable, is_function_call } - if self.tcx.features().enabled(gate) => - { + Status::Unstable { + gate, + safe_to_expose_on_stable, + is_function_call, + gate_already_checked, + } if gate_already_checked || self.tcx.features().enabled(gate) => { + if gate_already_checked { + assert!( + !safe_to_expose_on_stable, + "setting `gate_already_checked` without `safe_to_expose_on_stable` makes no sense" + ); + } // Generally this is allowed since the feature gate is enabled -- except // if this function wants to be safe-to-expose-on-stable. if !safe_to_expose_on_stable @@ -745,7 +754,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> { self.check_op(ops::IntrinsicUnstable { name: intrinsic.name, feature, - const_stable: is_const_stable, + const_stable_indirect: is_const_stable, }); } Some(ConstStability { level: StabilityLevel::Stable { .. }, .. }) => { @@ -800,6 +809,12 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> { // We only honor `span.allows_unstable` aka `#[allow_internal_unstable]` if // the callee is safe to expose, to avoid bypassing recursive stability. + // This is not ideal since it means the user sees an error, not the macro + // author, but that's also the case if one forgets to set + // `#[allow_internal_unstable]` in the first place. Note that this cannot be + // integrated in the check below since we want to enforce + // `callee_safe_to_expose_on_stable` even if + // `!self.enforce_recursive_const_stability()`. if (self.span.allows_unstable(feature) || implied_feature.is_some_and(|f| self.span.allows_unstable(f))) && callee_safe_to_expose_on_stable @@ -830,15 +845,13 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> { && issue == NonZero::new(27812) && self.tcx.sess.opts.unstable_opts.force_unstable_if_unmarked }; - // We do *not* honor this if we are in the "danger zone": we have to enforce - // recursive const-stability and the callee is not safe-to-expose. In that - // case we need `check_op` to do the check. - let danger_zone = !callee_safe_to_expose_on_stable - && self.enforce_recursive_const_stability(); - if danger_zone || !feature_enabled { + // Even if the feature is enabled, we still need check_op to double-check + // this if the callee is not safe to expose on stable. + if !feature_enabled || !callee_safe_to_expose_on_stable { self.check_op(ops::FnCallUnstable { def_id: callee, feature, + feature_enabled, safe_to_expose_on_stable: callee_safe_to_expose_on_stable, }); } diff --git a/compiler/rustc_const_eval/src/check_consts/ops.rs b/compiler/rustc_const_eval/src/check_consts/ops.rs index 036ca76328045..ca95e42dd2bac 100644 --- a/compiler/rustc_const_eval/src/check_consts/ops.rs +++ b/compiler/rustc_const_eval/src/check_consts/ops.rs @@ -28,6 +28,9 @@ pub enum Status { Unstable { /// The feature that must be enabled to use this operation. gate: Symbol, + /// Whether the feature gate was already checked (because the logic is a bit more + /// complicated than just checking a single gate). + gate_already_checked: bool, /// Whether it is allowed to use this operation from stable `const fn`. /// This will usually be `false`. safe_to_expose_on_stable: bool, @@ -82,6 +85,7 @@ impl<'tcx> NonConstOp<'tcx> for ConditionallyConstCall<'tcx> { // We use the `const_trait_impl` gate for all conditionally-const calls. Status::Unstable { gate: sym::const_trait_impl, + gate_already_checked: false, safe_to_expose_on_stable: false, // We don't want the "mark the callee as `#[rustc_const_stable_indirect]`" hint is_function_call: false, @@ -330,6 +334,9 @@ impl<'tcx> NonConstOp<'tcx> for FnCallNonConst<'tcx> { pub(crate) struct FnCallUnstable { pub def_id: DefId, pub feature: Symbol, + /// If this is true, then the feature is enabled, but we need to still check if it is safe to + /// expose on stable. + pub feature_enabled: bool, pub safe_to_expose_on_stable: bool, } @@ -337,12 +344,14 @@ impl<'tcx> NonConstOp<'tcx> for FnCallUnstable { fn status_in_item(&self, _ccx: &ConstCx<'_, 'tcx>) -> Status { Status::Unstable { gate: self.feature, + gate_already_checked: self.feature_enabled, safe_to_expose_on_stable: self.safe_to_expose_on_stable, is_function_call: true, } } fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> Diag<'tcx> { + assert!(!self.feature_enabled); let mut err = ccx.dcx().create_err(errors::UnstableConstFn { span, def_path: ccx.tcx.def_path_str(self.def_id), @@ -376,14 +385,15 @@ impl<'tcx> NonConstOp<'tcx> for IntrinsicNonConst { pub(crate) struct IntrinsicUnstable { pub name: Symbol, pub feature: Symbol, - pub const_stable: bool, + pub const_stable_indirect: bool, } impl<'tcx> NonConstOp<'tcx> for IntrinsicUnstable { fn status_in_item(&self, _ccx: &ConstCx<'_, 'tcx>) -> Status { Status::Unstable { gate: self.feature, - safe_to_expose_on_stable: self.const_stable, + gate_already_checked: false, + safe_to_expose_on_stable: self.const_stable_indirect, // We do *not* want to suggest to mark the intrinsic as `const_stable_indirect`, // that's not a trivial change! is_function_call: false, @@ -410,6 +420,7 @@ impl<'tcx> NonConstOp<'tcx> for Coroutine { { Status::Unstable { gate: sym::const_async_blocks, + gate_already_checked: false, safe_to_expose_on_stable: false, is_function_call: false, } diff --git a/tests/ui/consts/min_const_fn/recursive_const_stab_unstable_if_unmarked.rs b/tests/ui/consts/min_const_fn/recursive_const_stab_unstable_if_unmarked.rs index 51811b14203ad..429fa060521f5 100644 --- a/tests/ui/consts/min_const_fn/recursive_const_stab_unstable_if_unmarked.rs +++ b/tests/ui/consts/min_const_fn/recursive_const_stab_unstable_if_unmarked.rs @@ -1,6 +1,6 @@ //@ compile-flags: -Zforce-unstable-if-unmarked //@ edition: 2021 -#![feature(const_async_blocks, rustc_attrs, rustc_private)] +#![feature(const_async_blocks, rustc_attrs)] pub const fn not_stably_const() { // We need to do something const-unstable here.