Skip to content

Fix ClassCastException upon class Foo extends Int => <someTerm> #4487

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 2 commits into from
Jan 24, 2019

Conversation

Blaisorblade
Copy link
Contributor

To successfully typecheck

class Foo extends someTree

we need to detect correctly if someTree should be typechecked as a term or a
type.

If someTree is Int => 1, dotty crashes because typedFunction sees that Tree.isTerm on Int => this (or, untpd.Function(List('Int), 'this)) gives true and calls typedFunctionValue to typecheck this as a term. typedFunctionValue then crashes on an unchecked pattern match, because 'Int becomes an Ident and not a ValDef as it expects.

Instead, I think Int => 1 should be typechecked as a type, though I'm open to feedback.

Really, we shouldn't call isType at all, because the user might write a type in
place of a term or viceversa. I think we only want to know this is a constructor
call or a type; and maybe we should just let the parser tell us which, since it
knows.

Minimized from a testcase in #4389, in particular:

// 0f12f42d878dcd4cb8b8ab62dbfb6b41861989b1.scala
object i0 {
def main(i1: Array[String]): Unit = {
class i2
}
class i3(i4: => String) extends (i1 => (this
19): Option[String, Int] => 1
}

which triggers

Exception in thread "main" java.lang.ClassCastException: dotty.tools.dotc.ast.Trees$Ident cannot be cast to dotty.tools.dotc.ast.Trees$ValDef
	at dotty.tools.dotc.typer.Typer.$anonfun$11(Typer.scala:922)
	at scala.collection.TraversableLike$WithFilter.$anonfun$map$2(TraversableLike.scala:739)
	at scala.collection.immutable.List.foreach(List.scala:389)
	at scala.collection.TraversableLike$WithFilter.map(TraversableLike.scala:738)
	at dotty.tools.dotc.typer.Typer.typedFunctionValue(Typer.scala:926)
	at dotty.tools.dotc.typer.Typer.typedFunction(Typer.scala:756)
	at dotty.tools.dotc.typer.Typer.typedUnnamed$1(Typer.scala:1790)
	at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1832)
	at dotty.tools.dotc.typer.Typer.typedUnnamed$1(Typer.scala:1819)
	at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1832)
	at dotty.tools.dotc.typer.Typer.op1$2(Typer.scala:1863)
	at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1859)
	at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1871)
	at dotty.tools.dotc.typer.Typer.typedExpr(Typer.scala:1931)
	at dotty.tools.dotc.typer.Typer.typedParent$2(Typer.scala:1491)
	at dotty.tools.dotc.typer.Typer.$anonfun$29(Typer.scala:1525)
	at dotty.tools.dotc.core.Decorators$ListDecorator$.loop$1(Decorators.scala:62)
	at dotty.tools.dotc.core.Decorators$ListDecorator$.mapconserve$extension(Decorators.scala:78)
	at dotty.tools.dotc.typer.Typer.typedClassDef(Typer.scala:1453)
	at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:1772)
[...]

@Blaisorblade
Copy link
Contributor Author

Assigned to Martin as decided in the meeting.

odersky
odersky previously requested changes Jul 1, 2018
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I agree with the reasoning, but think we can cut it even more clearly. If you look at what the parser does (in constrApp), it produces either a type or a New expression applied to arguments. So I believe we can just check whether the parent is an Apply node and type it as a term, or type it as a type otherwise.

Can you try this and see whether it works?

@odersky
Copy link
Contributor

odersky commented Jan 12, 2019

@Blaisorblade Inactive for 6 months. What will happen with this?

@Blaisorblade Blaisorblade force-pushed the fix-classcastexception branch 2 times, most recently from aee57aa to 2361d2c Compare January 15, 2019 19:52
@Blaisorblade Blaisorblade dismissed odersky’s stale review January 15, 2019 20:48

Addressed comments

@Blaisorblade Blaisorblade requested a review from biboudis January 15, 2019 20:48
@Blaisorblade Blaisorblade removed the request for review from biboudis January 15, 2019 20:52
@@ -1589,8 +1589,7 @@ class Typer extends Namer
def isTreeType(t: untpd.Tree): Boolean = t match {
case _: untpd.Apply => false
case _: untpd.Select =>
// Could be produced by parsing incomplete argument lists.
assert(false, "Raw Select node found in constructor position")
// Can be produced for absent argument lists.

This comment was marked as outdated.

@Blaisorblade Blaisorblade force-pushed the fix-classcastexception branch from 1b7f94f to d21db0a Compare January 16, 2019 00:13
@Blaisorblade
Copy link
Contributor Author

Last commit is from #5721 to allow tests to pass.

To typecheck
```scala
class Foo extends someTree
```
we need to detect correctly if `someTree` should be typechecked as a term or a
type; `Int => 1` should be typechecked as a type, even though it looks like a
term according to `isTerm`.

Really, we shouldn't use isType at all, because the user might write a type in
place of a term or viceversa. I think we only want to know this is a constructor
call or a type; and maybe we should just let the parser tell us which, since it
knows.
@Blaisorblade Blaisorblade force-pushed the fix-classcastexception branch from d21db0a to 2732c6b Compare January 22, 2019 15:23
Copy link
Contributor

@biboudis biboudis left a comment

Choose a reason for hiding this comment

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

I tested it with a partial functions and implicit function types and it works. They could be added as tests but this looks already good.

@biboudis biboudis merged commit 450a6a8 into scala:master Jan 24, 2019
@biboudis biboudis deleted the fix-classcastexception branch January 24, 2019 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants