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

Remove special overriding logic for explicit nulls #22243

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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: 0 additions & 3 deletions compiler/src/dotty/tools/dotc/core/Contexts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -785,9 +785,6 @@ object Contexts {
def withNotNullInfos(infos: List[NotNullInfo]): Context =
if !c.explicitNulls || (c.notNullInfos eq infos) then c else c.fresh.setNotNullInfos(infos)

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

// TODO: Fix issue when converting ModeChanges and FreshModeChanges to extension givens
extension (c: Context) {
final def withModeBits(mode: Mode): Context =
Expand Down
5 changes: 2 additions & 3 deletions compiler/src/dotty/tools/dotc/core/Denotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -478,12 +478,11 @@ object Denotations {
else if sym1.is(Method) && !sym2.is(Method) then 1
else 0

val relaxedOverriding = ctx.explicitNulls && (sym1.is(JavaDefined) || sym2.is(JavaDefined))
val matchLoosely = sym1.matchNullaryLoosely || sym2.matchNullaryLoosely

if symScore <= 0 && info2.overrides(info1, relaxedOverriding, matchLoosely, checkClassInfo = false) then
if symScore <= 0 && info2.overrides(info1, matchLoosely, checkClassInfo = false) then
denot2
else if symScore >= 0 && info1.overrides(info2, relaxedOverriding, matchLoosely, checkClassInfo = false) then
else if symScore >= 0 && info1.overrides(info2, matchLoosely, checkClassInfo = false) then
denot1
else
val jointInfo = infoMeet(info1, info2, safeIntersection)
Expand Down
4 changes: 3 additions & 1 deletion compiler/src/dotty/tools/dotc/core/JavaNullInterop.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import Flags.JavaDefined
import StdNames.nme
import Symbols.*
import Types.*
import dotty.tools.dotc.reporting.*
import dotty.tools.dotc.core.Decorators.i

/** This module defines methods to interpret types of Java symbols, which are implicitly nullable in Java,
* as Scala types, which are explicitly nullable.
Expand Down Expand Up @@ -51,7 +53,7 @@ object JavaNullInterop {
*
* But the selection can throw an NPE if the returned value is `null`.
*/
def nullifyMember(sym: Symbol, tp: Type, isEnumValueDef: Boolean)(using Context): Type = {
def nullifyMember(sym: Symbol, tp: Type, isEnumValueDef: Boolean)(using Context): Type = trace(i"nullifyMember ${sym}, ${tp}"){
assert(ctx.explicitNulls)
assert(sym.is(JavaDefined), "can only nullify java-defined members")

Expand Down
7 changes: 1 addition & 6 deletions compiler/src/dotty/tools/dotc/core/Mode.scala
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ object Mode {

/** Use previous Scheme for implicit resolution. Currently significant
* in 3.0-migration where we use Scala-2's scheme instead and in 3.5 and 3.6-migration
* where we use the previous scheme up to 3.4 for comparison with the new scheme.
* where we use the previous scheme up to 3.4 for comparison with the new scheme.
*/
val OldImplicitResolution: Mode = newMode(15, "OldImplicitResolution")

Expand Down Expand Up @@ -163,11 +163,6 @@ object Mode {
*/
val ForceInline: Mode = newMode(29, "ForceInline")

/** 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")

/** Skip inlining of methods. */
val NoInline: Mode = newMode(31, "NoInline")
}
8 changes: 0 additions & 8 deletions compiler/src/dotty/tools/dotc/core/TypeComparer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -967,17 +967,9 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
|| compareGADT
|| tryLiftedToThis1
case _ =>
// `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.dealias match
case tp: TypeRef =>
val tpSym = tp.symbol
ctx.mode.is(Mode.RelaxedOverriding) && !tpSym.isPrimitiveValueClass ||
tpSym.isNullableClass
case tp: TermRef =>
// https://scala-lang.org/files/archive/spec/2.13/03-types.html#singleton-types
Expand Down
21 changes: 9 additions & 12 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1160,17 +1160,14 @@ object Types extends TypeUtils {
*
* @param isSubType a function used for checking subtype relationships.
*/
final def overrides(that: Type, relaxedCheck: Boolean, matchLoosely: => Boolean, checkClassInfo: Boolean = true,
final def overrides(that: Type, matchLoosely: => Boolean, checkClassInfo: Boolean = true,
isSubType: (Type, Type) => Context ?=> Boolean = (tp1, tp2) => tp1 frozen_<:< tp2)(using Context): Boolean = {
val overrideCtx = if relaxedCheck then ctx.relaxedOverrideContext else ctx
inContext(overrideCtx) {
!checkClassInfo && this.isInstanceOf[ClassInfo]
|| isSubType(this.widenExpr, that.widenExpr)
|| matchLoosely && {
val this1 = this.widenNullaryMethod
val that1 = that.widenNullaryMethod
((this1 `ne` this) || (that1 `ne` that)) && this1.overrides(that1, relaxedCheck, false, checkClassInfo)
}
!checkClassInfo && this.isInstanceOf[ClassInfo]
|| isSubType(this.widenExpr, that.widenExpr)
|| matchLoosely && {
val this1 = this.widenNullaryMethod
val that1 = that.widenNullaryMethod
((this1 `ne` this) || (that1 `ne` that)) && this1.overrides(that1, false, checkClassInfo)
}
}

Expand All @@ -1196,8 +1193,8 @@ object Types extends TypeUtils {
*/
def matches(that: Type)(using Context): Boolean = {
record("matches")
val overrideCtx = if ctx.explicitNulls then ctx.relaxedOverrideContext else ctx
TypeComparer.matchesType(this, that, relaxed = !ctx.phase.erasedTypes)(using overrideCtx)
withoutMode(Mode.SafeNulls)(
TypeComparer.matchesType(this, that, relaxed = !ctx.phase.erasedTypes))
}

/** This is the same as `matches` except that it also matches => T with T and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,7 @@ object OverridingPairs:
}
)
else
// releaxed override check for explicit nulls if one of the symbols is Java defined,
// force `Null` to be a subtype of non-primitive value types during override checking.
val relaxedOverriding = ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined))
member.name.is(DefaultGetterName) // default getters are not checked for compatibility
|| memberTp.overrides(otherTp, relaxedOverriding,
member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack, isSubType = isSubType)
|| memberTp.overrides(otherTp, member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack, isSubType = isSubType)

end OverridingPairs
7 changes: 2 additions & 5 deletions compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,8 @@ object ResolveSuper {
// of the superaccessor's type, see i5433.scala for an example where this matters
val otherTp = other.asSeenFrom(base.typeRef).info
val accTp = acc.asSeenFrom(base.typeRef).info
// Since the super class can be Java defined,
// 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 relaxedOverriding = ctx.explicitNulls && (sym.is(JavaDefined) || acc.is(JavaDefined))
if !otherTp.overrides(accTp, relaxedOverriding, matchLoosely = true) then

if !otherTp.overrides(accTp, matchLoosely = true) then
report.error(IllegalSuperAccessor(base, memberName, targetName, acc, accTp, other.symbol, otherTp), base.srcPos)
bcs = bcs.tail
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ object RefChecks {
for (mbrd <- self.member(name).alternatives) {
val mbr = mbrd.symbol
val mbrType = mbr.info.asSeenFrom(self, mbr.owner)
if (!mbrType.overrides(mbrd.info, relaxedCheck = false, matchLoosely = true))
if (!mbrType.overrides(mbrd.info, matchLoosely = true))
report.errorOrMigrationWarning(
em"""${mbr.showLocated} is not a legal implementation of `$name` in $clazz
| its type $mbrType
Expand Down
29 changes: 12 additions & 17 deletions project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ object Build {
* Reference version should track the latest version pushed to Maven:
* - In main branch it should be the last RC version
* - In release branch it should be the last stable release
*
* Warning: Change of this variable needs to be consulted with `expectedTastyVersion`
*
* Warning: Change of this variable needs to be consulted with `expectedTastyVersion`
*/
val referenceVersion = "3.6.3-RC1"

Expand All @@ -106,8 +106,8 @@ object Build {
*
* Should only be referred from `dottyVersion` or settings/tasks requiring simplified version string,
* eg. `compatMode` or Windows native distribution version.
*
* Warning: Change of this variable might require updating `expectedTastyVersion`
*
* Warning: Change of this variable might require updating `expectedTastyVersion`
*/
val developedVersion = "3.6.4"

Expand All @@ -119,13 +119,13 @@ object Build {
* During final, stable release is set exactly to `developedVersion`.
*/
val baseVersion = s"$developedVersion-RC1"
/** The version of TASTY that should be emitted, checked in runtime test
* For defails on how TASTY version should be set see related discussions:

/** The version of TASTY that should be emitted, checked in runtime test
* For defails on how TASTY version should be set see related discussions:
* - https://github.com/scala/scala3/issues/13447#issuecomment-912447107
* - https://github.com/scala/scala3/issues/14306#issuecomment-1069333516
* - https://github.com/scala/scala3/pull/19321
*
*
* Simplified rules, given 3.$minor.$patch = $developedVersion
* - Major version is always 28
* - TASTY minor version:
Expand Down Expand Up @@ -1058,7 +1058,6 @@ object Build {
// compiler is updated.
// Then, the next step is to enable flexible types by default and reduce the use of
// `unsafeNulls`.
scalacOptions ++= Seq("-Yno-flexible-types"),
packageAll := {
(`scala3-compiler` / packageAll).value ++ Seq(
"scala3-compiler" -> (Compile / packageBin).value.getAbsolutePath,
Expand Down Expand Up @@ -1450,10 +1449,6 @@ object Build {
.dependsOn(`scala3-compiler-bootstrapped`, `scala3-library-bootstrapped`, `scala3-presentation-compiler-testcases`)
.settings(presentationCompilerSettings)
.settings(scala3PresentationCompilerBuildInfo)
.settings(
// Add `-Yno-flexible-types` flag for bootstrap, see comments for `bootstrappedDottyCompilerSettings`
Compile / scalacOptions += "-Yno-flexible-types"
)

def scala3PresentationCompilerBuildInfo =
Seq(
Expand Down Expand Up @@ -2505,7 +2500,7 @@ object Build {
case Bootstrapped => commonBootstrappedSettings
})
}

/* Tests TASTy version invariants during NIGHLY, RC or Stable releases */
def checkReleasedTastyVersion(): Unit = {
lazy val (scalaMinor, scalaPatch, scalaIsRC) = baseVersion.split("\\.|-").take(4) match {
Expand All @@ -2518,19 +2513,19 @@ object Build {
case Array("28", minor, "experimental", _) => (minor.toInt, true)
case other => sys.error(s"Invalid TASTy version string: $expectedTastyVersion")
}

if(isNightly) {
assert(tastyIsExperimental, "TASTY needs to be experimental in nightly builds")
val expectedTastyMinor = if(scalaPatch == 0) scalaMinor else scalaMinor + 1
assert(tastyMinor == expectedTastyMinor, "Invalid TASTy minor version")
}

if(isRelease) {
assert(scalaMinor == tastyMinor, "Minor versions of TASTY vesion and Scala version should match in release builds")
if (scalaIsRC && scalaPatch == 0)
assert(tastyIsExperimental, "TASTy should be experimental when releasing a new minor version RC")
else
assert(!tastyIsExperimental, "Stable version cannot use experimental TASTY")
assert(!tastyIsExperimental, "Stable version cannot use experimental TASTY")
}
}
}
Expand Down
Loading