From b6e641fa28e58272d4982545f514bf57685bd779 Mon Sep 17 00:00:00 2001 From: noti0na1 Date: Fri, 24 Jun 2022 00:39:27 -0400 Subject: [PATCH 1/4] More consistant results in pickling --- .../dotty/tools/dotc/core/TypeComparer.scala | 13 ++-- .../tools/dotc/core/tasty/TreeUnpickler.scala | 26 ++++---- .../dotty/tools/dotc/typer/Nullables.scala | 2 +- .../dotty/tools/dotc/CompilationTests.scala | 2 +- .../i14947.scala | 0 .../pos-pickling}/i15097.scala | 1 + .../pos-pickling/match-case.scala | 59 +++++++++++++++++++ .../pos-pickling/other-pickling.scala | 35 +++++++++++ .../pos-pickling/try-catch.scala | 27 +++++++++ 9 files changed, 142 insertions(+), 23 deletions(-) rename tests/explicit-nulls/{pos-special => pos-pickling}/i14947.scala (100%) rename tests/{pos => explicit-nulls/pos-pickling}/i15097.scala (92%) create mode 100644 tests/explicit-nulls/pos-pickling/match-case.scala create mode 100644 tests/explicit-nulls/pos-pickling/other-pickling.scala create mode 100644 tests/explicit-nulls/pos-pickling/try-catch.scala diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 6d79f377c84e..e5216f2305ac 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -2141,7 +2141,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling * @note We do not admit singleton types in or-types as lubs. */ def lub(tp1: Type, tp2: Type, canConstrain: Boolean = false, isSoft: Boolean = true): Type = /*>|>*/ trace(s"lub(${tp1.show}, ${tp2.show}, canConstrain=$canConstrain, isSoft=$isSoft)", subtyping, show = true) /*<|<*/ { - if (tp1 eq tp2) tp1 + if tp1 eq tp2 then tp1 else if !tp1.exists || (tp2 eq WildcardType) then tp1 else if !tp2.exists || (tp1 eq WildcardType) then tp2 else if tp1.isAny && !tp2.isLambdaSub || tp1.isAnyKind || isBottom(tp2) then tp1 @@ -2157,16 +2157,19 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling if (hi1 & hi2).isEmpty then return orType(tp1, tp2) case none => case none => + + // Simplifying the super type is important to avoid + // inconsistant result in union type. val t1 = mergeIfSuper(tp1, tp2, canConstrain) - if (t1.exists) return t1 + if t1.exists then return t1.simplified val t2 = mergeIfSuper(tp2, tp1, canConstrain) - if (t2.exists) return t2 + if t2.exists then return t2.simplified - def widen(tp: Type) = if (widenInUnions) tp.widen else tp.widenIfUnstable + def widen(tp: Type) = if widenInUnions then tp.widen else tp.widenIfUnstable val tp1w = widen(tp1) val tp2w = widen(tp2) - if ((tp1 ne tp1w) || (tp2 ne tp2w)) lub(tp1w, tp2w, canConstrain = canConstrain, isSoft = isSoft) + if (tp1 ne tp1w) || (tp2 ne tp2w) then lub(tp1w, tp2w, canConstrain = canConstrain, isSoft = isSoft) else orType(tp1w, tp2w, isSoft = isSoft) // no need to check subtypes again } mergedLub(tp1.stripLazyRef, tp2.stripLazyRef) diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala index 440871481114..b5c79209aec9 100644 --- a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala +++ b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala @@ -1194,10 +1194,6 @@ class TreeUnpickler(reader: TastyReader, res.withAttachment(SuppressedApplyToNone, ()) else res - def simplifyLub(tree: Tree): Tree = - tree.overwriteType(tree.tpe.simplified) - tree - def readLengthTerm(): Tree = { val end = readEnd() val result = @@ -1247,16 +1243,15 @@ class TreeUnpickler(reader: TastyReader, val tpt = ifBefore(end)(readTpt(), EmptyTree) Closure(Nil, meth, tpt) case MATCH => - simplifyLub( - if (nextByte == IMPLICIT) { - readByte() - InlineMatch(EmptyTree, readCases(end)) - } - else if (nextByte == INLINE) { - readByte() - InlineMatch(readTerm(), readCases(end)) - } - else Match(readTerm(), readCases(end))) + if (nextByte == IMPLICIT) { + readByte() + InlineMatch(EmptyTree, readCases(end)) + } + else if (nextByte == INLINE) { + readByte() + InlineMatch(readTerm(), readCases(end)) + } + else Match(readTerm(), readCases(end)) case RETURN => val from = readSymRef() val expr = ifBefore(end)(readTerm(), EmptyTree) @@ -1264,8 +1259,7 @@ class TreeUnpickler(reader: TastyReader, case WHILE => WhileDo(readTerm(), readTerm()) case TRY => - simplifyLub( - Try(readTerm(), readCases(end), ifBefore(end)(readTerm(), EmptyTree))) + Try(readTerm(), readCases(end), ifBefore(end)(readTerm(), EmptyTree)) case SELECTouter => val levels = readNat() readTerm().outerSelect(levels, SkolemType(readType())) diff --git a/compiler/src/dotty/tools/dotc/typer/Nullables.scala b/compiler/src/dotty/tools/dotc/typer/Nullables.scala index 9104418d406f..60f0530bdc33 100644 --- a/compiler/src/dotty/tools/dotc/typer/Nullables.scala +++ b/compiler/src/dotty/tools/dotc/typer/Nullables.scala @@ -28,7 +28,7 @@ object Nullables: ctx.explicitNulls && !ctx.mode.is(Mode.SafeNulls) private def needNullifyHi(lo: Type, hi: Type)(using Context): Boolean = - ctx.explicitNulls + ctx.mode.is(Mode.SafeNulls) && lo.isExactlyNull // only nullify hi if lo is exactly Null type && hi.isValueType // We cannot check if hi is nullable, because it can cause cyclic reference. diff --git a/compiler/test/dotty/tools/dotc/CompilationTests.scala b/compiler/test/dotty/tools/dotc/CompilationTests.scala index 1abb1ffd9ea3..5a143f112783 100644 --- a/compiler/test/dotty/tools/dotc/CompilationTests.scala +++ b/compiler/test/dotty/tools/dotc/CompilationTests.scala @@ -249,9 +249,9 @@ class CompilationTests { compileFilesInDir("tests/explicit-nulls/pos", explicitNullsOptions), compileFilesInDir("tests/explicit-nulls/pos-separate", explicitNullsOptions), compileFilesInDir("tests/explicit-nulls/pos-patmat", explicitNullsOptions and "-Xfatal-warnings"), + compileFilesInDir("tests/explicit-nulls/pos-pickling", explicitNullsOptions and "-Ytest-pickler" and "-Xprint-types"), compileFilesInDir("tests/explicit-nulls/unsafe-common", explicitNullsOptions and "-language:unsafeNulls"), compileFile("tests/explicit-nulls/pos-special/i14682.scala", explicitNullsOptions and "-Ysafe-init"), - compileFile("tests/explicit-nulls/pos-special/i14947.scala", explicitNullsOptions and "-Ytest-pickler" and "-Xprint-types"), ) }.checkCompile() diff --git a/tests/explicit-nulls/pos-special/i14947.scala b/tests/explicit-nulls/pos-pickling/i14947.scala similarity index 100% rename from tests/explicit-nulls/pos-special/i14947.scala rename to tests/explicit-nulls/pos-pickling/i14947.scala diff --git a/tests/pos/i15097.scala b/tests/explicit-nulls/pos-pickling/i15097.scala similarity index 92% rename from tests/pos/i15097.scala rename to tests/explicit-nulls/pos-pickling/i15097.scala index 8b9cb2b5d893..4eab7a39ce8d 100644 --- a/tests/pos/i15097.scala +++ b/tests/explicit-nulls/pos-pickling/i15097.scala @@ -10,6 +10,7 @@ class C: if ??? then g else "" def f3 = + import scala.language.unsafeNulls (??? : Boolean) match case true => g case _ => "" diff --git a/tests/explicit-nulls/pos-pickling/match-case.scala b/tests/explicit-nulls/pos-pickling/match-case.scala new file mode 100644 index 000000000000..f1f481c6e72e --- /dev/null +++ b/tests/explicit-nulls/pos-pickling/match-case.scala @@ -0,0 +1,59 @@ +import scala.language.unsafeNulls + +def a(): String | Null = ??? +val b: String | Null = ??? + +val i: Int = ??? + +def f1 = i match { + case 0 => b + case _ => a() +} + +def f2 = i match { + case 0 => a() + case _ => b +} + +def f3 = i match { + case 0 => a() + case _ => "".trim +} + +def f4 = i match { + case 0 => b + case _ => "".trim +} + +def g1 = i match { + case 0 => a() + case 1 => "" + case _ => null +} + +def g2 = i match { + case 0 => "" + case 1 => null + case _ => b +} + +def g3 = i match { + case 0 => null + case 1 => b + case _ => "" +} + +def h1(i: Int) = i match + case 0 => 0 + case 1 => true + case 2 => i.toString + case _ => null + +// This test still fails. +// Even without explicit nulls, the type of Match +// is (0, true, "2"), which is wrong. +// def h2(i: Int) = i match +// case 0 => 0 +// case 1 => true +// case 2 => "2" +// case _ => null \ No newline at end of file diff --git a/tests/explicit-nulls/pos-pickling/other-pickling.scala b/tests/explicit-nulls/pos-pickling/other-pickling.scala new file mode 100644 index 000000000000..1177eef2958a --- /dev/null +++ b/tests/explicit-nulls/pos-pickling/other-pickling.scala @@ -0,0 +1,35 @@ +import scala.language.unsafeNulls + +def associatedFile: String | Null = ??? + +def source: String = { + val f = associatedFile + if (f != null) f else associatedFile +} + +def defines(raw: String): List[String] = { + val ds: List[(Int, Int)] = ??? + ds map { case (start, end) => raw.substring(start, end) } +} + +abstract class DeconstructorCommon[T >: Null <: AnyRef] { + var field: T = null + def get: this.type = this + def isEmpty: Boolean = field eq null + def isDefined = !isEmpty + def unapply(s: T): this.type ={ + field = s + this + } +} + +def genBCode = + val bsmArgs: Array[Object | Null] | Null = null + val implMethod = bsmArgs(3).asInstanceOf[Integer].toInt + implMethod + +val arrayApply = "a".split(" ")(2) + +val globdir: String = if (??? : Boolean) then associatedFile.replaceAll("[\\\\/][^\\\\/]*$", "") else "" + +def newInstOfC(c: Class[?]) = c.getConstructor().newInstance() \ No newline at end of file diff --git a/tests/explicit-nulls/pos-pickling/try-catch.scala b/tests/explicit-nulls/pos-pickling/try-catch.scala new file mode 100644 index 000000000000..40c670173b89 --- /dev/null +++ b/tests/explicit-nulls/pos-pickling/try-catch.scala @@ -0,0 +1,27 @@ +import scala.language.unsafeNulls + + +def s: String | Null = ??? + +def tryString = try s catch { + case _: NullPointerException => null + case _ => "" +} + +def tryString2 = try s catch { + case _: NullPointerException => "" + case _ => s +} + +def loadClass(classLoader: ClassLoader, name: String): Class[?] = + try classLoader.loadClass(name) + catch { + case _ => + throw new Exception() + } + +def loadClass2(classLoader: ClassLoader, name: String): Class[?] = + try classLoader.loadClass(name) + catch { + case _ => null + } \ No newline at end of file From 3bd0823477d572e5c1230108e4047c6eaef85588 Mon Sep 17 00:00:00 2001 From: noti0na1 Date: Tue, 5 Jul 2022 15:58:05 -0400 Subject: [PATCH 2/4] Simplify the types of trees before and after pickling --- .../dotty/tools/dotc/core/TypeComparer.scala | 13 ++++----- .../dotty/tools/dotc/transform/Pickler.scala | 27 +++++++++++++++++-- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index e5216f2305ac..6d79f377c84e 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -2141,7 +2141,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling * @note We do not admit singleton types in or-types as lubs. */ def lub(tp1: Type, tp2: Type, canConstrain: Boolean = false, isSoft: Boolean = true): Type = /*>|>*/ trace(s"lub(${tp1.show}, ${tp2.show}, canConstrain=$canConstrain, isSoft=$isSoft)", subtyping, show = true) /*<|<*/ { - if tp1 eq tp2 then tp1 + if (tp1 eq tp2) tp1 else if !tp1.exists || (tp2 eq WildcardType) then tp1 else if !tp2.exists || (tp1 eq WildcardType) then tp2 else if tp1.isAny && !tp2.isLambdaSub || tp1.isAnyKind || isBottom(tp2) then tp1 @@ -2157,19 +2157,16 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling if (hi1 & hi2).isEmpty then return orType(tp1, tp2) case none => case none => - - // Simplifying the super type is important to avoid - // inconsistant result in union type. val t1 = mergeIfSuper(tp1, tp2, canConstrain) - if t1.exists then return t1.simplified + if (t1.exists) return t1 val t2 = mergeIfSuper(tp2, tp1, canConstrain) - if t2.exists then return t2.simplified + if (t2.exists) return t2 - def widen(tp: Type) = if widenInUnions then tp.widen else tp.widenIfUnstable + def widen(tp: Type) = if (widenInUnions) tp.widen else tp.widenIfUnstable val tp1w = widen(tp1) val tp2w = widen(tp2) - if (tp1 ne tp1w) || (tp2 ne tp2w) then lub(tp1w, tp2w, canConstrain = canConstrain, isSoft = isSoft) + if ((tp1 ne tp1w) || (tp2 ne tp2w)) lub(tp1w, tp2w, canConstrain = canConstrain, isSoft = isSoft) else orType(tp1w, tp2w, isSoft = isSoft) // no need to check subtypes again } mergedLub(tp1.stripLazyRef, tp2.stripLazyRef) diff --git a/compiler/src/dotty/tools/dotc/transform/Pickler.scala b/compiler/src/dotty/tools/dotc/transform/Pickler.scala index 4d9b42a36fe7..17ade99102a9 100644 --- a/compiler/src/dotty/tools/dotc/transform/Pickler.scala +++ b/compiler/src/dotty/tools/dotc/transform/Pickler.scala @@ -12,6 +12,7 @@ import Phases._ import Symbols._ import Flags.Module import reporting.{ThrowingReporter, Profile} +import typer.Nullables import collection.mutable import scala.concurrent.{Future, Await, ExecutionContext} import scala.concurrent.duration.Duration @@ -60,13 +61,15 @@ class Pickler extends Phase { val unit = ctx.compilationUnit pickling.println(i"unpickling in run ${ctx.runId}") + val typeSimplifier = new TypeSimplifyTransformer + for cls <- dropCompanionModuleClasses(topLevelClasses(unit.tpdTree)) tree <- sliceTopLevel(unit.tpdTree, cls) do val pickler = new TastyPickler(cls) if ctx.settings.YtestPickler.value then - beforePickling(cls) = tree.show + beforePickling(cls) = typeSimplifier.transform(tree).show picklers(cls) = pickler val treePkl = new TreePickler(pickler) treePkl.pickle(tree :: Nil) @@ -134,8 +137,11 @@ class Pickler extends Phase { cls -> unpickler } pickling.println("************* entered toplevel ***********") + + val typeSimplifier = new TypeSimplifyTransformer + for ((cls, unpickler) <- unpicklers) { - val unpickled = unpickler.rootTrees + val unpickled = typeSimplifier.transform(unpickler.rootTrees) testSame(i"$unpickled%\n%", beforePickling(cls), cls) } } @@ -151,4 +157,21 @@ class Pickler extends Phase { | | diff before-pickling.txt after-pickling.txt""".stripMargin) end testSame + + // Overwrite types of If, Match, and Try nodes with simplified types + // to avoid inconsistencies in unsafe nulls + class TypeSimplifyTransformer extends TreeMapWithPreciseStatContexts: + override def transform(tree: Tree)(using Context): Tree = + try tree match + case _: If | _: Match | _: Try if Nullables.unsafeNullsEnabled => + val newTree = super.transform(tree) + newTree.overwriteType(newTree.tpe.simplified) + newTree + case _ => + super.transform(tree) + catch + case ex: TypeError => + report.error(ex, tree.srcPos) + tree + end TypeSimplifyTransformer } From cbd747dd7f44cd5b28503078b919434ca9acad1e Mon Sep 17 00:00:00 2001 From: noti0na1 Date: Tue, 5 Jul 2022 16:30:39 -0400 Subject: [PATCH 3/4] Place typeSimplifier as a member of Pickling --- compiler/src/dotty/tools/dotc/transform/Pickler.scala | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Pickler.scala b/compiler/src/dotty/tools/dotc/transform/Pickler.scala index 17ade99102a9..24227bfd9703 100644 --- a/compiler/src/dotty/tools/dotc/transform/Pickler.scala +++ b/compiler/src/dotty/tools/dotc/transform/Pickler.scala @@ -50,6 +50,8 @@ class Pickler extends Phase { private val beforePickling = new mutable.HashMap[ClassSymbol, String] private val picklers = new mutable.HashMap[ClassSymbol, TastyPickler] + private val typeSimplifier = new TypeSimplifyTransformer + /** Drop any elements of this list that are linked module classes of other elements in the list */ private def dropCompanionModuleClasses(clss: List[ClassSymbol])(using Context): List[ClassSymbol] = { val companionModuleClasses = @@ -61,8 +63,6 @@ class Pickler extends Phase { val unit = ctx.compilationUnit pickling.println(i"unpickling in run ${ctx.runId}") - val typeSimplifier = new TypeSimplifyTransformer - for cls <- dropCompanionModuleClasses(topLevelClasses(unit.tpdTree)) tree <- sliceTopLevel(unit.tpdTree, cls) @@ -137,9 +137,6 @@ class Pickler extends Phase { cls -> unpickler } pickling.println("************* entered toplevel ***********") - - val typeSimplifier = new TypeSimplifyTransformer - for ((cls, unpickler) <- unpicklers) { val unpickled = typeSimplifier.transform(unpickler.rootTrees) testSame(i"$unpickled%\n%", beforePickling(cls), cls) From 83a60b668dd83e3df54cf54a3e46151f52410566 Mon Sep 17 00:00:00 2001 From: noti0na1 Date: Tue, 12 Jul 2022 02:27:30 -0400 Subject: [PATCH 4/4] Simplify the pickled tree after unpickler --- compiler/src/dotty/tools/dotc/transform/Pickler.scala | 11 +++++------ .../src/dotty/tools/dotc/transform/PostTyper.scala | 4 ++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Pickler.scala b/compiler/src/dotty/tools/dotc/transform/Pickler.scala index 24227bfd9703..d5c9bb33862c 100644 --- a/compiler/src/dotty/tools/dotc/transform/Pickler.scala +++ b/compiler/src/dotty/tools/dotc/transform/Pickler.scala @@ -12,7 +12,6 @@ import Phases._ import Symbols._ import Flags.Module import reporting.{ThrowingReporter, Profile} -import typer.Nullables import collection.mutable import scala.concurrent.{Future, Await, ExecutionContext} import scala.concurrent.duration.Duration @@ -50,7 +49,7 @@ class Pickler extends Phase { private val beforePickling = new mutable.HashMap[ClassSymbol, String] private val picklers = new mutable.HashMap[ClassSymbol, TastyPickler] - private val typeSimplifier = new TypeSimplifyTransformer + private val typeSimplifier = new TypeSimplifier /** Drop any elements of this list that are linked module classes of other elements in the list */ private def dropCompanionModuleClasses(clss: List[ClassSymbol])(using Context): List[ClassSymbol] = { @@ -69,7 +68,7 @@ class Pickler extends Phase { do val pickler = new TastyPickler(cls) if ctx.settings.YtestPickler.value then - beforePickling(cls) = typeSimplifier.transform(tree).show + beforePickling(cls) = tree.show picklers(cls) = pickler val treePkl = new TreePickler(pickler) treePkl.pickle(tree :: Nil) @@ -157,10 +156,10 @@ class Pickler extends Phase { // Overwrite types of If, Match, and Try nodes with simplified types // to avoid inconsistencies in unsafe nulls - class TypeSimplifyTransformer extends TreeMapWithPreciseStatContexts: + class TypeSimplifier extends TreeMapWithPreciseStatContexts: override def transform(tree: Tree)(using Context): Tree = try tree match - case _: If | _: Match | _: Try if Nullables.unsafeNullsEnabled => + case _: If | _: Match | _: Try => val newTree = super.transform(tree) newTree.overwriteType(newTree.tpe.simplified) newTree @@ -170,5 +169,5 @@ class Pickler extends Phase { case ex: TypeError => report.error(ex, tree.srcPos) tree - end TypeSimplifyTransformer + end TypeSimplifier } diff --git a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala b/compiler/src/dotty/tools/dotc/transform/PostTyper.scala index 8b7ac94675a1..ab3372ab7d53 100644 --- a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala +++ b/compiler/src/dotty/tools/dotc/transform/PostTyper.scala @@ -460,6 +460,10 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase ) case Block(_, Closure(_, _, tpt)) if ExpandSAMs.needsWrapperClass(tpt.tpe) => superAcc.withInvalidCurrentClass(super.transform(tree)) + case _: If | _: Match | _: Try => + val newTree = super.transform(tree) + newTree.overwriteType(newTree.tpe.simplified) + newTree case tree => super.transform(tree) }