Incorrect effect of stability attribute on calling private const fn

[dtolnay]

In general we don't require private functions to have stability attributes. Stable functions are free to call private functions, and stable const functions are free to call private const functions that have no stability attribute.

But when a private const function inherits a stability attribute from an enclosing module, it currently can no longer be called from stable const functions. I think this behavior is not correct. For a private function, I would think stability attributes should be irrelevant.

This is causing trouble in #75772 (comment).

#![feature(staged_api, const_fn)]
#![stable(feature = "repro", since = "0")]

mod detail {
    #![unstable(feature = "detail", issue = "none")]  // works if removed

    pub(crate) const fn f() {}

    // presumably other public stable things
}

#[stable(feature = "repro", since = "0")]
#[rustc_const_stable(feature = "repro", since = "0")]
pub const fn f() {
    detail::f();
}

error[E0723]: can only call other `const fn` within a `const fn`, but `const detail::f` is not stable as `const fn`
  --> src/lib.rs:15:5
   |
15 |     detail::f();
   |     ^^^^^^^^^^^
   |
   = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
   = help: add `#![feature(const_fn)]` to the crate attributes to enable

Cross referencing const fn tracking issue #57563.

[ecstatic-morse]

This was a conscious choice:

rust/src/librustc_mir/const_eval/fn_queries.rs

Lines 66 to 71 in 663d2f5

	// Internal functions are forced to conform to min_const_fn.
	// Annotate the internal function with a const stability attribute if
	// you need to use unstable features.
	// Note: this is an arbitrary choice that does not affect stability or const
	// safety or anything, it just changes whether we need to annotate some
	// internal functions with `rustc_const_stable` or with `rustc_const_unstable`

I suppose we could change this if it's causing problems, but some caution here is warranted. We've made some mistakes in the past with these checks.

[dtolnay]

Thanks for the link!

Looking at the workaround in https://github.com/rust-lang/rust/pull/75772/files#r474918624 (adding an unstable annotation on private function), something still seems wrong; it may not have been the thing I first thought.

If we want private helpers to be annotated with rustc_const_stable when part of the control flow of a public stable const fn, that's great and I am on board with that. We avoid accidentally committing to too much power in const evaluation. So I see why rustc_const_stable on private functions is a useful thing.

But from reading the comment you linked, I'm not sure why this specific error would go away by adding an unstable annotation on a private function -- how come stable or unstable on a private function has any meaning at all?

[ecstatic-morse]

Well the branch that makes unstable work around the restriction is immediately above the previous link:

rust/src/librustc_mir/const_eval/fn_queries.rs

Lines 62 to 63 in 45060c2

	// Unstable functions need not conform to min_const_fn.
	false

As for the "why", I didn't write the code or formulate the rules. My understanding is that, as long as a function is unstable in some way (either with the usual stability attributes or the similar const ones), there's no danger of accidentally depending on some nightly-only const feature since stable functions can't call unstable ones and those checks are relatively straightforward. Whether private functions are considered (const) "stable" or "unstable" is not obvious, so this code takes a conservative approach an considers all un-annotated functions possibly non-conformant.

[dtolnay]

What would go wrong if we treat all private functions as unstable for the purpose of this check in const eval?

They may then separately be rustc_const_stable or not, as appropriate.

[ecstatic-morse]

We would have to ensure that all private unstable but not rustc_const_unstable functions conform to the "min const fn" restrictions and do not use any unstable const-eval features. Also, if there were any existing private, unstable const fn that did not pass the "min const fn" checks, you would have to annotate them withrustc_const_unstable. I think this is not hard but also not high priority for me personally.

[dtolnay]

That seems backward--- If "Unstable functions need not conform to min_const_fn" according to #75794 (comment), and we were to treat all private functions as if they were unstable as per #75794 (comment), why would that imply ensuring that all private functions conform to min const fn restrictions?

[ecstatic-morse]

If you want to call a function (the callee) from a function that is marked both stable and rustc_const_stable (the caller), the callee must also pass the "min const fn" checks. This is enforced as part of the "min const fn" checks on the caller. In other words, "min const fn" conformance is transitive. I interpreted your comment by assuming you want to have unstable functions that don't have any of rustc_const_unstable or rustc_const_stable be subject to the "min const fn" checks. Is this incorrect?

