Remove unnecessary bounds for some Hash{Map,Set} methods

[upsuper]

This PR moves HashMap::{into_keys,into_values,retain} and HashSet::retain from impl blocks with K: Eq + Hash, S: BuildHasher into the blocks without them. It doesn't seem to me there is any reason these methods need to be bounded by that. This change brings HashMap::{into_keys,into_values} on par with HashMap::{keys,values,values_mut} which are not bounded either.

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[kennytm]

i think we need an FCP like #79245

[upsuper]

r? @dtolnay

[bors]

☔ The latest upstream changes (presumably #91760) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #91761) made this pull request unmergeable. Please resolve the merge conflicts.

[Folyd]

I'm in favor of the K: Eq + Hash, S: BuildHasher bound is unnecessary. BTW, don't forget the BTreeMap/Set::into_key/values().

[dtolnay]

into_keys and into_values seems straightforward to me, since IntoIterator for HashMap and IntoIterator for &HashMap already do not require any bound to iterate, and these functions are doing even less because they don't even iterate, just stick self inside of an iterator struct, which already has no bounds on its Iterator impl.

retain is not obvious to me. Is there an easy way to see why this wouldn't need to rehash items as part of shrinking the map, in any hash map implementation?

[upsuper]

retain is not obvious to me. Is there an easy way to see why this wouldn't need to rehash items as part of shrinking the map, in any hash map implementation?

It's a good question. I don't think retain should ever shrink the hash map in capacity. It doesn't now, nor do retains of other collections, and as long as we want to guarantee deletion of a single entry wouldn't trigger a reallocation, the same guarantee should always apply to retain as well, since retain is just a batch deletion.

The question then fall into whether we ever need to rehash anything for deletion of an entry. I feel this is hard, if not impossible, to prove, as there is an open design space. However, it may not matter here, because in std's API, OccupiedEntry::remove_entry is stable and doesn't have the bounds, so any implementation that Rust can be using would likely be restricted by this API, and is unable to do any rehash when removing a single entry. (TBH, as you can't get an occupied entry without going through a function bounded by the traits, theoretically it's possible to pre-compute all needed rehashes and store it inside OccupiedEntry, but that's likely a lot of unnecessary computation, and would probably be a violation of zero cost principle.)

Since retain is semantically just a combination of

• going through each occupied entry, which HashMap::iter_mut is already doing without the bounds, and
• conditionally delete entries occurred, which OccupiedEntry::remove_entry can do without the bounds

I believe it wouldn't ever need the bounds.

Also, if the bounds here are deemed not to be dropped for retain, we should also move the unstable HashMap::drain_filter (tracked in #59618) into the bounded impl block before stabilization, since it's a more powerful version of retain.

[dtolnay]

Let's get the rest of the team to weigh in.

  impl<K, V, S> HashMap<K, V, S> {
      pub fn into_keys(self) -> IntoKeys<K, V>
      where
-         K: Eq + Hash,
-         S: BuildHasher;

      pub fn into_values(self) -> IntoValues<K, V>
      where
-         K: Eq + Hash,
-         S: BuildHasher;

      pub fn retain<F>(&mut self, f: F)
      where
-         K: Eq + Hash,
-         S: BuildHasher,
          F: FnMut(&K, &mut V) -> bool;
  }

  impl<T, S> HashSet<T, S> {
      pub fn retain<F>(&mut self, f: F)
      where
-         T: Eq + Hash,
-         S: BuildHasher,
          F: FnMut(&T) -> bool;
  }

@rfcbot fcp merge

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

Interesting! I agree that remove_entry is effectively constraining possible implementations here; good catch. That does limit some hash table designs, but those designs are already limited.

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[dtolnay]

Thanks!

[dtolnay]

@bors r+

[bors]

📌 Commit fb1e031 has been approved by dtolnay