const-stabilize HashMap/HashSet::with_hasher (+ required compiller changes) #118427

aDotInTheVoid · 2023-11-29T00:31:15Z

Best reviewed commit-by-commit

It's somewhat involved, as it needs to introduce new details to rustc_allow_const_fn_unstable to allow calling 3rd party code. CC @rust-lang/wg-const-eval

Because std's dependencies are build with `-Zforce-unstable-if-unmarked` normally, std is unable to call them in const-stable function. However, becuase they had no explicit feature gates, there was no way to use `rustc_allow_const_fn_unstable` to allow this. Therefor we add `#[rustc_allow_const_fn_unstable(any)]` to allow using these functions. The reson to do this is rust-lang#102575, which relies on calling hashbrow function in const-stable std functions.

This needs to be `cfg(bootstrap)`ed, because we rely on `#[rustc_allow_const_fn_unstable(any)]`, which was introduced in the previous commit.

rustbot · 2023-11-29T00:31:24Z

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

RalfJung · 2023-11-29T15:51:44Z

library/std/src/collections/hash/map.rs

+            since = "CURRENT_RUSTC_VERSION"
+        )
+    )]
+    #[rustc_allow_const_fn_unstable(any)]


So this would allow this function to call any unstable function? That's not great, we specifically want to keep an eye on what is (recursively) exposed to const on stable, and with such an "any" handling that's not really possible.

In fact this calls into hashbrown, doesn't it? We'd want to ensure that hashbrown only uses const-stable features then. const nightly features are much more prone to making things fail-to-build when they get changed, which is why we have a more strict stability check than for regular non-const functions.

Ideally we'd be able to, and be required to, mark functions as rustc_const_stable in hashbrown itself; together with an agreement with the hashbrown maintainers that wg-const-eval will be consulted before making use of rustc_allow_const_fn_unstable. That requires a rework of the const stability stuff though I think since currently it is tied up with the regular stability stuff (but there's no need for that, having an unstable function be const-stable makes perfect sense).

I'm not sure what is the best way to do that. This might have to wait until Oli is back from vacation, it's a pretty fundamental change to our const stabilization policy.

So this would allow this function to call any unstable function?

No, only unmarked unstable functions. I should add a test for this (and change the name as well)

We'd want to ensure that hashbrown only uses const-stable features then.

This already happens because hashbrown's functions are const on stable (so only have access to things that have been const-stabilized).

However, because deps-of-std are build with -Zforce-unstable-if-unmarked, the compiler believes them to be unstably-const.

Ideally we'd be able to, and be required to, mark functions as rustc_const_stable in hashbrown itself;

This could be accomplished with a load of

#[cfg(feature = "rustc_dep_of_std", rustc_const_stable(...)]

