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

Fix overriding in explicit nulls #13647

Merged
merged 6 commits into from Oct 6, 2021

Conversation

noti0na1
Copy link
Member

@noti0na1 noti0na1 commented Oct 1, 2021

Fix #13040 #13486 #13608.

During overriding check, Array[? <: FromJavaObject | Null].matchs(Array[? <: Object | Null]) is tested.
The TypeBounds ? <: FromJavaObject | Null is normalized with ? <: Any using & (in def isSubArg).
Since Any <:< FromJavaObject, the intersection result is arbitrary, and in this case, is ? <: Any.
Then, ? <: Any is no longer a subtype of ? <: Object | Null, and the match test fails.

This fix will handle FromJavaObject specially when computing the intersection of two TypeBounds. We will try to preserve the information of FromJavaObject in upper bounds.

// This & will try to preserve the FromJavaObjects type in upper bounds
// For example, (? <: FromJavaObjects | Null) & (? <: Any),
// we want to get (? <: FromJavaObjects | Null) intead of (? <: Any),
// because we may check FromJavaObjects | Null <:< Object | Null later.
def bounds_&(tp1: TypeBounds, tp2: TypeBounds) =
  if tp1.hi.containsFromJavaObject
    && (tp1.hi frozen_<:< tp2.hi)
    && (tp2.lo frozen_<:< tp1.lo) then
    // FromJavaObject in tp1.hi guarantees tp2.hi <:< tp1.hi
    // prefer tp1 if FromJavaObject is in its hi
    tp1
  else if tp2.hi.containsFromJavaObject
    && (tp2.hi frozen_<:< tp1.hi)
    && (tp1.lo frozen_<:< tp2.lo) then
    // Similarly, prefer tp2 if FromJavaObject is in its hi
    tp2
  else
    // Use regular & to solve other cases
    tp1 & tp2

@noti0na1
Copy link
Member Author

noti0na1 commented Oct 1, 2021

At https://github.com/lampepfl/dotty/blob/01f040bbf7b703a7aa0e8dc03799e0e142073410/compiler/src/dotty/tools/dotc/typer/RefChecks.scala#L564 , would is be better to write impl.matchesLoosely(mbrDenot, alwaysCompareTypes = true)?

According to def matchesType in TypeComparer.scala, "A function implementing tp1 matches tp2", although the result should be same.

@smarter
Copy link
Member

smarter commented Oct 5, 2021

although the result should be same.

Yes, matching should be symmetric, if it isn't it's a bug, so the order doesn't matter.

@smarter
Copy link
Member

smarter commented Oct 5, 2021

Maybe we're looking at this the wrong way? FromJavaObject is defined as a type alias of Object (https://github.com/lampepfl/dotty/blob/eb8773ecbedd32b7f7f6822696f3c3df3cde6930/compiler/src/dotty/tools/dotc/core/Definitions.scala#L407), but maybe it should be a type alias of Object | Null with explicit nulls?

@noti0na1
Copy link
Member Author

noti0na1 commented Oct 5, 2021

@smarter We don't need to change the FromJavaObject alias in explicit nulls, because we are going nullify the types from Java anyway.

I added some logic in glb and lub now to handle FromJavaObject union.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Thank you!

@smarter smarter enabled auto-merge October 6, 2021 14:28
@smarter smarter merged commit 307fcc4 into scala:master Oct 6, 2021
@smarter smarter deleted the explicit-nulls-override branch October 6, 2021 15:34
@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment