Allow binary HashSet operations to be generic over hashers #48363

varkor · 2018-02-20T01:35:09Z

Allow the two sets that are parameters in HashSet::{difference, intersection, is_disjoint, is_subset, is_superset} to have different hashing algorithms. Fixes #31712.

Allow the two sets that are parameters in `HashSet::{difference, intersection, is_disjoint, is_subset, is_superset}` to have different hashing algorithms. Fixes rust-lang#31712.

rust-highfive · 2018-02-20T01:35:12Z

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

pietroalbini · 2018-02-26T17:04:30Z

@bluss ping from triage! Can you (or someone else from @rust-lang/libs) review this PR?

alexcrichton · 2018-02-26T21:03:24Z

Thanks for the PR @varkor! This looks like a great idea to me.

Technically this is a breaking change due to the addition of a type parameter on the method (I think?) but I'm having trouble coming up with a realistic case where it'd actually cause breakage. In that sense I think we should be good to go to merge this, but let's check with...

@rfcbot fcp merge

rfcbot · 2018-02-26T21:03:24Z

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

sfackler · 2018-02-28T04:24:49Z

src/libstd/collections/hash/set.rs

    /// ```
    #[stable(feature = "rust1", since = "1.0.0")]
-    pub fn difference<'a>(&'a self, other: &'a HashSet<T, S>) -> Difference<'a, T, S> {
+    pub fn difference<'a, U>(&'a self, other: &'a HashSet<T, U>) -> Difference<'a, T, U> {


Should we uniformly require the BuildHasher bound here? Not sure if it would matter but it might be safer to add?

Yeah, I find it hard to reason about inference issues in cases like this. I agree that it probably is a good idea to make explicit.

pietroalbini · 2018-03-05T15:25:24Z

Ping from the release team! @sfackler and @withoutboats, there are some nice checkboxes waiting for you :)

rfcbot · 2018-03-05T20:54:38Z

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

withoutboats · 2018-03-05T20:54:51Z

Technically this is a breaking change due to the addition of a type parameter on the method (I think?) but I'm having trouble coming up with a realistic case where it'd actually cause breakage. In that sense I think we should be good to go to merge this, but let's check with...

Seems plausible to cause inference breakage, maybe we should do a crater run.

alexcrichton · 2018-03-05T21:45:49Z

@rust-lang/infra, mind doing a crater run for this?

kennytm · 2018-03-06T04:37:30Z

@bors try

bors · 2018-03-06T04:37:43Z

⌛ Trying commit 2e55a0b with merge beb6477...

…<try> Allow binary HashSet operations to be generic over hashers Allow the two sets that are parameters in `HashSet::{difference, intersection, is_disjoint, is_subset, is_superset}` to have different hashing algorithms. Fixes #31712.

bors · 2018-03-06T06:49:41Z

☀️ Test successful - status-travis
State: approved= try=True

pietroalbini · 2018-03-12T11:16:51Z

Added to the crater queue.

rfcbot · 2018-03-15T21:00:53Z

The final comment period is now complete.

aidanhs · 2018-03-20T01:31:30Z

Crater run started

aidanhs · 2018-03-26T11:43:11Z

Hi @alexcrichton (crater requester/PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-48363/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

kennytm · 2018-03-26T11:53:11Z

Some legit inference failures (only checked the last few):

pombase.pombase-chado-json.83d98003936c84b11f6eb9421bbe0e652f56c684

Mar 26 07:41:31.623 INFO kablam! error[E0282]: type annotations needed
Mar 26 07:41:31.623 INFO kablam!    --> src/pombase/api/query.rs:110:32
Mar 26 07:41:31.623 INFO kablam!     |
Mar 26 07:41:31.623 INFO kablam! 110 |     let mut current_gene_set = HashSet::from_iter(current_genes);
Mar 26 07:41:31.623 INFO kablam!     |         --------------------   ^^^^^^^^^^^^^^^^^^ cannot infer type for `S`
Mar 26 07:41:31.623 INFO kablam!     |         |
Mar 26 07:41:31.623 INFO kablam!     |         consider giving `current_gene_set` a type

marte.baraha.2f0407c1eeaa5973b3e9b207b55b34d8988f34aa

Mar 26 06:39:12.977 INFO kablam! error[E0282]: type annotations needed
Mar 26 06:39:12.977 INFO kablam!    --> src/game.rs:405:25
Mar 26 06:39:12.977 INFO kablam!     |
Mar 26 06:39:12.977 INFO kablam! 405 |         self.hands[p-1].is_superset(&cards.0.iter().cloned().collect())
Mar 26 06:39:12.977 INFO kablam!     |                         ^^^^^^^^^^^ cannot infer type for `U`

voidmap-1.1.2

Mar 25 20:42:41.973 INFO kablam! error[E0282]: type annotations needed
Mar 25 20:42:41.973 INFO kablam!     --> src/screen.rs:2119:49
Mar 25 20:42:41.973 INFO kablam!      |
Mar 25 20:42:41.973 INFO kablam! 2118 |                 let children_new = children.into_iter().collect();
Mar 25 20:42:41.973 INFO kablam!      |                     ------------ consider giving `children_new` a type
Mar 25 20:42:41.973 INFO kablam! 2119 |                 let intersection = children_acc.intersection(&children_new);
Mar 25 20:42:41.974 INFO kablam!      |                                                 ^^^^^^^^^^^^ cannot infer type for `U`

alexcrichton · 2018-03-26T20:14:07Z

Alas! Looks like we won't be able to land this :(

alexcrichton · 2018-03-26T20:14:21Z

Thanks for the crater run @aidanhs and the analysis @kennytm!

leoyvens · 2018-03-26T20:20:07Z

I believe this is a "making an already generic parameter more generic" case that could be possible with type parameter fallback.

varkor · 2018-03-26T20:24:35Z

@leodasvacas: yes, this is precisely what I was thinking. This PR is something to revisit once we have something like that in place.

Allow binary HashSet operations to be generic over hashers

84898e7

Allow the two sets that are parameters in `HashSet::{difference, intersection, is_disjoint, is_subset, is_superset}` to have different hashing algorithms. Fixes rust-lang#31712.

rust-highfive assigned bluss Feb 20, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2018

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 26, 2018

alexcrichton assigned alexcrichton and unassigned bluss Feb 26, 2018

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 26, 2018

alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 26, 2018

sfackler reviewed Feb 28, 2018

View reviewed changes

Explicate BuildHasher bound on difference and intersection

2e55a0b

pietroalbini added S-waiting-on-team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2018

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 5, 2018

alexcrichton added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-team labels Mar 5, 2018

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 26, 2018

alexcrichton closed this Mar 26, 2018

alexcrichton reopened this Mar 26, 2018

alexcrichton closed this Mar 26, 2018

Uh oh!

Allow binary HashSet operations to be generic over hashers #48363

Allow binary HashSet operations to be generic over hashers #48363

Uh oh!

Conversation

varkor commented Feb 20, 2018

Uh oh!

rust-highfive commented Feb 20, 2018

Uh oh!

pietroalbini commented Feb 26, 2018

Uh oh!

alexcrichton commented Feb 26, 2018

Uh oh!

rfcbot commented Feb 26, 2018 • edited by withoutboats Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfackler Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

varkor Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

pietroalbini commented Mar 5, 2018

Uh oh!

rfcbot commented Mar 5, 2018

Uh oh!

withoutboats commented Mar 5, 2018

Uh oh!

alexcrichton commented Mar 5, 2018

Uh oh!

kennytm commented Mar 6, 2018

Uh oh!

bors commented Mar 6, 2018

Uh oh!

bors commented Mar 6, 2018

Uh oh!

pietroalbini commented Mar 12, 2018

Uh oh!

rfcbot commented Mar 15, 2018

Uh oh!

aidanhs commented Mar 20, 2018

Uh oh!

aidanhs commented Mar 26, 2018

Uh oh!

kennytm commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Mar 26, 2018

Uh oh!

alexcrichton commented Mar 26, 2018

Uh oh!

leoyvens commented Mar 26, 2018

Uh oh!

varkor commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

rfcbot commented Feb 26, 2018 •

edited by withoutboats

Loading

kennytm commented Mar 26, 2018 •

edited

Loading

varkor commented Mar 26, 2018 •

edited

Loading