Skip to content
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.

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

HashSet<K, V> has confusing errors thanks to lax BuildHasher bounds #123018

Open
kornelski opened this issue Mar 24, 2024 · 2 comments
Open

HashSet<K, V> has confusing errors thanks to lax BuildHasher bounds #123018

kornelski opened this issue Mar 24, 2024 · 2 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@kornelski
Copy link
Contributor

kornelski commented Mar 24, 2024

Code

use std::collections::*;

fn make_map() -> HashSet<String, String> {
    Default::default()
}

fn main() {
    let map = make_map();
    
    // compiles!
    for pair in &map { 
    }

    // expected `String`, found `(_, _)`
    for (k, v) in map {
    }
}

Writing HashSet instead of HashMap does not cause immediate error, because HashSet is allowed to have two type arguments, and very few methods require it to have a specific type.

Current output

playground

13 |     for (k, v) in map {
   |         ^^^^^^    --- this is an iterator with items of type `String`
   |         |
   |         expected `String`, found `(_, _)`

Desired output

Ideally Rust should check early that the second type in HashSet is supposed to be a BuildHasher, and warn about that as soon as HashSet<_, InvalidType> is used.

Rationale and extra context

Lack of bound on HashSet itself + type inference make incorrect the type compile in many cases, producing either surprising behavior or confusing errors far away from the mistake.

Other cases

Similar to #87015

It's not possible to add actual trait bounds on HashSet, but maybe the compiler could be somehow informed that these bounds are always expected to be satisfied, and warn whenever they're not.

Rust Version

rustc 1.79.0-nightly (3c85e56 2024-03-18)

@kornelski kornelski added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 24, 2024
@ChayimFriedman2
Copy link
Contributor

If we add such lint, we probably want to limit it to concrete types, to not create ecosystem churn, at which point it will be nice to expose this lint to other crates and types that want to have a "soft" limit on types but not force generic functions to add unnecessary bounds.

@krtab
Copy link
Contributor

krtab commented Mar 31, 2024

I think we may leverage one difference with #87015 to our advantage. If a lot of code is indeed polymorphic in K and/or V, I think that very little code is actually written outside of the standard lib that keeps a generic BuilHasher parameter.

I think that a possible solution would hence be to add bounds to HashMap::default and new that enforce that the third parameter is actually a BuildHasher, and do a crater run on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants