Skip to content

Make MapWrapper.Entry's hashCode conform to the contract in java.util.Map.Entry's documentation #6233

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

Merged
merged 1 commit into from
Feb 27, 2018

Conversation

isnotinvain
Copy link
Contributor

@isnotinvain isnotinvain commented Dec 19, 2017

See scala/bug#10663

I added a regression test, and I'd like to let the CI run the rest of the tests which I think I can do by opening this PR.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Dec 19, 2017
@SethTisue SethTisue added the WIP label Dec 19, 2017
@isnotinvain
Copy link
Contributor Author

The following test is failing: https://github.com/scala/scala/blob/2.13.x/test/files/run/t5880.scala

Which is actually a chi-square test to test that MapWrapper has a decent hash function, and was created as part of fixing https://issues.scala-lang.org/browse/SI-5880.

Unfortunately, I think the right thing to do is to delete this test, and reverse the decision made in SI-5880 to "fix" this as this less than optimal hashing behavior for java.util.Maps is actually the correct one. The bug reported in SI-5880 is that MapWrapper's hashcode can create collisions, but that's not actually a bug (some amount of collisions are always expected) and it seems we can't really do any better than what all the other java.util.Maps do.

@isnotinvain
Copy link
Contributor Author

@SethTisue I can't seem to figure out how to remove the WIP label, but this PR is ready to be reviewed / discussed now. Thanks!

@hrhino hrhino removed the WIP label Dec 19, 2017
@hrhino
Copy link
Contributor

hrhino commented Dec 19, 2017

If you squash your commits together, the Jenkins failure will go away.

@isnotinvain
Copy link
Contributor Author

@hrhino squashed!

Does anyone have any objections / feedback for this change?

@isnotinvain
Copy link
Contributor Author

I couldn't comment on https://issues.scala-lang.org/browse/SI-5880, but I'd like to cc the folks on that ticket:@komamitsu and @axel22 (hope I found the right github accounts)

@SethTisue
Copy link
Member

the old JIRA tickets were migrated to scala/bug, so SI-5880 is here now: scala/bug#5880

@isnotinvain
Copy link
Contributor Author

Thanks!

@komamitsu
Copy link

@isnotinvain Thanks for notifying me!

@isnotinvain
Copy link
Contributor Author

Hi @SethTisue @hrhino, what's the next step for getting this reviewed?

Should I also backport it to the the other 2.* branches?

Thanks!

@isnotinvain
Copy link
Contributor Author

Hello, what's the next step here?

Thanks,
Alex

@SethTisue
Copy link
Member

Should I also backport it to the the other 2.* branches?

we're no longer accepting 2.11.x PRs (unless they are exceptionally important)

this looks like a change that could reasonably target 2.12.x. feel free to retarget the PR to 2.12.x

as for 2.13.x, normally we merge everything forward from 2.12.x to 2.13.x. but for 2.13.x there is a special situation w/r/t the collections API; in 2.13 the collections code will be replaced entirely by https://github.com/scala/collection-strawman . so if this PR gets merged here, the next step will be to submit an equivalent PR in that repo.

@Ichoran do you have time to review the content of this PR...?

@SethTisue SethTisue requested a review from Ichoran January 17, 2018 23:11
@isnotinvain
Copy link
Contributor Author

Happy to change the base to 2.12.x, maybe I can wait for some review feedback just in case that messes up the diff.

@isnotinvain
Copy link
Contributor Author

@Ichoran sorry to bug you, have you had a chance to take a look at this?

@isnotinvain
Copy link
Contributor Author

Sorry to keep bumping this, I just don't want to forget about it. Thanks!

@msolomon
Copy link

I could use this patch, anything I can do to help?

@Ichoran
Copy link
Contributor

Ichoran commented Feb 23, 2018

@isnotinvain - Sorry, I'll get to this over the weekend--probably tonight. (I get so many notifications I didn't notice that I was on listed for this one until Seth told me personally yesterday.)

@isnotinvain
Copy link
Contributor Author

Thanks @Ichoran !

Copy link
Contributor

@Ichoran Ichoran left a comment

Choose a reason for hiding this comment

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

I agree that given the javadocs for java.util.Map.Entry#hashCode, this must be the implementation. Thus, the code change is correct. Also, the test for being well-distributed is no longer appropriate.

override def hashCode = byteswap32(k.##) + (byteswap32(v.##) << 16)

/**
* It's important that this implementation conform to the contract
Copy link
Contributor

@Ichoran Ichoran Feb 25, 2018

Choose a reason for hiding this comment

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

This shouldn't be a scaladoc comment. It is to implementors. (Users don't have control over what hashCode is used.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK -- what's preferred:

// this kind
// of comment

or

/* This
  * kind
  */

Copy link
Member

Choose a reason for hiding this comment

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

doesn't matter, I don't think we have a consistent style on that. personally, I tend to like // because it makes it easier to see that something definitely isn't a Scaladoc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will do, thanks

@Ichoran
Copy link
Contributor

Ichoran commented Feb 25, 2018

Aside from the comment explaining things not working very well as a doc comment, the PR looks good.

@isnotinvain
Copy link
Contributor Author

OK, I've updated the comments.

Should I squash the two commits into 1? Do I need to backport these changes to any other branches?

Thanks!

@isnotinvain
Copy link
Contributor Author

Re-reading @SethTisue 's earlier comment, i can rebase this onto the 2.12 branch, and then send a similar PR to https://github.com/scala/collection-strawman. Should I squash this down to 1 commit or leave it as is?

@isnotinvain isnotinvain changed the base branch from 2.13.x to 2.12.x February 26, 2018 19:50
@isnotinvain isnotinvain changed the base branch from 2.12.x to 2.13.x February 26, 2018 19:51
@isnotinvain isnotinvain changed the base branch from 2.13.x to 2.12.x February 26, 2018 19:52
@isnotinvain
Copy link
Contributor Author

OK, squashed + rebased to 2.12.x -- assuming the tests etc pass I think this is ready to go. Thanks!

@isnotinvain
Copy link
Contributor Author

The validate-main check actually passed if you click on "Details" not sure why it hasn't updated in the UI here.

@SethTisue SethTisue modified the milestones: 2.13.0-M4, 2.12.5 Feb 27, 2018
@SethTisue SethTisue merged commit 088914b into scala:2.12.x Feb 27, 2018
@SethTisue
Copy link
Member

thank you Alex and Rex!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants