From 2007ad2a1c6a99225c15d8d46f32cd51156d4fe7 Mon Sep 17 00:00:00 2001 From: Abel Nieto Date: Tue, 12 Feb 2019 17:01:47 -0500 Subject: [PATCH 1/2] Fix #5067: handle scrutinee of bottom type in pattern matcher When desugaring pattern matching code for expressions where the matched value has type `Null` or `Nothing`, we used to generate code that's type-incorrect. Example: ``` val Some(x) = null ``` got desugared into ``` val x: Nothing = matchResult1[Nothing]: { case val x1: Null @unchecked = null: Null @unchecked if x1.ne(null) then { case val x: Nothing = x1.value.asInstanceOf[Nothing] return[matchResult1] x: Nothing } else () return[matchResult1] throw new MatchError(x1) } ``` There were two problems here: 1) `x1.ne(null)` 2) `x1.value` In both cases, we're trying to invoke methods that don't exist for type `Nothing` (and #2 doesn't exist for `Null`). This commits changes the desugaring so that 1) is solved by adding an ascription, if needed: (x1: AnyRef).ne(null) 2) is added by generating throw-away but type-correct code that never executes: `throw null` --- compiler/src/dotty/tools/dotc/ast/tpd.scala | 12 +++++-- .../dotty/tools/dotc/core/Definitions.scala | 5 +++ .../tools/dotc/transform/PatternMatcher.scala | 14 +++++--- tests/run/i5067.check | 3 ++ tests/run/i5067.scala | 26 ++++++++++++++ tests/run/i5067b.check | 3 ++ tests/run/i5067b.scala | 34 +++++++++++++++++++ 7 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 tests/run/i5067.check create mode 100644 tests/run/i5067.scala create mode 100644 tests/run/i5067b.check create mode 100644 tests/run/i5067b.scala diff --git a/compiler/src/dotty/tools/dotc/ast/tpd.scala b/compiler/src/dotty/tools/dotc/ast/tpd.scala index 514377802e57..e486f9a9232d 100644 --- a/compiler/src/dotty/tools/dotc/ast/tpd.scala +++ b/compiler/src/dotty/tools/dotc/ast/tpd.scala @@ -889,9 +889,15 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo { else Erasure.Boxing.adaptToType(tree, tp) /** `tree ne null` (might need a cast to be type correct) */ - def testNotNull(implicit ctx: Context): Tree = - tree.ensureConforms(defn.ObjectType) - .select(defn.Object_ne).appliedTo(Literal(Constant(null))) + def testNotNull(implicit ctx: Context): Tree = { + val receiver = if (!defn.isSubtypeOfBottom(tree.tpe)) tree.ensureConforms(defn.ObjectType) + else { + // If the receiver is of type `Nothing` or `Null`, add an ascription so that the selection + // succeeds: e.g. `null.ne(null)` doesn't type, but `(null: AnyRef).ne(null)` does. + Typed(tree, TypeTree(defn.AnyRefType)) + } + receiver.select(defn.Object_ne).appliedTo(Literal(Constant(null))) + } /** If inititializer tree is `_', the default value of its type, * otherwise the tree itself. diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index d3b9b655b65f..005396e6ff6b 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -1004,6 +1004,11 @@ class Definitions { def isBottomType(tp: Type): Boolean = tp.derivesFrom(NothingClass) || tp.derivesFrom(NullClass) + /** Is `tp` a subtype of `Nothing` or `Null`? Unlike `isBottomType`, this uses subtyping instead of inheritance. */ + def isSubtypeOfBottom(tp: Type): Boolean = { + tp.frozen_<:<(NothingType) || tp.frozen_<:<(NullType) + } + /** Is a function class. * - FunctionXXL * - FunctionN for N >= 0 diff --git a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala index 65aadb86ccb9..42474219700a 100644 --- a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala +++ b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala @@ -335,10 +335,16 @@ object PatternMatcher { patternPlan(casted, pat, onSuccess) }) case UnApply(extractor, implicits, args) => - val mt @ MethodType(_) = extractor.tpe.widen - var unapp = extractor.appliedTo(ref(scrutinee).ensureConforms(mt.paramInfos.head)) - if (implicits.nonEmpty) unapp = unapp.appliedToArgs(implicits) - val unappPlan = unapplyPlan(unapp, args) + val unappPlan = if (!defn.isSubtypeOfBottom(scrutinee.info)) { + val mt @ MethodType(_) = extractor.tpe.widen + var unapp = extractor.appliedTo(ref(scrutinee).ensureConforms(mt.paramInfos.head)) + if (implicits.nonEmpty) unapp = unapp.appliedToArgs(implicits) + unapplyPlan(unapp, args) + } else { + // Generate a throwaway but type-correct plan. + // This plan will never execute because it'll be guarded by a `NonNullTest`. + ResultPlan(tpd.Throw(tpd.Literal(Constant(null)))) + } if (scrutinee.info.isNotNull || nonNull(scrutinee)) unappPlan else TestPlan(NonNullTest, scrutinee, tree.span, unappPlan) case Bind(name, body) => diff --git a/tests/run/i5067.check b/tests/run/i5067.check new file mode 100644 index 000000000000..111f693cd4a4 --- /dev/null +++ b/tests/run/i5067.check @@ -0,0 +1,3 @@ +matches null literal +matches null literal +not implemented diff --git a/tests/run/i5067.scala b/tests/run/i5067.scala new file mode 100644 index 000000000000..6be745cc2f08 --- /dev/null +++ b/tests/run/i5067.scala @@ -0,0 +1,26 @@ +// Test that we correctly handle scrutinees with type `Null` or `Nothing`. +object Test { + def main(args: Array[String]): Unit = { + null match { + case Some(_) => println("matches Some") + case (_, _) => println("matches Pair") + case null => println("matches null literal") + } + + type X = Null + (null: X) match { + case Some(_) => println("matches Some") + case (_, _) => println("matches Pair") + case null => println("matches null literal") + } + + type Y = Nothing + try { + (??? : Y) match { + case _ => println("matches anything") + } + } catch { + case e: NotImplementedError => println("not implemented") + } + } +} diff --git a/tests/run/i5067b.check b/tests/run/i5067b.check new file mode 100644 index 000000000000..be60186f7c25 --- /dev/null +++ b/tests/run/i5067b.check @@ -0,0 +1,3 @@ +match error +match error nested +not implemented error diff --git a/tests/run/i5067b.scala b/tests/run/i5067b.scala new file mode 100644 index 000000000000..cd1f3898cb7e --- /dev/null +++ b/tests/run/i5067b.scala @@ -0,0 +1,34 @@ +// Test that we correctly handle scrutinees with type `Null` or `Nothing`. +object Test { + def main(args: Array[String]): Unit = { + class B[T] {} + object B { + def unapply[T](x: Any): Option[B[T]] = None + } + try { + val B(_) = null + } catch { + case e: MatchError => println("match error") + } + + null match { + case null => + try { + null match { + case Some(_) => () + } + } catch { + case e: MatchError => println("match error nested") + } + } + + try { + ??? match { + case (_, _) => () + case _ => () + } + } catch { + case e: NotImplementedError => println("not implemented error") + } + } +} From bf2fd57a691953046340d5cab40bfae32f9cc1d3 Mon Sep 17 00:00:00 2001 From: Abel Nieto Date: Thu, 14 Feb 2019 11:53:33 -0500 Subject: [PATCH 2/2] Address review comments Flip some conditions. Remove isSubtypeOfBottom. Add new tests. --- compiler/src/dotty/tools/dotc/ast/tpd.scala | 4 ++-- .../dotty/tools/dotc/core/Definitions.scala | 5 ----- .../tools/dotc/transform/PatternMatcher.scala | 10 ++++----- tests/run/i5067.check | 3 +++ tests/run/i5067.scala | 21 +++++++++++++++++++ 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/tpd.scala b/compiler/src/dotty/tools/dotc/ast/tpd.scala index e486f9a9232d..10f3fe634495 100644 --- a/compiler/src/dotty/tools/dotc/ast/tpd.scala +++ b/compiler/src/dotty/tools/dotc/ast/tpd.scala @@ -890,12 +890,12 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo { /** `tree ne null` (might need a cast to be type correct) */ def testNotNull(implicit ctx: Context): Tree = { - val receiver = if (!defn.isSubtypeOfBottom(tree.tpe)) tree.ensureConforms(defn.ObjectType) - else { + val receiver = if (defn.isBottomType(tree.tpe)) { // If the receiver is of type `Nothing` or `Null`, add an ascription so that the selection // succeeds: e.g. `null.ne(null)` doesn't type, but `(null: AnyRef).ne(null)` does. Typed(tree, TypeTree(defn.AnyRefType)) } + else tree.ensureConforms(defn.ObjectType) receiver.select(defn.Object_ne).appliedTo(Literal(Constant(null))) } diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 005396e6ff6b..d3b9b655b65f 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -1004,11 +1004,6 @@ class Definitions { def isBottomType(tp: Type): Boolean = tp.derivesFrom(NothingClass) || tp.derivesFrom(NullClass) - /** Is `tp` a subtype of `Nothing` or `Null`? Unlike `isBottomType`, this uses subtyping instead of inheritance. */ - def isSubtypeOfBottom(tp: Type): Boolean = { - tp.frozen_<:<(NothingType) || tp.frozen_<:<(NullType) - } - /** Is a function class. * - FunctionXXL * - FunctionN for N >= 0 diff --git a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala index 42474219700a..00ebc03cff46 100644 --- a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala +++ b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala @@ -335,15 +335,15 @@ object PatternMatcher { patternPlan(casted, pat, onSuccess) }) case UnApply(extractor, implicits, args) => - val unappPlan = if (!defn.isSubtypeOfBottom(scrutinee.info)) { + val unappPlan = if (defn.isBottomType(scrutinee.info)) { + // Generate a throwaway but type-correct plan. + // This plan will never execute because it'll be guarded by a `NonNullTest`. + ResultPlan(tpd.Throw(tpd.Literal(Constant(null)))) + } else { val mt @ MethodType(_) = extractor.tpe.widen var unapp = extractor.appliedTo(ref(scrutinee).ensureConforms(mt.paramInfos.head)) if (implicits.nonEmpty) unapp = unapp.appliedToArgs(implicits) unapplyPlan(unapp, args) - } else { - // Generate a throwaway but type-correct plan. - // This plan will never execute because it'll be guarded by a `NonNullTest`. - ResultPlan(tpd.Throw(tpd.Literal(Constant(null)))) } if (scrutinee.info.isNotNull || nonNull(scrutinee)) unappPlan else TestPlan(NonNullTest, scrutinee, tree.span, unappPlan) diff --git a/tests/run/i5067.check b/tests/run/i5067.check index 111f693cd4a4..7f5dbdea750f 100644 --- a/tests/run/i5067.check +++ b/tests/run/i5067.check @@ -1,3 +1,6 @@ matches null literal matches null literal not implemented +match error +not implemented +match error diff --git a/tests/run/i5067.scala b/tests/run/i5067.scala index 6be745cc2f08..663ccb15e39a 100644 --- a/tests/run/i5067.scala +++ b/tests/run/i5067.scala @@ -17,10 +17,31 @@ object Test { type Y = Nothing try { (??? : Y) match { + case Some(_) => println("matches Some") case _ => println("matches anything") } } catch { case e: NotImplementedError => println("not implemented") } + + + try { + val Some(_) = null + } catch { + case e: MatchError => println("match error") + } + + try { + val Some(_) = ??? + } catch { + case e: NotImplementedError => println("not implemented") + } + + val x: X = null + try { + val Some(_) = x + } catch { + case e: MatchError => println("match error") + } } }