Skip to content

Fix #5107: disallow block as Apply.fun #5190

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

Merged
merged 1 commit into from
Oct 20, 2018
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
3 changes: 1 addition & 2 deletions compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ class Compiler {
new ExplicitOuter, // Add accessors to outer classes from nested ones.
new ExplicitSelf, // Make references to non-trivial self types explicit as casts
new StringInterpolatorOpt, // Optimizes raw and s string interpolators by rewriting them to string concatentations
new CrossCastAnd, // Normalize selections involving intersection types.
new Splitter) :: // Expand selections involving union types into conditionals
Copy link
Contributor

Choose a reason for hiding this comment

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

The splitter source file was nod removed

new CrossCastAnd) :: // Normalize selections involving intersection types.
List(new PruneErasedDefs, // Drop erased definitions from scopes and simplify erased expressions
new VCInlineMethods, // Inlines calls to value class methods
new SeqLiterals, // Express vararg arguments as arrays
Expand Down
12 changes: 9 additions & 3 deletions compiler/src/dotty/tools/dotc/ast/tpd.scala
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,15 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
def Super(qual: Tree, mixName: TypeName, inConstrCall: Boolean, mixinClass: Symbol = NoSymbol)(implicit ctx: Context): Super =
Super(qual, if (mixName.isEmpty) untpd.EmptyTypeIdent else untpd.Ident(mixName), inConstrCall, mixinClass)

def Apply(fn: Tree, args: List[Tree])(implicit ctx: Context): Apply =
def Apply(fn: Tree, args: List[Tree])(implicit ctx: Context): Apply = {
assert(fn.isInstanceOf[RefTree] || fn.isInstanceOf[GenericApply[_]])
ta.assignType(untpd.Apply(fn, args), fn, args)
}

def TypeApply(fn: Tree, args: List[Tree])(implicit ctx: Context): TypeApply =
def TypeApply(fn: Tree, args: List[Tree])(implicit ctx: Context): TypeApply = {
assert(fn.isInstanceOf[RefTree] || fn.isInstanceOf[GenericApply[_]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have nested TypeApply? Should we check fn.isInstanceOf[Apply[_]] instead of fn.isInstanceOf[GenericApply[_]]?

Copy link
Member

Choose a reason for hiding this comment

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

With named type arguments yes I think so foo[A=Int][String]

Copy link
Member

Choose a reason for hiding this comment

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

... also with curried type lambdas.

ta.assignType(untpd.TypeApply(fn, args), fn, args)
Copy link
Member

Choose a reason for hiding this comment

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

You could try to do the same for TypeApply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could do that. I'm wondering how if/else could possibly appear in fun. But it is always good to have these invariants if they don't incur much performance cost.

Copy link
Contributor

Choose a reason for hiding this comment

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

You will probably not be able to. You could add the assertion to make sure.

}

def Literal(const: Constant)(implicit ctx: Context): Literal =
ta.assignType(untpd.Literal(const))
Expand Down Expand Up @@ -181,8 +185,10 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
def Alternative(trees: List[Tree])(implicit ctx: Context): Alternative =
ta.assignType(untpd.Alternative(trees), trees)

def UnApply(fun: Tree, implicits: List[Tree], patterns: List[Tree], proto: Type)(implicit ctx: Context): UnApply =
def UnApply(fun: Tree, implicits: List[Tree], patterns: List[Tree], proto: Type)(implicit ctx: Context): UnApply = {
assert(fun.isInstanceOf[RefTree] || fun.isInstanceOf[GenericApply[_]])
ta.assignType(untpd.UnApply(fun, implicits, patterns), proto)
}

def ValDef(sym: TermSymbol, rhs: LazyTree = EmptyTree)(implicit ctx: Context): ValDef =
ta.assignType(untpd.ValDef(sym.name, TypeTree(sym.info), rhs), sym)
Expand Down
3 changes: 0 additions & 3 deletions compiler/src/dotty/tools/dotc/transform/ElimByName.scala
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ class ElimByName extends TransformByNameApply with InfoTransformer {

override def phaseName: String = ElimByName.name

override def runsAfterGroupsOf: Set[String] = Set(Splitter.name)
// I got errors running this phase in an earlier group, but I did not track them down.

override def changesParents: Boolean = true // Only true for by-names

/** Map `tree` to `tree.apply()` is `ftree` was of ExprType and becomes now a function */
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/Erasure.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class Erasure extends Phase with DenotTransformer {
override def phaseName: String = Erasure.name

/** List of names of phases that should precede this phase */
override def runsAfter: Set[String] = Set(InterceptedMethods.name, Splitter.name, ElimRepeated.name)
override def runsAfter: Set[String] = Set(InterceptedMethods.name, ElimRepeated.name)

override def changesMembers: Boolean = true // the phase adds bridges
override def changesParents: Boolean = true // the phase drops Any
Expand Down
137 changes: 0 additions & 137 deletions compiler/src/dotty/tools/dotc/transform/Splitter.scala

This file was deleted.

7 changes: 6 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2365,7 +2365,12 @@ class Typer extends Namer
}
} else issueErrors()
}
else readaptSimplified(tpd.Apply(tree, args))
else tree match {
case tree: Block =>
readaptSimplified(tpd.Block(tree.stats, tpd.Apply(tree.expr, args)))
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added an assert in tpd.Apply, it seems this can never happen.

case _ =>
readaptSimplified(tpd.Apply(tree, args))
}
}
addImplicitArgs(argCtx(tree))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import vulpix.TestConfiguration
class PatmatExhaustivityTest {
val testsDir = "tests/patmat"
// stop-after: patmatexhaust-huge.scala crash compiler
Copy link
Member

Choose a reason for hiding this comment

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

Does it still crash the compiler with the new labelled blocks ?

Copy link
Contributor Author

@liufengyun liufengyun Oct 19, 2018

Choose a reason for hiding this comment

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

Just tried locally, it works now.

Edit: CI not happy, reverted.

val options = List("-color:never", "-Ystop-after:splitter", "-Ycheck-all-patmat", "-classpath", TestConfiguration.basicClasspath)
val options = List("-color:never", "-Ystop-after:crossCast", "-Ycheck-all-patmat", "-classpath", TestConfiguration.basicClasspath)

private def compileFile(path: JPath) = {
val stringBuffer = new StringWriter()
Expand Down
9 changes: 9 additions & 0 deletions tests/pos/i5107.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class Test {
inline def foo(x: Int = 5)(implicit y: Int): Int =
if (x > 0) y * y
else y + y

implicit val m: Int = 7

(new Test).foo()
}