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

Preserve null policy in wrapped Java Maps #10129

Merged
merged 1 commit into from Sep 13, 2022

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 3, 2022

When using compute, which has "remove" semantics for null value,
try alternative means when user wants to put a null. In particular,
if the underlying map wants to throw NPE, the fallback should do so.

Follow-up to #10027

@scala-jenkins scala-jenkins added this to the 2.13.10 milestone Sep 3, 2022
@SethTisue SethTisue requested a review from a team Sep 3, 2022
@SethTisue SethTisue added the library:collections PRs involving changes to the standard collection library label Sep 3, 2022
override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op)
override def getOrElseUpdate(key: K, op: => V): V =
underlying.computeIfAbsent(key, _ => op) match {
case null => super.getOrElseUpdate(key, op)
Copy link
Member

@lrytz lrytz Sep 5, 2022

Choose a reason for hiding this comment

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

would be good to clarify super[Which] here (and below)

Copy link
Member

@lrytz lrytz Sep 12, 2022

Choose a reason for hiding this comment

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

I see, super[concurrent.Map].getOrElseUpdate is not allowed, interesting language limitation.

lrytz
lrytz approved these changes Sep 12, 2022
Copy link
Member

@lrytz lrytz left a comment

It seems null handling of JConcurrentMapWrapper.putIfAbsent is not aligned with Scala's semantics, if the underlying map contained a binding with value null, it will return None. Would it make sense to fix this?

I guess it's not too important as both concurrent map implementations in the JDK don't support null. Is there a known / commonly used one that does?

LGTM

override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op)
override def getOrElseUpdate(key: K, op: => V): V =
underlying.computeIfAbsent(key, _ => op) match {
case null => super.getOrElseUpdate(key, op)
Copy link
Member

@lrytz lrytz Sep 12, 2022

Choose a reason for hiding this comment

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

I see, super[concurrent.Map].getOrElseUpdate is not allowed, interesting language limitation.

}
try Option(underlying.compute(key, remap))
catch {
case PutNull => super.updateWith(key)(remappingFunction)
Copy link
Member

@lrytz lrytz Sep 12, 2022

Choose a reason for hiding this comment

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

So the dance is between

  • supporting Scala semantics for updateWith (can put Some(null))
  • supporting the underlying map's contract (the ConcurrentHashMap.compute method evaluates the argument function only once)

and we can't have both, so for Some(null) the function will re-evaluate. That looks fine to me.

@lrytz
Copy link
Member

lrytz commented Sep 13, 2022

@som-snytt I pushed a commit to remove the getOrElseUpdate and updateWith overrides from non-concurrent map wrappers. Let me know if I missed something, if there was actually a reason for them besides making the wrapper implementations more aligned.

@som-snytt
Copy link
Contributor Author

som-snytt commented Sep 13, 2022

I think the non-concurrent case was for efficiency.

When using `compute`, which has "remove" semantics for `null` value,
try alternative means when user wants to put a `null`. In particular,
if the underlying map wants to throw NPE, the fallback should do so.
@lrytz lrytz force-pushed the followup/12586-preserve-NPE branch from 7abe9b8 to b824b84 Compare Sep 13, 2022
@lrytz
Copy link
Member

lrytz commented Sep 13, 2022

Ah, that's a good reason 👍 thanks.

@lrytz lrytz merged commit 986dcc1 into scala:2.13.x Sep 13, 2022
3 checks passed
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Sep 13, 2022
@som-snytt som-snytt deleted the followup/12586-preserve-NPE branch Sep 13, 2022
@SethTisue SethTisue changed the title Preserve null policy in wrapped Java Map Preserve null policy in wrapped Java Maps Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:collections PRs involving changes to the standard collection library release-notes worth highlighting in next release notes
4 participants