Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec12fea
const_eval: Allow std to stabley call const fns from deps.
aDotInTheVoid Nov 29, 2023
9318d0a
const-stabilize HashMap/HashSet::with_hasher
aDotInTheVoid Nov 29, 2023
f4136eb
`any` -> `any_unmarked` for `rustc_allow_const_fn_unstable`
aDotInTheVoid Dec 1, 2023
ec8f6af
Add test for calling const-unstable function with `-Zforce-unstable-i…
aDotInTheVoid Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion compiler/rustc_const_eval/src/transform/check_consts/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,9 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
// 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 {
let can_call_any_function =
super::rustc_allow_const_fn_unstable(tcx, caller, sym::any);
if callee_is_unstable_unmarked && !can_call_any_function {
trace!("callee_is_unstable_unmarked");
// We do not use `const` modifiers for intrinsic "functions", as intrinsics are
// `extern` functions, and these have no way to get marked `const`. So instead we
Expand Down
13 changes: 12 additions & 1 deletion library/std/src/collections/hash/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,18 @@ impl<K, V, S> HashMap<K, V, S> {
/// ```
#[inline]
#[stable(feature = "hashmap_build_hasher", since = "1.7.0")]
#[rustc_const_unstable(feature = "const_collections_with_hasher", issue = "102575")]
#[cfg_attr(
bootstrap,
rustc_const_unstable(feature = "const_collections_with_hasher", issue = "102575")
)]
#[cfg_attr(
not(bootstrap),
rustc_const_stable(
feature = "const_collections_with_hasher",
since = "CURRENT_RUSTC_VERSION"
)
)]
#[rustc_allow_const_fn_unstable(any)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@RalfJung RalfJung Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@RalfJung RalfJung Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub const fn with_hasher(hash_builder: S) -> HashMap<K, V, S> {
HashMap { base: base::HashMap::with_hasher(hash_builder) }
}
Expand Down
13 changes: 12 additions & 1 deletion library/std/src/collections/hash/set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,18 @@ impl<T, S> HashSet<T, S> {
/// ```
#[inline]
#[stable(feature = "hashmap_build_hasher", since = "1.7.0")]
#[rustc_const_unstable(feature = "const_collections_with_hasher", issue = "102575")]
#[cfg_attr(
bootstrap,
rustc_const_unstable(feature = "const_collections_with_hasher", issue = "102575")
)]
#[cfg_attr(
not(bootstrap),
rustc_const_stable(
feature = "const_collections_with_hasher",
since = "CURRENT_RUSTC_VERSION"
)
)]
#[rustc_allow_const_fn_unstable(any)]
pub const fn with_hasher(hasher: S) -> HashSet<T, S> {
HashSet { base: base::HashSet::with_hasher(hasher) }
}
Expand Down
3 changes: 2 additions & 1 deletion library/std/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@
#![feature(no_sanitize)]
#![feature(platform_intrinsics)]
#![feature(prelude_import)]
#![feature(rustc_allow_const_fn_unstable)]
#![feature(rustc_attrs)]
#![feature(rustdoc_internals)]
#![feature(staged_api)]
Expand Down Expand Up @@ -388,7 +389,7 @@
//
// Only for const-ness:
// tidy-alphabetical-start
#![feature(const_collections_with_hasher)]
#![cfg_attr(bootstrap, feature(const_collections_with_hasher))]
#![feature(const_hash)]
#![feature(const_io_structs)]
#![feature(const_ip)]
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/stability-attribute/auxiliary/normal-const-fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// compile-flags: -Zforce-unstable-if-unmarked

// This emulates a dep-of-std (eg hashbrown), that has const functions it
// cannot mark as stable, and is build with force-unstable-if-unmarked.

pub const fn do_something_else() {}
22 changes: 22 additions & 0 deletions tests/ui/stability-attribute/const-stable-cross-crate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// aux-build:normal-const-fn.rs
// check-pass
#![crate_type = "lib"]
#![feature(staged_api)]
#![feature(rustc_attrs)]
#![feature(rustc_private)]
#![allow(internal_features)]
#![feature(rustc_allow_const_fn_unstable)]
#![stable(feature = "stable_feature", since = "1.0.0")]

extern crate normal_const_fn;

// This ensures std can call const functions in it's deps that don't have
// access to rustc_const_stable annotations (and hense don't have a feature)
// gate.

#[rustc_const_stable(feature = "stable_feature", since = "1.0.0")]
#[stable(feature = "stable_feature", since = "1.0.0")]
#[rustc_allow_const_fn_unstable(any)]
pub const fn do_something() {
normal_const_fn::do_something_else()
}