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 override containing type param in explicit nulls #13975

Merged
merged 7 commits into from
Feb 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Contexts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,9 @@ object Contexts {

def withNotNullInfos(infos: List[NotNullInfo]): Context =
if c.notNullInfos eq infos then c else c.fresh.setNotNullInfos(infos)

def relaxedOverrideContext: Context =
c.withModeBits(c.mode &~ Mode.SafeNulls | Mode.RelaxedOverriding)
end ops

// TODO: Fix issue when converting ModeChanges and FreshModeChanges to extension givens
Expand Down
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Mode.scala
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,9 @@ object Mode {
* This mode forces expansion of inline calls in those positions even during typing.
*/
val ForceInline: Mode = newMode(29, "ForceInline")

noti0na1 marked this conversation as resolved.
Show resolved Hide resolved
/** This mode is enabled when we check Java overriding in explicit nulls.
* Type `Null` becomes a subtype of non-primitive value types in TypeComparer.
*/
val RelaxedOverriding: Mode = newMode(30, "RelaxedOverriding")
}
16 changes: 13 additions & 3 deletions compiler/src/dotty/tools/dotc/core/TypeComparer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -766,14 +766,24 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling

isSubType(hi1, tp2, approx.addLow) || compareGADT || tryLiftedToThis1
case _ =>
def isNullable(tp: Type): Boolean = tp.widenDealias match {
case tp: TypeRef => tp.symbol.isNullableClass
// `Mode.RelaxedOverriding` is only enabled when checking Java overriding
// in explicit nulls, and `Null` becomes a bottom type, which allows
// `T | Null` being a subtype of `T`.
// A type variable `T` from Java is translated to `T >: Nothing <: Any`.
// However, `null` can always be a value of `T` for Java side.
// So the best solution here is to let `Null` be a subtype of non-primitive
// value types temporarily.
def isNullable(tp: Type): Boolean = tp.widenDealias match
case tp: TypeRef =>
val tpSym = tp.symbol
ctx.mode.is(Mode.RelaxedOverriding) && !tpSym.isPrimitiveValueClass ||
tpSym.isNullableClass
case tp: RefinedOrRecType => isNullable(tp.parent)
case tp: AppliedType => isNullable(tp.tycon)
case AndType(tp1, tp2) => isNullable(tp1) && isNullable(tp2)
case OrType(tp1, tp2) => isNullable(tp1) || isNullable(tp2)
case _ => false
}

val sym1 = tp1.symbol
(sym1 eq NothingClass) && tp2.isValueTypeOrLambda ||
(sym1 eq NullClass) && isNullable(tp2)
Expand Down
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1112,8 +1112,8 @@ object Types {
*/
def matches(that: Type)(using Context): Boolean = {
record("matches")
withoutMode(Mode.SafeNulls)(
TypeComparer.matchesType(this, that, relaxed = !ctx.phase.erasedTypes))
val overrideCtx = if ctx.explicitNulls then ctx.relaxedOverrideContext else ctx
TypeComparer.matchesType(this, that, relaxed = !ctx.phase.erasedTypes)(using overrideCtx)
}

/** This is the same as `matches` except that it also matches => T with T and
Expand Down
11 changes: 5 additions & 6 deletions compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package transform
import core._
import Flags._, Symbols._, Contexts._, Scopes._, Decorators._, Types.Type
import NameKinds.DefaultGetterName
import NullOpsDecorator._
import collection.mutable
import collection.immutable.BitSet
import scala.annotation.tailrec
Expand Down Expand Up @@ -216,14 +217,12 @@ object OverridingPairs:
)
else
// releaxed override check for explicit nulls if one of the symbols is Java defined,
// force `Null` being a subtype of reference types during override checking
val relaxedCtxForNulls =
if ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined)) then
ctx.retractMode(Mode.SafeNulls)
else ctx
// force `Null` to be a subtype of non-primitive value types during override checking.
val overrideCtx = if ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined))
then ctx.relaxedOverrideContext else ctx
member.name.is(DefaultGetterName) // default getters are not checked for compatibility
|| memberTp.overrides(otherTp,
member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack
)(using relaxedCtxForNulls)
)(using overrideCtx)

end OverridingPairs
14 changes: 6 additions & 8 deletions compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import Names._
import StdNames._
import NameOps._
import NameKinds._
import NullOpsDecorator._
import ResolveSuper._
import reporting.IllegalSuperAccessor

Expand Down Expand Up @@ -111,15 +112,12 @@ object ResolveSuper {
val otherTp = other.asSeenFrom(base.typeRef).info
val accTp = acc.asSeenFrom(base.typeRef).info
// Since the super class can be Java defined,
// we use releaxed overriding check for explicit nulls if one of the symbols is Java defined.
// This forces `Null` being a subtype of reference types during override checking.
val relaxedCtxForNulls =
if ctx.explicitNulls && (sym.is(JavaDefined) || acc.is(JavaDefined)) then
ctx.retractMode(Mode.SafeNulls)
else ctx
if (!(otherTp.overrides(accTp, matchLoosely = true)(using relaxedCtxForNulls)))
// we use relaxed overriding check for explicit nulls if one of the symbols is Java defined.
// This forces `Null` to be a subtype of non-primitive value types during override checking.
val overrideCtx = if ctx.explicitNulls && (sym.is(JavaDefined) || acc.is(JavaDefined))
then ctx.relaxedOverrideContext else ctx
if !otherTp.overrides(accTp, matchLoosely = true)(using overrideCtx) then
report.error(IllegalSuperAccessor(base, memberName, targetName, acc, accTp, other.symbol, otherTp), base.srcPos)

bcs = bcs.tail
}
assert(sym.exists, i"cannot rebind $acc, ${acc.targetName} $memberName")
Expand Down
47 changes: 47 additions & 0 deletions tests/explicit-nulls/neg/opaque-nullable.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Unboxed option type using unions + null + opaque.
// Relies on the fact that Null is not a subtype of AnyRef.
// Test suggested by Sébastien Doeraene.

object Nullables {
opaque type Nullable[+A <: AnyRef] = A | Null // disjoint by construction!

object Nullable:
def apply[A <: AnyRef](x: A | Null): Nullable[A] = x

def some[A <: AnyRef](x: A): Nullable[A] = x
def none: Nullable[Nothing] = null

extension [A <: AnyRef](x: Nullable[A])
def isEmpty: Boolean = x == null
def get: A | Null = x

extension [A <: AnyRef, B <: AnyRef](x: Nullable[A])
def flatMap(f: A => Nullable[B]): Nullable[B] =
if (x == null) null
else f(x)

def map(f: A => B): Nullable[B] = x.flatMap(f)

def test1 =
val s1: Nullable[String] = Nullable("hello")
val s2: Nullable[String] = "world"
val s3: Nullable[String] = Nullable.none
val s4: Nullable[String] = null

s1.isEmpty
s1.flatMap((x) => true)

assert(s2 != null)
}

def test2 =
import Nullables._

val s1: Nullable[String] = Nullable("hello")
val s2: Nullable[String] = Nullable.none
val s3: Nullable[String] = null // error: don't leak nullable union

s1.isEmpty
s1.flatMap((x) => Nullable(true))

assert(s2 == null) // error
26 changes: 0 additions & 26 deletions tests/explicit-nulls/pos/opaque-nullable.scala

This file was deleted.

18 changes: 18 additions & 0 deletions tests/explicit-nulls/pos/override-type-params.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Testing relaxed overriding check for explicit nulls.
// The relaxed check is only enabled if one of the members is Java defined.

import java.util.Comparator

class C1[T <: AnyRef] extends Ordering[T]:
override def compare(o1: T, o2: T): Int = 0

// The following overriding is not allowed, because `compare`
// has already been declared in Scala class `Ordering`.
// class C2[T <: AnyRef] extends Ordering[T]:
// override def compare(o1: T | Null, o2: T | Null): Int = 0

class D1[T <: AnyRef] extends Comparator[T]:
override def compare(o1: T, o2: T): Int = 0

class D2[T <: AnyRef] extends Comparator[T]:
override def compare(o1: T | Null, o2: T | Null): Int = 0