Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update const-stability rules because we're not in min-const-fn times any more #129815

Open
RalfJung opened this issue Aug 31, 2024 · 3 comments
Open
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-const-eval Working group: Const evaluation

Comments

@RalfJung
Copy link
Member

The rustc_const_unstable/rustc_const_stable system was designed in a time when most const fn in the standard library were not meant to be const-stable, no matter whether they are #[stable] or #[unstable]. However, as time passes, it becomes more and more common that functions are const fn from the start, and become const-stable the same time they become stable. So maybe we should reconsider some aspects of that system.

In particular, I think it would make sense to treat a function like this

#[unstable(...)]
const fn foo() {}

as-if it also carried a rustc_const_unstable attribute (with the same feature gate and issue as the unstable attribute). That would avoid confusion such as what we saw here.

The more interesting question is what to do with

#[stable(...)]
const fn foo() {}

If the body of the function doesn't do anything const-unstable, it seems fine to just treat this as const-stable, maybe? Or do we want to still force people to add an explicit rustc_const_stable? I am not sure I see the value in that -- the dangerous case is when that function calls some const-unstable things, such as const-unstable intrinsics. That will still require rustc_allow_const_fn_unstable, and that is the point where we have to ensure we get involved.

To avoid any kind of accidental changes, we probably want to start by making it an error for a const fn to carry stable or unstable without also carrying rustc_const_stable or rustc_const_unstable. I had a brief look at the stability system but couldn't find an obvious place to add such a lint; so if someone has an idea please let me know. :)

And speaking of rustc_allow_const_fn_unstable -- I feel like the way we use that also may need to change. A lot of the current uses of that attribute are a case of "some public stable function is implemented using still-unstable functions" -- but the still-unstable functions are not actually doing anything funky const-wise. They could be marked rustc_const_stable, meaning "wg-const-eval approved their use on stable", even while t-libs-api still debates whether the API itself (independent of its const usage) should exist on stable.

Cc @rust-lang/wg-const-eval

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 31, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Aug 31, 2024

They could be marked rustc_const_stable, meaning "wg-const-eval approved their use on stable", even while t-libs-api still debates whether the API itself (independent of its const usage) should exist on stable.