but it would require a load of ugly changes to hashbrown, as this would also require them to mark all public functions as stable (because currently stability and const-stability are under the same #![feature(staged_api)]. Spliting that up would be a lot of work for very little benefit.

This already happens because hashbrown's functions are const on stable (so only have access to things that have been const-stabilized).

I see a lot of feature flags in hashbrown, in particular core_intrinsics which is potentially problematic. We surely don't want it to const-use any intrinsic we haven't exposed stably yet. How can we be sure that the hashbrown built for the standard library still only uses stable const features?

No, only unmarked unstable functions. I should add a test for this (and change the name as well)

What are "unmarked" unstable functions?

How can we be sure that the hashbrown built for the standard library still only uses stable const features?

I supose hashbrown could change to #[cfg_attr(feature="rustc_dep_of_std"), feature(const_super_unstable_and_broken)], and we wouldn't mechanicly know. I think the best way would be to ask them not to do that. Given that hashbrown needs to be able to provide a const-stable implementation of new, I'm not concerned about being backed into a corner where there's no way to implement new with stable const features.

core_intrinsics seems to only be used for

#[cfg(feature = "nightly")] use core::intrinsics::{likely, unlikely};

Can we move those under a different feature so that hashbrown can use them but not accidentally use anything else?

What are "unmarked" unstable functions?

See

rust/compiler/rustc_const_eval/src/transform/check_consts/check.rs

Lines 984 to 989 in b10cfcd

// FIXME(ecstaticmorse); For compatibility, we consider `unstable` callees that

// have no `rustc_const_stable` attributes to be const-unstable as well. This

// should be fixed later.

let callee_is_unstable_unmarked = tcx.lookup_const_stability(callee).is_none()

&& tcx.lookup_stability(callee).is_some_and(|s| s.is_unstable());

if callee_is_unstable_unmarked {

When a function is unstable, but not either const-stable or const-unstable. This is true for hashbrown, despite it not having any stability attributes, as it is built with -Zforce-unstable-if-unmarked such that it isn't availibe to normal crates, despite being available in the sysroot

rust/compiler/rustc_passes/src/stability.rs

Lines 635 to 651 in b10cfcd

// If the `-Z force-unstable-if-unmarked` flag is passed then we provide

// a parent stability annotation which indicates that this is private

// with the `rustc_private` feature. This is intended for use when

// compiling `librustc_*` crates themselves so we can leverage crates.io

// while maintaining the invariant that all sysroot crates are unstable

// by default and are unable to be used.

if tcx.sess.opts.unstable_opts.force_unstable_if_unmarked {

let stability = Stability {

level: attr::StabilityLevel::Unstable {

reason: UnstableReason::Default,

issue: NonZeroU32::new(27812),

is_soft: false,

implied_by: None,

},

feature: sym::rustc_private,

};

annotator.parent_stab = Some(stability);

Can we move those under a different feature so that hashbrown can use them but not accidentally use anything else?

Don't see why not. Although it looks like the pattern with intrinsic is to add wrappers that don't live in intrinsic, and put them on a separate feature and maybe stabilize them.

When reviewing a hashbrown PR it seems easy to miss that a new intrinsic being added is (a) const-unstable and (b) now exposed to const.

#![feature(core_intrinsics)] doesn't allow using them in const-contexts, that's on a seperate feature. While it's not impossible, it'd be much more likely (ha) to be flagged in review.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=43a21473dbd0af03612e0805e2a847f2

When a function is unstable, but not either const-stable or const-unstable. This is true for hashbrown, despite it not having any stability attributes, as it is built with -Zforce-unstable-if-unmarked such that it isn't availibe to normal crates, despite being available in the sysroot

Okay I see. I also see this comment

// FIXME(ecstaticmorse); For compatibility, we consider `unstable` callees that // have no `rustc_const_stable` attributes to be const-unstable as well. This // should be fixed later.

@ecstatic-morse I wonder what you meant by "fixing" this, what would be the more correct behavior?

IMO these should be treated like those with explicit rustc_const_unstable except they have no feature gate.

Don't see why not. Although it looks like the pattern with intrinsic is to add wrappers that don't live in intrinsic, and put them on a separate feature and maybe stabilize them.

Yes. But those will often not be const at all, or they are const unstable and hashbrown shouldn't use that feature ideally.

Hm... we could raise an error when an "unmarked" const function calls a const-unstable function? That should guarantee that when we allow rustc_allow_const_fn_unstable(unmarked) this can never call a function explicitly marked as const-unstable, not even transitively?

we could raise an error when an "unmarked" const function calls a const-unstable function?

We already do this! I've added a test for this.

Also, I've renamed the attribute to #[rustc_allow_const_fn_unstable(any_unmarked)], so it's clearer that you can't call arbitrary unstableley-const fns.

…f-unmarked`

RalfJung · 2023-12-01T16:01:45Z

tests/ui/stability-attribute/const-unstable-from-unmarked.rs

+
+pub const fn identity(x: i32) -> i32 {
+    const_unstable::identity(x)
+    //~^ ERROR `const_unstable::identity` is not yet stable as a const fn


What I was asking is that this should error even if the crate has the feature flag enabled. So please add feature(const_identity). I think then the error will disappear but we still want it to error (and require rustc_allow_const_fn_unstable to pass).

That's how const-stable functions behave, and if we are going to allow calling unmarked functions from const-stable functions we should also apply this check to unmarked functions.

Changing this breaks a load of stuff in std, as it has a load of these unmarked-unstable functions, and relies on them being able to call further const-unstable functions.

How bad would it be to mark those as #[rustc_const_unstable("const_internals")] or so?

I'd really prefer us to guard against accidental stabilization of const features. It's way too easy to screw that up. I am not very concerned about specifically HashMap::with_hasher as that function does basically nothing, I am concerned about all the other const fn we will have in the future that call into hashbrown (or into other crates). We have to get this right.

How bad would it be to mark those as #[rustc_const_unstable("const_internals")] or so?

It's 43 errors in core, can't see how many in std/alloc, so deffinatly doable if T-libs is fine with it.

I'll open a separate PR to do this (+ compiler changes), probably in a couple of weeks (exams).

Dylan-DPC · 2024-02-18T06:30:15Z

@aDotInTheVoid any updates on this pr? thanks

Dylan-DPC · 2024-04-16T10:58:28Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

aDotInTheVoid added 2 commits November 29, 2023 00:20

const-stabilize HashMap/HashSet::with_hasher

9318d0a

This needs to be `cfg(bootstrap)`ed, because we rely on `#[rustc_allow_const_fn_unstable(any)]`, which was introduced in the previous commit.

rustbot assigned m-ou-se Nov 29, 2023

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 29, 2023

This was referenced Nov 29, 2023

Tracking Issue for const_collections_with_hasher #102575

Closed

[WIP] const-stabilize Hash{Map/Set}::new #118423

Closed

RalfJung reviewed Nov 29, 2023

View reviewed changes

aDotInTheVoid added 2 commits December 1, 2023 15:06

any -> any_unmarked for rustc_allow_const_fn_unstable

f4136eb

Add test for calling const-unstable function with `-Zforce-unstable-i…

ec8f6af

…f-unmarked`

RalfJung reviewed Dec 1, 2023

View reviewed changes

Noratrieb 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 Jan 8, 2024

krtab mentioned this pull request Mar 29, 2024

Tracking Issue for fn const BuildHasherDefault::new() #123197

Closed

3 tasks

Dylan-DPC closed this Apr 16, 2024

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

const-stabilize HashMap/HashSet::with_hasher (+ required compiller changes) #118427

const-stabilize HashMap/HashSet::with_hasher (+ required compiller changes) #118427

aDotInTheVoid commented Nov 29, 2023

rustbot commented Nov 29, 2023

RalfJung Nov 29, 2023

RalfJung Nov 29, 2023 •

edited

Loading

aDotInTheVoid Nov 29, 2023

RalfJung Nov 29, 2023 •

edited

Loading

aDotInTheVoid Nov 29, 2023

RalfJung Nov 29, 2023

aDotInTheVoid Nov 29, 2023

aDotInTheVoid Nov 29, 2023

RalfJung Nov 29, 2023

aDotInTheVoid Dec 1, 2023

RalfJung Dec 1, 2023 •

edited

Loading

aDotInTheVoid Dec 1, 2023

RalfJung Dec 2, 2023 •

edited

Loading

aDotInTheVoid Dec 5, 2023

Dylan-DPC commented Feb 18, 2024

Dylan-DPC commented Apr 16, 2024

	// FIXME(ecstaticmorse); For compatibility, we consider `unstable` callees that
	// have no `rustc_const_stable` attributes to be const-unstable as well. This
	// should be fixed later.
	let callee_is_unstable_unmarked = tcx.lookup_const_stability(callee).is_none()
	&& tcx.lookup_stability(callee).is_some_and(\|s\| s.is_unstable());
	if callee_is_unstable_unmarked {

	// If the `-Z force-unstable-if-unmarked` flag is passed then we provide
	// a parent stability annotation which indicates that this is private
	// with the `rustc_private` feature. This is intended for use when
	// compiling `librustc_*` crates themselves so we can leverage crates.io
	// while maintaining the invariant that all sysroot crates are unstable
	// by default and are unable to be used.
	if tcx.sess.opts.unstable_opts.force_unstable_if_unmarked {
	let stability = Stability {
	level: attr::StabilityLevel::Unstable {
	reason: UnstableReason::Default,
	issue: NonZeroU32::new(27812),
	is_soft: false,
	implied_by: None,
	},
	feature: sym::rustc_private,
	};
	annotator.parent_stab = Some(stability);

const-stabilize HashMap/HashSet::with_hasher (+ required compiller changes) #118427

const-stabilize HashMap/HashSet::with_hasher (+ required compiller changes) #118427

Conversation

aDotInTheVoid commented Nov 29, 2023

rustbot commented Nov 29, 2023

Choose a reason for hiding this comment

RalfJung Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Nov 29, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dylan-DPC commented Feb 18, 2024

Dylan-DPC commented Apr 16, 2024

RalfJung Nov 29, 2023 •

edited

Loading

RalfJung Nov 29, 2023 •

edited

Loading

RalfJung Dec 1, 2023 •

edited

Loading

RalfJung Dec 2, 2023 •

edited

Loading