Scala.js: Support for reflective calls. #9427

sjrd · 2020-07-24T12:14:06Z

~~Based on #9420 (the first commit).~~

This is significantly different from the code in scalac. In scalac, reflective calls reach the back-end as dedicated ApplyDynamic nodes. In dotc however, they are expanded by the type-checker into user-level calls to method of scala.reflect.Selectable.

We must therefore reverse-engineer the patterns of code produced by the compiler to construct SJSIR Apply nodes with reflective proxy method names.

The big comment on JSCodeGen.genReflectiveCall explains the translation in detail.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

library/src/scala/Selectable.scala

compiler/src/dotty/tools/dotc/ast/tpd.scala

smarter · 2020-07-24T15:49:55Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+    val receiver = receiver0 match {
+      case Apply(fun1, receiver :: _)
+          if fun1.symbol == jsdefn.ReflectSelectable_reflectiveSelectable ||
+              fun1.symbol == jsdefn.Selectable_reflectiveSelectableFromLangReflectiveCalls =>


I wonder if you could make that conversion method inline to avoid having to deal with it here.

I tried that initially, but if I do that, then suddenly Type Mismatch errors anywhere suggests to import that implicit conversion because it could "make progress towards a solution" ... even though the result type is monomorphically scala.reflect.Selectable, even for type mismatches like "Expected String but found Boolean".

I think the suggestion mechanism assumes that any inline implicit def could return a more precise type than what is declared (because you know, blackbox macros are just whitebox macros with a special body, so nobody can tell from the outside, which is a problem that seems to pop up every now and then in various scenarios).

I think the suggestion mechanism assumes that any inline implicit def could return a more precise type than what is declared (because you know, blackbox macros are just whitebox macros with a special body, so nobody can tell from the outside, which is a problem that seems to pop up every now and then in various scenarios).

@nicolasstucki What's the current status of whether transparent should be a flag or not? Is this a known issue with the current encoding?

Anyway, for this specific case, I figured it would be better not to have it inline, otherwise it gets sketchy to rely on the fact that the inliner will preserve the shape of the tree, and in particular that the fun1 directly inside the Apply will indeed be the call to reflectiveSelectable (and not a Block that ends in a call to reflectiveSelectable, notably).

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

smarter · 2020-07-24T15:53:26Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+              "Passing the varargs of Selectable.applyDynamic with `: _*` " +
+              "is not supported in Scala.js.",


Do we have neg tests for the various error conditions in this method?

No. I'm not sure yet how to write negative tests for Scala.js-specific error messages.

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

gzm0 · 2020-07-24T16:25:27Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+   *  - the actual arguments as a compile-time `List[Tree]`
+   *
+   *  Virtually all of the code in `genReflectiveCall` deals with recovering
+   *  those elements. Constructing the IR Tree is the easy part after that.


Using a thing like scala.reflect.Selectable seems like a neat trick to easily implement reflective calls on backends that support reflection.

However, here it seems that this trick is now doing the entire compiler a disservice: We have ~100 LOC simply reconstructing things and then a single LOC actually issuing the call. (and additionally failures that IIUC simply cannot happen with reflective calls).

Have you considered introducing a form of transient tree and doing the translation to a call to Selectable later? (I realize that the call relies on other transformations that happen earlier, so it might be worse overall).

I agree that the situation is very inconvenient. But the desugaring that is now done in Typer is actually specced as is now. It's not "a trick". It is the normal way of compiling calls to members of structural types. reflectiveSelectable is just one possible implicit that can provide an implementation for those, but there can be other, user-defined implementation (that don't really on Java reflection, but on an internal Map, for example).

The reference is here: https://dotty.epfl.ch/docs/reference/changed-features/structural-types.html

We need to support reflectiveSelectable because it corresponds to Scala 2 reflective calls, and it's the only way that our back-end would emit calls to reflective proxies (which we really need in some cases; we use them in the JDK implem in Scala.js itself for example).

Users can implement other Selectables not relying on Java run-time reflection, and those would also work.

So we can't introduce a form of transient tree to be desugared later. The desugaring is by spec and requires interactions with other tasks of typechecking, including implicit resolution. It's impossible to delay that.

Hmmm... I see. So is the expectation that any custom Selectable that needs compile time reflection needs to do this ceremony? The page you linked doesn't go into details about that.

Further, if that is the case, shouldn't we introduce another Selectable for Scala.js? The main issue I see here is that with the current specification, it would be allowed to use reflectiveSelectable on its own, without structural types. However, such use would immediately lead to non-portable code. Maybe we should make this more obvious?

