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

Don't optimize explicitly written isInstanceOf tests away. #14715

Merged
merged 4 commits into from
Mar 21, 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
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ enum ErrorMessageID extends java.lang.Enum[ErrorMessageID]:
DoubleDefinitionID,
MatchCaseOnlyNullWarningID,
ImportRenamedTwiceID,
TypeTestAlwaysSucceedsID,
TypeTestAlwaysDivergesID,
TermMemberNeedsNeedsResultTypeForImplicitSearchID,
ClassCannotExtendEnumID,
ValueClassParameterMayNotBeCallByNameID,
Expand Down
10 changes: 3 additions & 7 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2177,13 +2177,9 @@ import transform.SymUtils._
def explain = ""
}

class TypeTestAlwaysSucceeds(scrutTp: Type, testTp: Type)(using Context) extends SyntaxMsg(TypeTestAlwaysSucceedsID) {
def msg = {
val addendum =
if (scrutTp != testTp) s" is a subtype of ${testTp.show}"
else " is the same as the tested type"
s"The highlighted type test will always succeed since the scrutinee type ${scrutTp.show}" + addendum
}
class TypeTestAlwaysDiverges(scrutTp: Type, testTp: Type)(using Context) extends SyntaxMsg(TypeTestAlwaysDivergesID) {
def msg =
s"This type test will never return a result since the scrutinee type ${scrutTp.show} does not contain any value."
Comment on lines -2180 to +2182
Copy link
Member

Choose a reason for hiding this comment

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

These changes don't seem to belong to this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are actually linked to the changes in TypeTestsCasts. We had an error TypeTestAlwaysSucceeds before but now we have a TypeTestAlwaysDiverges instead.

def explain = ""
}

Expand Down
31 changes: 20 additions & 11 deletions compiler/src/dotty/tools/dotc/transform/TailRec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,16 @@ package transform

import ast.{TreeTypeMap, tpd}
import config.Printers.tailrec
import core.Contexts._
import core.Constants.Constant
import core.Flags._
import core.NameKinds.{TailLabelName, TailLocalName, TailTempName}
import core.StdNames.nme
import core.Symbols._
import reporting._
import core.*
import Contexts.*, Flags.*, Symbols.*
import Constants.Constant
import NameKinds.{TailLabelName, TailLocalName, TailTempName}
import StdNames.nme
import reporting.*
import transform.MegaPhase.MiniPhase
import util.LinearSet
import dotty.tools.uncheckedNN


/** A Tail Rec Transformer.
*
* What it does:
Expand Down Expand Up @@ -161,15 +159,26 @@ class TailRec extends MiniPhase {
val rhsFullyTransformed = varForRewrittenThis match {
case Some(localThisSym) =>
val thisRef = localThisSym.termRef
new TreeTypeMap(
val substitute = new TreeTypeMap(
typeMap = _.substThisUnlessStatic(enclosingClass, thisRef)
.subst(rewrittenParamSyms, varsForRewrittenParamSyms.map(_.termRef)),
treeMap = {
case tree: This if tree.symbol == enclosingClass => Ident(thisRef)
case tree => tree
}
).transform(rhsSemiTransformed)

)
// The previous map will map `This` references to `Ident`s even under `Super`.
// This violates super's contract. We fix this by cleaning up `Ident`s under
// super, mapping them back to the original `This` reference. This is not
// very elegant, but I did not manage to find a cleaner way to handle this.
// See pos/tailrec-super.scala for a test case.
val cleanup = new TreeMap:
override def transform(t: Tree)(using Context) = t match
case Super(qual: Ident, mix) if !qual.tpe.isInstanceOf[Types.ThisType] =>
cpy.Super(t)(This(enclosingClass), mix)
case _ =>
super.transform(t)
cleanup.transform(substitute.transform(rhsSemiTransformed))
case none =>
new TreeTypeMap(
typeMap = _.subst(rewrittenParamSyms, varsForRewrittenParamSyms.map(_.termRef))
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/TreeChecker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ class TreeChecker extends Phase with SymTransformer {
}

override def typedSuper(tree: untpd.Super, pt: Type)(using Context): Tree =
assert(tree.qual.tpe.isInstanceOf[ThisType], i"expect prefix of Super to be This, actual = ${tree.qual}")
assert(tree.qual.typeOpt.isInstanceOf[ThisType], i"expect prefix of Super to be This, actual = ${tree.qual}")
super.typedSuper(tree, pt)

override def typedTyped(tree: untpd.Typed, pt: Type)(using Context): Tree =
Expand Down
9 changes: 4 additions & 5 deletions compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,12 @@ object TypeTestsCasts {
else foundClasses.exists(check)
end checkSensical

if (expr.tpe <:< testType)
if (expr.tpe.isNotNull) {
if (!inMatch) report.warning(TypeTestAlwaysSucceeds(expr.tpe, testType), tree.srcPos)
constant(expr, Literal(Constant(true)))
}
if (expr.tpe <:< testType) && inMatch then
if expr.tpe.isNotNull then constant(expr, Literal(Constant(true)))
else expr.testNotNull
else {
if expr.tpe.isBottomType then
report.warning(TypeTestAlwaysDiverges(expr.tpe, testType), tree.srcPos)
val nestedCtx = ctx.fresh.setNewTyperState()
val foundClsSyms = foundClasses(expr.tpe.widen, Nil)
val sensical = checkSensical(foundClsSyms)(using nestedCtx)
Expand Down
3 changes: 3 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i14705.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
val n = Nil
val b = n.head.isInstanceOf[String] // error

17 changes: 17 additions & 0 deletions tests/pos/tailrec-super.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class Tree
case class Inlined(call: Tree, bindings: List[String], expr: Tree) extends Tree
case object EmptyTree extends Tree
class Context

class Transform:
def transform(tree: Tree)(using Context): Tree = tree

class Inliner:
var enclosingInlineds: List[String] = Nil
private def expandMacro(using Context) =
val inlinedNormalizer = new Transform:
override def transform(tree: Tree)(using Context) = tree match
case Inlined(EmptyTree, Nil, expr) if enclosingInlineds.isEmpty => transform(expr)
case _ => super.transform(tree)

object Inliner
19 changes: 19 additions & 0 deletions tests/run/i14705.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
trait Fruit
case class Apple() extends Fruit
case class Orange() extends Fruit

case class Box[C](fruit: C) extends Fruit

val apple = Box(fruit = Apple())
val orange = Box(fruit = Orange())


val result = List(apple, orange).map {
case appleBox: Box[Apple] @unchecked if appleBox.fruit.isInstanceOf[Apple] => //contains apple
"apple"
case _ =>
"orange"
}

@main def Test =
assert(result == List("apple", "orange"))