The thing that makes this currently not work well is that we can't just do

    #[unstable(feature = "ptr_alignment_type", issue = "102070")]
    #[rustc_const_stable]
    pub const fn as_usize(self) -> usize {

We need to say under which feature gate and since when the function is stable, which makes no sense for a function that is still unstable. Really here we want to use rustc_const_stable to just mark "the function may be called from const-stable functions, but it can't be called directly by users" -- this shouldn't show up in the docs or so, it is a purely internal decision.

We have at least one feature gates exist just due to this: raw_vec_internals_const.

Maybe we need a new attribute like #[rustc_const_expose_on_stable] for that? This could only be used on #[unstable] functions, and wouldn't have a feature gate or issue number. If that function ever becomes #[stable], it can then immediately also become const-stable. #[rustc_const_stable] would then only be used for functions that became const-stable at a different time / under a different feature gate than when they became #[stable]. (People are anyway confused that you can mark an unstable item #[rustc_const_stable] so this would also help with that.)

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-const-eval Working group: Const evaluation labels Aug 31, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Aug 31, 2024

Another issue with the current system around rustc_allow_const_fn_unstable is that using this with a library feature is kind of leaky: if the functions in that library feature change to use more unstable things, suddenly those things are also exposed on stable, possibly without the person doing that change even realizing.

We should define what exactly the goal of this "transitive stability check" for const fn is, and then ensure the implementation can achieve that goal. Right now, I think it fails to achieve the desired goal -- it is too easy to accidentally bypass, or to forget to Cc us when an intrinsic gets exposed on stable the first time (e.g. the ptr_metadata and aggregate_raw_ptr intrinsics got exposed and we didn't even realize). Granted, that was caused by rustc_allow_const_fn_unstable being added without consulting us, which we can't do much about -- but we could re-design the system so that rustc_allow_const_fn_unstable is hardly ever required, which makes it a more significant signal and hopefully makes it less likely that the attribute is overlooked.

I think the goal should be to protect language features and intrinsics from being const-exposed on stable, even transitively. We don't really care about unstable library features being const-exposed on stable.

So we need to partition the const fn in the standard library in two sets:

  1. Those that don't use any unstable language feature or intrinsics, and thus could be exposed on stable without further involvement from wg-const-eval. These functions may or may not be directly const-callable on stable; that question is orthogonal!
  2. Those that do use unstable language feature or intrinsics.

rustc_allow_const_fn_unstable can be used to put a function in the first set even though it uses an unstable language feature or intrinsic. But this should not let the function call functions from the second set! The first set may only ever call functions from the first set, no exceptions.

Furthermore, every function that is directly const-callable on stable needs to be in the first set.

Proposed design

Fundamentally the main thing I think I want to change is to disambiguate whether a rustc_const_unstable function is "unstable because t-libs-api is not sure yet whether we want this as a public API" or "unstable because it does things that shouldn't be const-exposed on stable". Currently we have no way of distinguishing these cases, and that's not great.

So overall I would propose the following:

  • We only use rustc_const_stable and rustc_const_unstable on publicly reachable functions, and they control whether a function can be directly const-called. This matches the way the stable and unstable attributes work. If a const fn has a #[stable] or #[unstable] attribute and no const stability attribute, its const-stability matches its regular stability.
  • We have a new attribute rustc_const_not_exposed_on_stable that indicates that a function may never, not even indirectly, be const-called on stable. This attribute is incompatible with rustc_const_stable (and in particular, a #[stable] function that is rustc_const_not_exposed_on_stable must also be #[rustc_const_unstable]).
  • In functions not marked rustc_const_not_exposed_on_stable, you can only
    • use language features and intrinsics that are either const-unstable, or allowed via rustc_allow_const_fn_unstable
    • call const fn that are not marked rustc_const_not_exposed_on_stable

rustc_const_not_exposed_on_stable can be added by t-libs without our involvement, but every use of rustc_allow_const_fn_unstable has to be approved by t-lang and involve wg-const-eval. (The names of the attributes could probably be improved to make it more clear that rustc_allow_const_fn_unstable is scary, but rustc_const_not_exposed_on_stable is harmless as we are enforcing that it is indeed not exposed on stable.)

This variant puts a const fn in set 1 by default unless indicated otherwise. That's because I think most functions fall in that set. The main alternative we could consider is to put a const fn in set 2 by default unless indicated otherwise, and require a rustc_const_expose_on_stable attribute to move it to set 1.

The status quo is that we interpret rustc_const_unstable to also mean rustc_const_not_exposed_on_stable, and hence allow all unstable things to be used in these functions -- but that is incompatible with the common pattern of using unstable APIs internally to implement stable APIs, which leads to more use of rustc_allow_const_fn_unstable and thus makes the entire system less effective at ensuring that we don't accidentally const-expose unstable const features.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 31, 2024

To avoid any kind of accidental changes, we probably want to start by making it an error for a const fn to carry stable or unstable without also carrying rustc_const_stable or rustc_const_unstable.

FWIW that doesn't work, stdarch has a ton of unstable const fn without an attribute. Even std has some such cases.

But we can at least ensure that stable const fn have an attribute.
EDIT: Ah, we already do that. :)

@compiler-errors compiler-errors added the C-discussion Category: Discussion or questions that doesn't represent real issues. label Aug 31, 2024
@fmease fmease changed the title Update const-stability rules becuase we're not in min-const-fn times any more Update const-stability rules because we're not in min-const-fn times any more Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-const-eval Working group: Const evaluation
Projects
None yet
Development

No branches or pull requests

4 participants