The hard rule here is that we can't let const-stable functions call functions that do not pass the "min const fn" checks.

[dtolnay]

I'll poke @oli-obk because from looking at the commit history of the is_min_const_fn logic, this looks like maybe just an implementation oversight in 83b8a56.

The scenario is:

#![feature(staged_api, const_fn)]
#![stable(feature = "repro", since = "0")]
#![allow(dead_code)]

#[unstable(feature = "detail", issue = "none")]  // does not compile
//#[stable(feature = "repro", since = "0")]  // compiles
pub mod detail {
    pub(crate) const fn bar() {}
}

// not public, not rustc_const_stable, maybe not even used
const fn foo() {
    detail::bar();
}

(playground)

@oli-obk can you tell whether it's intentional in this code that foo would compile or not based on whether the module containing the private helper is stable or unstable?

[dtolnay]

@ecstatic-morse:

I interpreted your comment by assuming you want to have unstable functions that don't have any of rustc_const_unstable or rustc_const_stable be subject to the "min const fn" checks. Is this incorrect?

The thing I want is for stable vs unstable to not be meaningful whatsoever on a private function. Those attributes are meant to describe the public API of a crate only.

I think the code I opened this issue with ended up not being representative of the confusion in #75772. The code in my previous comment is the most accurate.

[ecstatic-morse]

In that example, the choice of whether foo must pass the "min const fn" checks is arbitrary as explained here:

rust/src/librustc_mir/const_eval/fn_queries.rs

Lines 66 to 71 in 663d2f5

	// Internal functions are forced to conform to min_const_fn.
	// Annotate the internal function with a const stability attribute if
	// you need to use unstable features.
	// Note: this is an arbitrary choice that does not affect stability or const
	// safety or anything, it just changes whether we need to annotate some
	// internal functions with `rustc_const_stable` or with `rustc_const_unstable`

However, in your original example, the outer function is const-stable, and thus must be "min const fn".

There's also the question of whether bar must conform to "min const fn". I think we could (not necessarily should) force all unstable functions to conform to "min const fn" and allow opt-out solely via rustc_const_unstable, in which case the process in #75794 (comment) would have to be followed. edit: In that comment, I used the term "private" instead of the very specific combination of stability attributes that applies to bar. This probably caused some of the confusion.

[dtolnay]

There's also the question of whether bar must conform to "min const fn". I think we could force all unstable functions to conform to "min const fn" and allow opt-out solely via rustc_const_unstable, in which case the process in #75794 (comment) would have to be followed.

I think this is where we are not seeing this the same way. As the libs team sees it, functions in the standard library are one of:

• public and stable
• public and unstable
• private

Bar is not unstable, it is private. foo and bar in #75794 (comment) should have exactly the same capabilities and restrictions.

In reality it appears that foo is experiencing is_min_const_fn=true as per here, while bar is experiencing is_min_const_fn=false as per here, which I think might have been an oversight in 83b8a56.

[ecstatic-morse]

It seems that you would be more receptive to an explanation if it came from @oli-obk. I'll stop polluting this thread with noise, but I will point out that bar inherits any stability attributes from it's containing lexical scope, so bar is unstable according to lookup_stability despite not being public.

[dtolnay]

bar inherits any stability attributes from it's containing lexical scope, so bar is unstable according to lookup_stability.

That's where I'm saying the is_min_const_fn implementation is buggy. :)

Not knowing the actual implementation but just the practical effect from the libs team point of view, the reason lookup_stability is implemented that way is so that we can be lazy with unstable attributes while iterating quickly on a new module. The following compiles, intentionally:

#![feature(staged_api, const_fn)]
#![stable(feature = "repro", since = "0")]

#[unstable(feature = "wip", issue = "none")]
pub mod detail {
    pub struct U;
    pub struct V;
}

Though note that stable are not inherited; the following does not compile.

#![feature(staged_api, const_fn)]
#![stable(feature = "repro", since = "0")]