I realize this is probably a larger discussion, so you might want to go ahead with this PR. I still think it is worth thinking about this a bit more.

gzm0 · 2020-07-24T16:31:25Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+
+    val methodName = MethodName.reflectiveProxy(methodNameStr, formalParamTypeRefs)
+
+    js.Apply(js.ApplyFlags.empty, receiver, js.MethodIdent(methodName), actualArgs)(jstpe.AnyType)


Judging from the code of reflect.Selectable, selectDynamic is able to select actual bytecode fields (as opposed to accessors). Then again, it falls back to applyDynamic.

I do not know the details of how dotty translates fields, but it seems this is problematic for exact compatibility.

On the JVM, we need to look up fields because of Java interop. A Java class such as

public class A { public int foo = 5; }

qualifies for the Scala structural type

{ val foo: Int }

so for Java we need to try fields.

In the SJSIR, however, since JDK classes are implemented in Scala anyway, I don't think there can be an issue.

Ah, OK. Fair enough.

sjrd · 2020-07-26T09:22:26Z

Updated to address the reviews.

The translation of `classOf[Prim]` (e.g., `classOf[Int]`) to `BoxedPrimClass.TYPE` (e.g., `j.l.Integer.TYPE`) is specific to the JVM back-end, because the JVM bytecode does not have any instruction to load them. Other back-ends have appropriate opcodes and prefer to receive them as `Constant(Literal(primType))`, so that they don't have to reverse-engineer the `Select`s. Since that transformation is not harder to do in the back-end, we move it directly to the JVM codegen. The code is actually shorter there. This also helps to keep the trees a bit smaller across the phases after erasure.

This is significantly different from the code in scalac. In scalac, reflective calls reach the back-end as dedicated `ApplyDynamic` nodes. In dotc however, they are expanded by the type-checker into user-level calls to method of `scala.reflect.Selectable`. We must therefore reverse-engineer the patterns of code produced by the compiler to construct SJSIR `Apply` nodes with reflective proxy method names. The big comment on `JSCodeGen.genReflectiveCall` explains the translation in detail.

sjrd · 2020-07-28T12:52:15Z

Ping @smarter. Is there still something you'd like me to do here? How much do you care about the neg tests at this point? (at some point I'll need to figure them out, because we've got loads of Scala.js-specific error messages in our front-end phase, which does not exist at all in dotc)

smarter

The neg tests can wait.

sjrd requested review from smarter and gzm0 July 24, 2020 12:14

dottybot reviewed Jul 24, 2020

View reviewed changes

sjrd force-pushed the scalajs-reflective-calls branch from 30cb648 to c99b7e0 Compare July 24, 2020 13:45

smarter reviewed Jul 24, 2020

View reviewed changes

library/src/scala/Selectable.scala Outdated Show resolved Hide resolved

smarter requested changes Jul 24, 2020

View reviewed changes

gzm0 reviewed Jul 24, 2020

View reviewed changes

nicolasstucki assigned sjrd Jul 24, 2020

sjrd force-pushed the scalajs-reflective-calls branch from c99b7e0 to 73b34b1 Compare July 26, 2020 09:11

sjrd requested review from gzm0 and smarter July 26, 2020 09:22

sjrd assigned smarter and gzm0 and unassigned sjrd Jul 26, 2020

gzm0 approved these changes Jul 27, 2020

View reviewed changes

sjrd added 3 commits July 27, 2020 11:02

Scala.js: Do not emit definitions for primitive methods.

ea641d3

sjrd force-pushed the scalajs-reflective-calls branch from 73b34b1 to d5e8831 Compare July 27, 2020 09:02

sjrd unassigned gzm0 Jul 27, 2020

smarter approved these changes Jul 28, 2020

View reviewed changes

sjrd merged commit 2ff15be into scala:master Jul 28, 2020

sjrd deleted the scalajs-reflective-calls branch July 28, 2020 13:20

		"Passing the varargs of Selectable.applyDynamic with `: _*` " +
		"is not supported in Scala.js.",


		val methodName = MethodName.reflectiveProxy(methodNameStr, formalParamTypeRefs)

		js.Apply(js.ApplyFlags.empty, receiver, js.MethodIdent(methodName), actualArgs)(jstpe.AnyType)

Scala.js: Support for reflective calls. #9427

Scala.js: Support for reflective calls. #9427

Uh oh!

Conversation

sjrd commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Jul 26, 2020

Uh oh!

sjrd commented Jul 28, 2020

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjrd commented Jul 24, 2020 •

edited

Loading