Skip to content

Fix #9462: Identify inserted apply's more reliably in the Dynamic treatment. #9582

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
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
18 changes: 17 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,23 @@ trait Applications extends Compatibility {

fun1.tpe match {
case err: ErrorType => cpy.Apply(tree)(fun1, proto.typedArgs()).withType(err)
case TryDynamicCallType => typedDynamicApply(tree, pt)
case TryDynamicCallType =>
val isInsertedApply = fun1 match {
case Select(_, nme.apply) => fun1.span.isSynthetic
case TypeApply(sel @ Select(_, nme.apply), _) => sel.span.isSynthetic
/* TODO Get rid of this case. It is still syntax-based, therefore unreliable.
* It is necessary for things like `someDynamic[T](...)`, because in that case,
* somehow typedFunPart returns a tree that was typed as `TryDynamicCallType`,
* so clearly with the view that an apply insertion was necessary, but doesn't
* actually insert the apply!
* This is probably something wrong in apply insertion, but I (@sjrd) am out of
* my depth there.
* In the meantime, this makes tests pass.
*/
case TypeApply(fun, _) => !fun.isInstanceOf[Select]
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also check that the name of fun has a synthetic span?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, no

Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of trees do we get here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anything. For the test that was failing, it was TypeApply(Ident("qual"), List(oneTArg)).

Copy link
Member Author

Choose a reason for hiding this comment

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

Where I was expecting typedFunPart to have returned TypeApply(Select(Ident("qual"), nme.apply), List(oneTarg)) instead, where the Select node would have had a synthetic span.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we can get a TypeApply(Block(..., Select(..., nme.apply)), ...). I guess those would never be inserted applies.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, in that case, for an inserted apply, the Select should be around the Block, not inside it.

case _ => false
}
typedDynamicApply(tree, isInsertedApply, pt)
case _ =>
if (originalProto.isDropped) fun1
else if (fun1.symbol == defn.Compiletime_summonFrom)
Expand Down
27 changes: 17 additions & 10 deletions compiler/src/dotty/tools/dotc/typer/Dynamic.scala
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ trait Dynamic {
* foo.bar(x = bazX, y = bazY, baz, ...) ~~> foo.applyDynamicNamed("bar")(("x", bazX), ("y", bazY), ("", baz), ...)
* foo.bar[T0, ...](x = bazX, y = bazY, baz, ...) ~~> foo.applyDynamicNamed[T0, ...]("bar")(("x", bazX), ("y", bazY), ("", baz), ...)
*/
def typedDynamicApply(tree: untpd.Apply, pt: Type)(using Context): Tree = {
def typedDynamicApply(tree: untpd.Apply, isInsertedApply: Boolean, pt: Type)(using Context): Tree = {
def typedDynamicApply(qual: untpd.Tree, name: Name, selSpan: Span, targs: List[untpd.Tree]): Tree = {
def isNamedArg(arg: untpd.Tree): Boolean = arg match { case NamedArg(_, _) => true; case _ => false }
val args = tree.args
Expand All @@ -79,15 +79,22 @@ trait Dynamic {
}
}

tree.fun match {
case sel @ Select(qual, name) if !isDynamicMethod(name) =>
typedDynamicApply(qual, name, sel.span, Nil)
case TypeApply(sel @ Select(qual, name), targs) if !isDynamicMethod(name) =>
typedDynamicApply(qual, name, sel.span, targs)
case TypeApply(fun, targs) =>
typedDynamicApply(fun, nme.apply, fun.span, targs)
case fun =>
typedDynamicApply(fun, nme.apply, fun.span, Nil)
if (isInsertedApply) {
tree.fun match {
case TypeApply(fun, targs) =>
typedDynamicApply(fun, nme.apply, fun.span, targs)
case fun =>
typedDynamicApply(fun, nme.apply, fun.span, Nil)
}
} else {
tree.fun match {
case sel @ Select(qual, name) if !isDynamicMethod(name) =>
typedDynamicApply(qual, name, sel.span, Nil)
case TypeApply(sel @ Select(qual, name), targs) if !isDynamicMethod(name) =>
typedDynamicApply(qual, name, sel.span, targs)
case _ =>
errorTree(tree, em"Dynamic insertion not applicable")
}
}
}

Expand Down
18 changes: 7 additions & 11 deletions project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,7 @@ object Build {
-- "InteroperabilityTest.scala" // various compile errors
-- "OptimizerTest.scala" // compile errors: false + string and () + string
-- "ReflectionTest.scala" // tests fail
-- "RegressionJSTest.scala" // compile error with js.Dynamic.literal
-- "RegressionJSTest.scala" // non-native JS classes
-- "RuntimeTypesTest.scala" // compile errors: no ClassTag for Null and Nothing
)).get

Expand All @@ -1081,23 +1081,22 @@ object Build {

++ (dir / "js/src/test/scala/org/scalajs/testsuite/jsinterop" ** (("*.scala": FileFilter)
-- "AsyncTest.scala" // needs PromiseMock.scala
-- "DynamicTest.scala" // compile error with js.Dynamic.literal
-- "DynamicTest.scala" // one test requires JS exports, all other tests pass
-- "ExportsTest.scala" // JS exports
-- "FunctionTest.scala" // IR checking errors
-- "IterableTest.scala" // non-native JS classes
-- "JSExportStaticTest.scala" // JS exports
-- "JSNameTest.scala" // compile error with js.Dynamic.literal
-- "JSNativeInPackage.scala" // IR checking errors
-- "JSOptionalTest.scala" // non-native JS classes
-- "JSSymbolTest.scala" // compile error with js.Dynamic.literal
-- "MiscInteropTest.scala" // compile error with js.Dynamic.literal
-- "JSSymbolTest.scala" // non-native JS classes
-- "MiscInteropTest.scala" // non-native JS classes
-- "ModulesWithGlobalFallbackTest.scala" // non-native JS classes
-- "NestedJSClassTest.scala" // non-native JS classes
-- "NonNativeJSTypeTest.scala" // non-native JS classes
-- "PromiseMock.scala" // non-native JS classes
-- "SpecialTest.scala" // compile error with js.Dynamic.literal
-- "SpecialTest.scala" // assertion error in ExpandSAMs
-- "SymbolTest.scala" // IR checking errors
-- "ThisFunctionTest.scala" // compile error with js.Dynamic.literal
-- "ThisFunctionTest.scala" // assertion error in ExpandSAMs
-- "UndefOrTest.scala" // StackOverflow in the compiler
)).get

Expand All @@ -1118,10 +1117,7 @@ object Build {
)).get

++ (dir / "js/src/test/scala/org/scalajs/testsuite/niobuffer" ** "*.scala").get

++ (dir / "js/src/test/scala/org/scalajs/testsuite/scalalib" ** (("*.scala": FileFilter)
-- "ScalaRunTimeJSTest.scala" // compile error with js.Dynamic.literal
)).get
++ (dir / "js/src/test/scala/org/scalajs/testsuite/scalalib" ** "*.scala").get

++ (dir / "js/src/test/scala/org/scalajs/testsuite/typedarray" ** (("*.scala": FileFilter)
-- "TypedArrayTest.scala" // assertion error in ExpandSAMs
Expand Down
58 changes: 58 additions & 0 deletions tests/run/dynamicDynamicTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ object Test {
def main(args: Array[String]): Unit = {
runFooTests1()
runFooTests2()
runFooTests3()
runBarTests()
runBazTests()
runQuxTests()
Expand Down Expand Up @@ -102,6 +103,63 @@ object Test {
assertEquals("selectDynamic(bazSelectUpdate).update(key, value)", foo.bazSelectUpdate("key") = "value")
}

/** Test apply insertions kick in before dynamic calls. */
def runFooTests3() = {
val foo = new Foo

assertEquals("applyDynamic(apply)()", foo())
assertEquals("applyDynamic(apply)(1)", foo(1))
assertEquals("applyDynamic(apply)(1, 2, 3)", foo(1, 2, 3))
assertEquals("applyDynamic(apply)(1, 2, a)", foo(1, 2, "a"))
assertEquals("applyDynamic(apply)(1, 2, a)", foo(List(1, 2, "a"): _*))

assertEquals("applyDynamicNamed(apply)((a,1))", foo(a = 1))
assertEquals("applyDynamicNamed(apply)((a,1), (b,2))", foo(a = 1, b = 2))
assertEquals("applyDynamicNamed(apply)((a,1), (,0))", foo(a = 1, 0))
assertEquals("applyDynamicNamed(apply)((a,1), (a,5))", foo(a = 1, a = 5))
assertEquals("applyDynamicNamed(apply)((,d), (a,1), (,5), (a,c))", foo("d", a = 1, 5, a = 'c'))

assertEquals("applyDynamic(apply)()", foo.apply())
assertEquals("applyDynamic(apply)(1)", foo.apply(1))
assertEquals("applyDynamic(apply)(1, 2, 3)", foo.apply(1, 2, 3))
assertEquals("applyDynamic(apply)(1, 2, a)", foo.apply(1, 2, "a"))
assertEquals("applyDynamic(apply)(1, 2, a)", foo.apply(List(1, 2, "a"): _*))

assertEquals("applyDynamicNamed(apply)((a,1))", foo.apply(a = 1))
assertEquals("applyDynamicNamed(apply)((a,1), (b,2))", foo.apply(a = 1, b = 2))
assertEquals("applyDynamicNamed(apply)((a,1), (,0))", foo.apply(a = 1, 0))
assertEquals("applyDynamicNamed(apply)((a,1), (a,5))", foo.apply(a = 1, a = 5))
assertEquals("applyDynamicNamed(apply)((,d), (a,1), (,5), (a,c))", foo.apply("d", a = 1, 5, a = 'c'))

object bar {
val foo: Foo = new Foo
}

assertEquals("applyDynamic(apply)()", bar.foo())
assertEquals("applyDynamic(apply)(1)", bar.foo(1))
assertEquals("applyDynamic(apply)(1, 2, 3)", bar.foo(1, 2, 3))
assertEquals("applyDynamic(apply)(1, 2, a)", bar.foo(1, 2, "a"))
assertEquals("applyDynamic(apply)(1, 2, a)", bar.foo(List(1, 2, "a"): _*))

assertEquals("applyDynamicNamed(apply)((a,1))", bar.foo(a = 1))
assertEquals("applyDynamicNamed(apply)((a,1), (b,2))", bar.foo(a = 1, b = 2))
assertEquals("applyDynamicNamed(apply)((a,1), (,0))", bar.foo(a = 1, 0))
assertEquals("applyDynamicNamed(apply)((a,1), (a,5))", bar.foo(a = 1, a = 5))
assertEquals("applyDynamicNamed(apply)((,d), (a,1), (,5), (a,c))", bar.foo("d", a = 1, 5, a = 'c'))

assertEquals("applyDynamic(apply)()", bar.foo.apply())
assertEquals("applyDynamic(apply)(1)", bar.foo.apply(1))
assertEquals("applyDynamic(apply)(1, 2, 3)", bar.foo.apply(1, 2, 3))
assertEquals("applyDynamic(apply)(1, 2, a)", bar.foo.apply(1, 2, "a"))
assertEquals("applyDynamic(apply)(1, 2, a)", bar.foo.apply(List(1, 2, "a"): _*))

assertEquals("applyDynamicNamed(apply)((a,1))", bar.foo.apply(a = 1))
assertEquals("applyDynamicNamed(apply)((a,1), (b,2))", bar.foo.apply(a = 1, b = 2))
assertEquals("applyDynamicNamed(apply)((a,1), (,0))", bar.foo.apply(a = 1, 0))
assertEquals("applyDynamicNamed(apply)((a,1), (a,5))", bar.foo.apply(a = 1, a = 5))
assertEquals("applyDynamicNamed(apply)((,d), (a,1), (,5), (a,c))", bar.foo.apply("d", a = 1, 5, a = 'c'))
}

/** Test cains of dynamic calls. */
def runBarTests() = {
val bar = new Bar("bar")
Expand Down