#[stable(feature = "repro", since = "0")]
pub mod detail {
    pub struct U;
    pub struct V;
}

error: struct has missing stability attribute
 --> src/lib.rs:6:5
  |
6 |     pub struct U;
  |     ^^^^^^^^^^^^^

error: struct has missing stability attribute
 --> src/lib.rs:7:5
  |
7 |     pub struct V;
  |     ^^^^^^^^^^^^^

The thing is, before is_min_const_fn existed, the behavior of lookup_stability on anything private was completely irrelevant. Whatever behavior it has is probably whatever was easiest to implement.

For the behavior that we want for min const fn, I don't think it makes sense to use lookup_stability in the current way, or else lookup_stability itself needs to change in order to respect that stable/unstable isn't a concept that applies to private things.

[ecstatic-morse]

That all makes sense. I have long wanted a more consistent version of these rules, since the current ones seem to be mostly emergent. The hard requirement here is that all publicly exported, const-stable functions pass the "min const fn" checks. This requires that all of its (transitive) callees, whether private, public, stable or unstable, also pass the "min const fn" checks. Everything else is flexible as far as I know. I think the changes you want are broader than 83b8a56, but perhaps I'm mistaken.

[dtolnay]

The hard requirement here is that all publicly exported, const-stable functions pass the "min const fn" checks. This requires that all of its (transitive) callees, whether private, public, stable or unstable, also pass the "min const fn" checks.

Sounds good, I'm fully on board with that. The hard requirement from the libs team is that functions are exactly one of publicly exported and stable, publicly exported and unstable, or not publicly exported. There mustn't be anything that hinges on whether a private function is "stable" or "unstable". Those things refer to whether we are permitted to make breaking changes still to a public API, and they have no relevance to anything that cannot be accessed publicly.

We're fine with an independent dimension of const-stable vs const-unstable which applies to both public and private functions.

[oli-obk]

The reason we have this wonky system is that we didn't use to have rustc_const_(un)stable (and we didn't even get both of these at the same time, rustc_const_stable came quite a bit later than rustc_const_unstable). So yea, this is an emergent system that tried to not have any effect on unstable const fns because there were quite a few of them and annotating them with the fact that their constness is also unstable seemed odd.

I would definitely like to have a hard requirement of either rustc_const_stable or rustc_const_unstable on all const fn, but won't that be counter to the point of being able to quickly evolve an unstable module without requiring all these attributes?

I would have thought that

#![feature(staged_api, const_fn)]
#![stable(feature = "repro", since = "0")]
#![allow(dead_code)]

#[unstable(feature = "detail", issue = "none")]
pub mod detail {
    #[rustc_const_stable(...)]
    pub(crate) const fn bar() {}
}

// not public, not rustc_const_stable, maybe not even used
const fn foo() {
    detail::bar();
}

would compile, which is also how I'd expect all of this to work. Maybe we should not be checking the regular stability of the const fn, but whether the current crate is staged_api to decide whether we treat all unmarked const fns as rustc_const_unstable?

[RalfJung]

Cc @rust-lang/wg-const-eval (looks like the group hasn't been pinged yet)

[RalfJung]

@dtolnay isn't it currently the case that all private methods are considered implicitly "unstable" by the stability system? AFAIK in a crate with staged_api, every function (or type, ...) is either stable or unstable, and there is no notion of "private" (and interacting with the visibility system might be hard). But maybe I misremember.

If that is the case, then wouldn't the consistent thing to do with const fn be to treat const-unannounced const fn as implicitly rustc_const_usntable, even if they are #[stable]? Then everything should be like it is with run-time functions, except that rustc_const_stable is "infectious" in the sense that its callees must also all be rustc_const_stable (even if they are private).

EDIT: turns out visibility is taken into account by the normal stability system, that is skip_stability_check_due_to_privacy. So maybe the const-stability system should do the same.

[ecstatic-morse]

turns out visibility is taken into account by the normal stability system, that is skip_stability_check_due_to_privacy. So maybe the const-stability system should do the same.

This seems right. I mentioned this issue in #76618, but tweaking these rules can be done separately. I'll need to do a bit of refactoring to incorporate them into check_consts is all.