Skip to content

recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher #147171

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

Jump to bottom
Merged
bors merged 1 commit into from
Nov 19, 2025
Merged

recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher #147171

bors merged 1 commit into from
Nov 19, 2025

Conversation

@Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Sep 30, 2025 •
edited
Loading

closes #147147

The suggestion span is wrong, but I'm not sure how to find the right one. fixed

I'm relatively new to the diagnostics ecosystem, so I'm not sure if span_help is the right choice. span_suggestion_* might be better, but the output from x test looks weird in that case.
@rustbot 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 Sep 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2025

r? @fee1-dead

rustbot has assigned @fee1-dead.
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
@rust-log-analyzer

This comment has been minimized.

Sign in to view
fee1-dead
fee1-dead requested changes Oct 2, 2025
View reviewed changes
tests/ui/hashmap/hashset_generics.stderr Outdated
Comment on lines 10 to 13
note: the foreign item type `String` doesn't implement `BuildHasher`
--> $SRC_DIR/alloc/src/string.rs:LL:COL
|
= note: not implement `BuildHasher`
Copy link
Member

@fee1-dead fee1-dead Oct 1, 2025

Choose a reason for hiding this comment

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

note to self, this probably needs improvement.
tests/ui/hashmap/hashset_generics.rs
Copy link
Member

@fee1-dead fee1-dead Oct 2, 2025

Choose a reason for hiding this comment

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

Hmm. Could there be other tests that showcase when this help message could appear? For example when I try to insert, or when I try to create a new value of the HashSet type.

Also I think it might be better to put this under the less crowded tests/ui/hashmap directory.
@rustbot rustbot 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 Oct 2, 2025
@fee1-dead
Copy link
Member

fee1-dead commented Oct 7, 2025

Still waiting on other tests.

Also, use [@]rustbot ready if you want to resubmit for review
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Oct 15, 2025

I've added a test for the insert case. It seems pretty representative of most instances of this error, since the HashSet bounds are on methods. If we want to lint (not error) on HashSet construction with a second parameter that doesn't impl BuildHasher (and I'm not convinced we do), that's a separate conversation.

@rustbot ready
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2025
fee1-dead
fee1-dead requested changes Oct 15, 2025
View reviewed changes
tests/ui/hashmap/hashset_generics.stderr Outdated
Comment on lines 14 to 21
help: you might have intended to use a HashMap instead
--> $DIR/hashset_generics.rs:8:5
|
LL | #[derive(PartialEq)]
| --------- in this derive macro expansion
...
LL | pub parameters: HashSet<String, String>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

@fee1-dead fee1-dead Oct 15, 2025

Choose a reason for hiding this comment

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

Can we just use err.help() to avoid highlighting the same thing again?
@rustbot rustbot 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 Oct 15, 2025
@bors
Copy link
Collaborator

bors commented Oct 22, 2025

☔ The latest upstream changes (presumably #147957) made this pull request unmergeable. Please resolve the merge conflicts.
@rustbot
Copy link
Collaborator

rustbot commented Oct 30, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Oct 30, 2025

@rustbot ready
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2025
fee1-dead
fee1-dead requested changes Nov 4, 2025
View reviewed changes
@rustbot rustbot 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 Nov 4, 2025
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Nov 5, 2025

@rustbot ready
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2025
fee1-dead
fee1-dead requested changes Nov 9, 2025
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment •
edited
Loading

Choose a reason for hiding this comment

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

had one last nit, otherwise this is good to go!

View changes since this review
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated
Comment on lines 3085 to 3097
if foreign_preds.iter().any(|&(root_pred, pred)| {
if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(root_pred)) =
root_pred.kind().skip_binder()
&& let Some(root_adt) = root_pred.self_ty().ty_adt_def()
{
self.tcx.is_diagnostic_item(sym::HashSet, root_adt.did())
&& self.tcx.is_diagnostic_item(sym::BuildHasher, pred.def_id())
} else {
false
}
}) {
err.help("you might have intended to use a HashMap instead");
}
Copy link
Member

@fee1-dead fee1-dead Nov 9, 2025

Choose a reason for hiding this comment

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

Might be nice to extract this to a helper function that deduplicates the logic between this one and the one above, maybe named something like "suggest_hashmap_on_unsatisfied_hashset_buildhasher"
@rustbot rustbot 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 Nov 9, 2025
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Nov 18, 2025

@rustbot ready
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2025
@fee1-dead
Copy link
Member

fee1-dead commented Nov 18, 2025

@bors r+ rollup
@bors
Copy link
Collaborator

bors commented Nov 18, 2025

📌 Commit 754a82d has been approved by fee1-dead

It is now in the queue for this repository.
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 19, 2025
Merged
bors added a commit that referenced this pull request Nov 19, 2025
@bors
Auto merge of #149085 - matthiaskrgr:rollup-f8ia15e, r=matthiaskrgr
140044c 
Rollup of 7 pull requests Successful merges: - #147171 (recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher) - #147421 (Add check if span is from macro expansion) - #147521 (Make SIMD intrinsics available in `const`-contexts) - #148201 (Start documenting autodiff activities) - #148797 (feat: Add `bit_width` for unsigned `NonZero<T>`) - #148798 (Match <OsString as Debug>::fmt to that of str) - #149082 (autodiff: update formating, improve examples for the unstable-book) r? `@ghost` `@rustbot` modify labels: rollup
@bors bors merged commit 847c422 into rust-lang:main Nov 19, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Nov 19, 2025
rust-timer added a commit that referenced this pull request Nov 19, 2025
@rust-timer
Unrolled build for #147171
7a75fb0 
Rollup merge of #147171 - Qelxiros:hashmap_diag, r=fee1-dead recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher closes #147147 ~The suggestion span is wrong, but I'm not sure how to find the right one.~ fixed I'm relatively new to the diagnostics ecosystem, so I'm not sure if `span_help` is the right choice. `span_suggestion_*` might be better, but the output from `x test` looks weird in that case.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 20, 2025
@bors
Auto merge of #149085 - matthiaskrgr:rollup-f8ia15e, r=matthiaskrgr
9b024f0 
Rollup of 7 pull requests Successful merges: - rust-lang/rust#147171 (recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher) - rust-lang/rust#147421 (Add check if span is from macro expansion) - rust-lang/rust#147521 (Make SIMD intrinsics available in `const`-contexts) - rust-lang/rust#148201 (Start documenting autodiff activities) - rust-lang/rust#148797 (feat: Add `bit_width` for unsigned `NonZero<T>`) - rust-lang/rust#148798 (Match <OsString as Debug>::fmt to that of str) - rust-lang/rust#149082 (autodiff: update formating, improve examples for the unstable-book) r? `@ghost` `@rustbot` modify labels: rollup
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
@matthiaskrgr
Rollup merge of rust-lang#147171 - Qelxiros:hashmap_diag, r=fee1-dead
abde4a5 
recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher closes rust-lang#147147 ~The suggestion span is wrong, but I'm not sure how to find the right one.~ fixed I'm relatively new to the diagnostics ecosystem, so I'm not sure if `span_help` is the right choice. `span_suggestion_*` might be better, but the output from `x test` looks weird in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants

@Qelxiros @rustbot @rust-log-analyzer @fee1-dead @bors