Skip to content

Fix #6247: Heal selection errors by fully defining qualifier type #6257

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

Closed
wants to merge 1 commit into from
Closed
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
61 changes: 44 additions & 17 deletions compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import ast.Trees._
import NameOps._
import collection.mutable
import reporting.diagnostic.messages._
import util.Stats.record
import Inferencing._
import Checking.{checkNoPrivateLeaks, checkNoWildcard}

trait TypeAssigner {
Expand Down Expand Up @@ -163,6 +165,20 @@ trait TypeAssigner {

def arrayToRepeated(tree: Tree)(implicit ctx: Context): Tree = toRepeated(tree, defn.ArrayClass)

def tryEither[T](op: Context => T)(fallBack: (T, TyperState) => T)(implicit ctx: Context): T = {
val nestedCtx = ctx.fresh.setNewTyperState()
val result = op(nestedCtx)
if (nestedCtx.reporter.hasErrors && !nestedCtx.reporter.hasStickyErrors) {
record("tryEither.fallBack")
fallBack(result, nestedCtx.typerState)
}
else {
record("tryEither.commit")
nestedCtx.typerState.commit()
result
}
}

/** A denotation exists really if it exists and does not point to a stale symbol. */
final def reallyExists(denot: Denotation)(implicit ctx: Context): Boolean = try
denot match {
Expand Down Expand Up @@ -228,6 +244,7 @@ trait TypeAssigner {

/** The type of the selection `tree`, where `qual1` is the typed qualifier part. */
def selectionType(tree: untpd.RefTree, qual1: Tree)(implicit ctx: Context): Type = {

var qualType = qual1.tpe.widenIfUnstable
if (!qualType.hasSimpleKind && tree.name != nme.CONSTRUCTOR)
// constructors are selected on typeconstructor, type arguments are passed afterwards
Expand All @@ -236,29 +253,39 @@ trait TypeAssigner {
qualType = errorType(em"$qualType is illegal as a selection prefix", qual1.sourcePos)
val name = tree.name
val mbr = qualType.member(name)

def fail = if (name == nme.CONSTRUCTOR)
errorType(ex"$qualType does not have a constructor", tree.sourcePos)
else {
val kind = if (name.isTypeName) "type" else "value"
val addendum =
if (qualType.derivesFrom(defn.DynamicClass))
"\npossible cause: maybe a wrong Dynamic method signature?"
else qual1.getAttachment(Typer.HiddenSearchFailure) match {
case Some(failure) if !failure.reason.isInstanceOf[Implicits.NoMatchingImplicits] =>
i""".
|An extension method was tried, but could not be fully constructed:
|
| ${failure.tree.show.replace("\n", "\n ")}"""
case _ => ""
}
errorType(NotAMember(qualType, name, kind, addendum), tree.sourcePos)
}

if (reallyExists(mbr))
qualType.select(name, mbr)
else if (qualType.derivesFrom(defn.DynamicClass) && name.isTermName && !Dynamic.isDynamicMethod(name))
TryDynamicCallType
else if (qualType.isErroneous || name.toTermName == nme.ERROR)
UnspecifiedErrorType
else if (name == nme.CONSTRUCTOR)
errorType(ex"$qualType does not have a constructor", tree.sourcePos)
else {
val kind = if (name.isTypeName) "type" else "value"
val addendum =
if (qualType.derivesFrom(defn.DynamicClass))
"\npossible cause: maybe a wrong Dynamic method signature?"
else qual1.getAttachment(Typer.HiddenSearchFailure) match {
case Some(failure) if !failure.reason.isInstanceOf[Implicits.NoMatchingImplicits] =>
i""".
|An extension method was tried, but could not be fully constructed:
|
| ${failure.tree.show.replace("\n", "\n ")}"""
case _ => ""
}
errorType(NotAMember(qualType, name, kind, addendum), tree.sourcePos)
}
else if (!isFullyDefined(qualType, force = ForceDegree.none))
tryEither { implicit ctx =>
fullyDefinedType(qualType, "selection prefix", qual1.span)
selectionType(tree, qual1)
} { (_, _) =>
fail
}
else fail
}

/** The type of the selection in `tree`, where `qual1` is the typed qualifier part.
Expand Down
14 changes: 0 additions & 14 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2250,20 +2250,6 @@ class Typer extends Namer
def typedPattern(tree: untpd.Tree, selType: Type = WildcardType)(implicit ctx: Context): Tree =
typed(tree, selType)(ctx addMode Mode.Pattern)

def tryEither[T](op: Context => T)(fallBack: (T, TyperState) => T)(implicit ctx: Context): T = {
val nestedCtx = ctx.fresh.setNewTyperState()
val result = op(nestedCtx)
if (nestedCtx.reporter.hasErrors && !nestedCtx.reporter.hasStickyErrors) {
record("tryEither.fallBack")
fallBack(result, nestedCtx.typerState)
}
else {
record("tryEither.commit")
nestedCtx.typerState.commit()
result
}
}

/** Try `op1`, if there are errors, try `op2`, if `op2` also causes errors, fall back
* to errors and result of `op1`.
*/
Expand Down
10 changes: 10 additions & 0 deletions tests/pos/i6247.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
implicit class NN[T](x:T|Null) {
def nn: T = x.asInstanceOf[T]
}

class Foo {
val x1: String|Null = null
x1.nn.length
val x2: String = x1.nn
x1.nn.length
Copy link
Member

Choose a reason for hiding this comment

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

This line failed while the identical expression two lines above succeeded, so something fishy is going on, I've got what I think is a proper fix in #6301.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! But I think it would be still an improvement to have the changes in this PR also. It is insurance against the fact that we are more lazy instantiating type variables than scalac. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

Without an example showing that this is needed I'm not sure, there's now been two actual bugs we caught that would have stayed hidden with this PR in: #6301 and 38c6d46. so I'd rather keep things as is for now.

Copy link
Contributor Author

@odersky odersky Apr 15, 2019

Choose a reason for hiding this comment

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

I agree. I just went through all the inference related // dotty deviation comments in the compiler codebase and could remove them all without this fix. Except one which was related to us dropping weak conformance. So let's leave things as they are.

}