Implement hash_map macro

[stifskere]

Implementation of #144032

Implements the hash_map macro under std/src/macros.rs.

[rustbot]

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[NyxCode]

Wouldn't using one of HashMap::with_capacity(), HashMap::reserve(), FromIterator<(K, V)>, Extend<(K, V)> or From<[(K, V); N]> be better, avoiding unnecessary allocations?

[stifskere]

Wouldn't using one of HashMap::with_capacity(), HashMap::reserve(), FromIterator<(K, V)>, Extend<(K, V)> or From<[(K, V); N]> be better, avoiding unnecessary allocations?

I was thinking about HashMap::with_capacity() but I couldn't see how I could convert a repetition into a number.

From... would essentially be the same, just reallocation, so for clarity I just expanded into insert.

I could see that 0 $(+ 1)* could be evaluated constantly by the compiler, thus we could get a size, but I'm not sure if it's the best way.
It could not be the best way, because that expansion doesn't at all include a reference to the repetition declaration, so it can't be. There is no logical way of getting the size with a macro_rules unless with more complexity we make a recursive macro, which would limit the hash_map size to 254 without extra compiler parameters.

I will add a question about the size in the tracking issue.

[NyxCode]

All the options i mentioned should be equivalent to with_capacity as far as I can tell, though I don't know if the optimizer can look through all options equally well (e.g. wrt. stack usage).
Both these should work, for example:

let len = 1 $(+ {stringify!($k); 1})*;
let mut map = HashMap::with_capacity(len);
$( map.insert($k, $v); )*

HashMap::from([$(($k, $v)),*])

[stifskere]

All the options i mentioned should be equivalent to with_capacity as far as I can tell, though I don't know if the optimizer can look through all options equally well (e.g. wrt. stack usage). Both these should work, for example:

let len = 1 $(+ {stringify!($k); 1})*;
let mut map = HashMap::with_capacity(len);
$( map.insert($k, $v); )*

HashMap::from([$(($k, $v)),*])

I think I'd rather go for intrinsics, except since HashMap is a wrapper, I don't think there are many. I feel that if someone decides to expand the macro via IDE or cargo expand, they will find a lot of strings and be like "???". Probably open an issue later.

Otherwise, we could generate a constant count probably, the compiler should optimize this, and it should not have runtime cost, but a bit extra of constant cost, which may mean that if there are a lot of invocations the compiler might go a bit slower.

macro_rules! hash_map {
    () => {{
        ::std::collections::HashMap::new()
    }};

    ( $( $key:expr => $value:expr ),* $(,)? ) => {{
        let mut map = ::std::collections::HashMap::with_capacity(
            const { [$({ let _ = &$key; () }),*].len() }
        );

        $( map.insert($key, $value); )*
        map
    }}
}

[Noratrieb]

I don't think intrinsics make sense here, this is a library type. I think the repeat insert method makes the most sense as it doesn't have the potential stack usage problem of from. Repeat insert is also how extend is implemented under the hood: https://github.com/rust-lang/hashbrown/blob/670213fb32d208759d0331996894e05604de0c18/src/map.rs#L4562.

with_capacity definitely makes sense though. You can count the elements like this: https://lukaswirth.dev/tlborm/decl-macros/building-blocks/counting.html

[Noratrieb]

Oh actually you might be able to use the unstable macro_metavar_expr_count for the counting too, I forgot about that :D

[stifskere]

I don't think intrinsics make sense here, this is a library type. I think the repeat insert method makes the most sense as it doesn't have the potential stack usage problem of from. Repeat insert is also how extend is implemented under the hood: https://github.com/rust-lang/hashbrown/blob/670213fb32d208759d0331996894e05604de0c18/src/map.rs#L4562.

with_capacity definitely makes sense though. You can count the elements like this: https://lukaswirth.dev/tlborm/decl-macros/building-blocks/counting.html

I would go for the slice length approach, but resolved constantly, which is technically what I'm doing just that instead of making a variable using replace_expr!

So probably

macro_rules! hash_map {
    () => {{
        ::std::collections::HashMap::new()
    }};

    ( $( $key:expr => $value:expr ),* $(,)? ) => {{
        let mut map = ::std::collections::HashMap::with_capacity(
            const { [$($crate::hash_map!(@internal count $key)),*].len() }
        );

        $( map.insert($key, $value); )*
        map
    }};

    (@internal count $i:tt) => {()}
}

This would work best if there is some sort of thing inside the standard library so that the last branch is only used internally in an explicit way, which would also filter out the usage inside the second branch.

[Noratrieb]

Oh actually you might be able to use the unstable macro_metavar_expr_count for the counting too, I forgot about that :D

You should try this, which I only remembered after posting the first message

[stifskere]

Oh actually you might be able to use the unstable macro_metavar_expr_count for the counting too, I forgot about that :D

You should try this, which I only remembered after posting the first message

The problem with this is that it would delay stabilization a lot, so I'm not sure, I don't think adding unstable to unstable helps

[Noratrieb]

You're allowed to use unstable features internally with #[allow_internal_unstable()] and listing out the feature names there, on the macro.

[tmke8]

Would it be possible to use HashMap::default() instead of HashMap::new() so that this macro can be used with non-default hashing algorithms as well? (Though this might cause type inference problems...)

EDIT: or rather

HashMap::with_capacity_and_hasher(capacity, Default::default())

for specifying capacity.

[stifskere]

You're allowed to use unstable features internally with #[allow_internal_unstable()] and listing out the feature names there, on the macro.

Ok, I will check that now

[Noratrieb]

In this case you're just missing another allow_internal_unstable on your repetition_utils, since allow_internal_unstable isn't transitive:

diff --git a/library/std/src/macros.rs b/library/std/src/macros.rs
index 7055168e0a6..455d0ca9e24 100644
--- a/library/std/src/macros.rs
+++ b/library/std/src/macros.rs
@@ -383,6 +383,7 @@ macro_rules! dbg {
 #[doc(hidden)]
 #[macro_export]
 #[unstable(feature = "hash_map_internals", issue = "none")]
+#[allow_internal_unstable(hash_map_internals)]
 macro_rules! repetition_utils {
     (@count $($tokens:tt),*) => {{
         [$($crate::repetition_utils!(@replace $tokens => ())),*].len()

This makes it work for me.

[stifskere]

In this case you're just missing another allow_internal_unstable on your repetition_utils, since allow_internal_unstable isn't transitive:

diff --git a/library/std/src/macros.rs b/library/std/src/macros.rs
index 7055168e0a6..455d0ca9e24 100644
--- a/library/std/src/macros.rs
+++ b/library/std/src/macros.rs
@@ -383,6 +383,7 @@ macro_rules! dbg {
 #[doc(hidden)]
 #[macro_export]
 #[unstable(feature = "hash_map_internals", issue = "none")]
+#[allow_internal_unstable(hash_map_internals)]
 macro_rules! repetition_utils {
     (@count $($tokens:tt),*) => {{
         [$($crate::repetition_utils!(@replace $tokens => ())),*].len()

This makes it work for me.

Done, I added it, let's see if tests run correctly now.

By the way, what's the way you test whether tests fail and whether stability is correct locally? Because I would like to do the same instead of relying on running the workflows. I have read some about the ./x bootstrap, I need to figure out how to configure it locally.

[Noratrieb]

I can recommend reading this to see how to build and run the compiler with a standard library: https://rustc-dev-guide.rust-lang.org/building/quickstart.html
I did ./x build --stage 1 library to build the compiler and have it linked up as mentioned in the guide to then be able to do rustc +dev mytest.rs to test building the file (where I invoke the macro).

[Noratrieb]

(somehow github doesn't show it nicely despite me leaving it as a review comment but i added a comment here in case it got lost in the ui: #144070 (comment))

[stifskere]

(somehow github doesn't show it nicely despite me leaving it as a review comment but i added a comment here in case it got lost in the ui: #144070 (comment))

I believe it didn't show up because it was solved, I didn't see the comment at all, If you could re-review the actual documentation, I changed it a bit.

[Noratrieb]

I see yeah, I left the comment a few minutes ago so it was with the latest state of your changes

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[Noratrieb]

Thanks!
@bors r+

[bors]

📌 Commit 066023e has been approved by Noratrieb

It is now in the queue for this repository.

[danielhenrymantilla]

Looks good!

🤔 I wonder how the implementation would compare to doing:

<
    $crate::collections::HashMap<_, _, $crate::hash::RandomState>
    as
    $crate::convert::From<[(_, _); _]>
>::from([
    $(
        ($key, $value)
    ),+ // empty case still handled as `new()`.
])

Intuitively, doing a batch of .insert()s does require the map to go back to a fully usable-from-the-outside state, whereas a direct batched constructor such as From<[(K, V); N]> does not, so depending on the impl, there might be more room for optimisation with this more direct approach 🤔

[danielhenrymantilla]

Very tiny nit 🙂:

Suggested change

	( $( $key:expr => $value:expr ),* $(,)? ) => {{
	( $( $key:expr => $value:expr ),+ $(,)? ) => {{

would make the intent clearer, and also reject hash_map!(,)

[Noratrieb]

that makes sense, I suggest addressing this in a follow-up PR

[danielhenrymantilla]

Suggested change

	$( map.insert($key, $value); )*
	$( map.insert($key, $value); )+

[Noratrieb]

I have checked the FromIterator implementations, they are not any more efficient than repeat inserts (but the array impl will blow the stack :D)

[danielhenrymantilla]

Thanks for looking into these 🙏

[stifskere]

I have checked the FromIterator implementations, they are not any more efficient than repeat inserts (but the array impl will blow the stack :D)

I was now checking about this you said, which feels weird, I thought that without recursiveness this wouldn't have a problem, so should I add another PR to the tracking issue? Wait for a comment period?