Fix #1997: Add support for js.Symbols in @JSName annotations. #2659

gzm0 · 2016-11-21T15:20:34Z

(Old PR description below)

The @JSName annotation could previously be used to specify the JavaScript string name of a property/method, independently of its Scala name. This can be used both in native JS classes as well as Scala.js-defined JS classes.

This commit extends the capability of @JSName to admit a (static, stable) reference to a js.Symbol. This applies to both native and Scala.js-defined JS types.

At call site, the semantics and compilation scheme are straightforward: it is a JSBracketSelect whose item part is a call to the symbol accessor. Since it is static by construction, we can call it from anywhere.

At definition site for native JS types, there is nothing to do (besides validity checks).

At definition site for Scala.js-defined JS classes, things get tricky. We can now have "exported" class members whose name is not a constant string anymore. This requires to extend the IR with ComputedNames (a concept that exists in ECMAScript 2015). A computed name is an arbitrary expression Tree that is evaluated to obtain the dynamic name of a property. This complicates the Emitter somewhat, but not that much because Scala.js-defined JS classes are created lazily, so we can execute arbitrary user-defined code at class-definition time.

For overloading resolution, ComputedNames have a logicalName, derived from the fully qualified name of the symbol accessors. All ComputedName'd properties/methods with the same logical name (i.e., referencing the same static stable js.Symbol) are considered to be overloaded. This has an unavoidable caveat: if two ComputedNames in the same class have a different logical name, but actually evaluate to the same symbol, things will go wrong at run-time.

JSObjectConstr is enhanced to support ComputedNames as well, for consistency. It is also consistent with Object Initializers in ECMAScript 2015. This actually complicates their treatment, mostly because of evaluation order concerns.

Exports (in Scala classes and at the top-level) cannot use symbolic names. This is not expected to change in the future.

There is one implementation restriction: @JSName with a symbol is not supported on native JS classes and objects that are nested within a native JS object. There is no language reason to disallow this, but the implementation is surprisingly difficult, due to the way scalac flattens such entities.

Old PR description

We introduce @JSSymbol as a pendant to @JsName to define member access through ES6 symbols. On a high level, the following happens in the compiler:

PrepJSInterop / Definition Site

Take the content of @JSSymbol annotations and put it into a special method, the "symbol forwarder". This is necessary, since otherwise the trees won't get compiled. As a nice side effect, it allows changing the symbol without breaking binary compatibility.

Further, we replace the tree inside @JSSymbol with a simple tree calling the symbol forwarder. This tree will not get compiled, but we will only use its symbol.

PrepJSInterop / Call Site

Nothing.

GenJSCode / Definition Site

We emit symbol forwarders as static methods on the native JS class. This does not need an IR change, since this is already supported (but was unused so far).

For traits pre 2.12.0, this is a bit more involved, since the symbol forwarder gets moved to the implementation class. We retrieve the implementation class and move the forwarder.

GenJSCode / Call Site

Instead of generating a string literal inside the bracket select, we generate a call to the symbol forwarder. The symbol forwarder's symbol is conveniently available inside the tree of the @JSSymbol annotation.

Caveat

Since we generate number suffixes to disambiguate symbol forwarder overloads, adding an overload to a method with @JSSymbol is a potentially binary breaking change.

gzm0 · 2016-11-21T15:20:46Z

Review by @sjrd

scala-jenkins · 2016-11-21T17:19:39Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3496/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4603/
Test FAILed.

scala-jenkins · 2016-11-21T17:19:39Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3495/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4602/
Test FAILed.

sjrd

Reviewed until JSDefinitions.scala so far. Will continue later.

sjrd · 2016-11-21T17:00:37Z

library/src/main/scala/scala/scalajs/js/Symbol.scala

+  }
+
+  @inline def apply(): Symbol = NativeSymbol()
+  @inline def apply(description: String): Symbol = NativeSymbol()


NativeSymbol(description)?

sjrd · 2016-11-21T17:02:43Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

+         * - Implementation classes for native JS classes. The only useful
+         *   thing they contain are symbol forwarders for @JSSymbol access.
+         *   We want to emit the symbol forwarders as static methods on the
+         *   native JS class so we do not need to generate another class file.


another sjsir file?

sjrd · 2016-11-21T17:17:12Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

+          val patchedDef = {
+            if (scalaUsesImplClasses && sym.owner.isTraitOrInterface) {
+              assert(sym.isDeferred, "Found non-abstract method in trait at " +
+                s"${dd.pos}: ${sym.fullName}")


sjrd · 2016-11-21T17:18:13Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

+                s"${dd.pos}: ${sym.fullName}")
+
+              /* We grab the body from the implementation class. This does not
+               * work so directly in general, since the parameter symbols and


"so directly" does not sound right.

sjrd · 2016-11-21T17:18:48Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

+            } else {
+              assert(!sym.isDeferred, "Found an abstract symbol forwarder at " +
+                  s"${dd.pos}: ${sym.fullName}")
+              dd  // No patching necessary.


There are 2 spaces between dd and //

sjrd · 2016-11-21T17:22:10Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

@@ -1255,7 +1305,8 @@ abstract class GenJSCode extends plugins.PluginComponent
        if (scalaPrimitives.isPrimitive(sym) &&
            !jsPrimitives.shouldEmitPrimitiveBody(sym)) {
          None
-        } else if (isAbstractMethod(dd)) {
+        } else if (isAbstractMethod(dd) && !(scalaUsesImplClasses &&
+            jsInterop.isSymbolForwarder(sym))) {


Is scalaUsesImplClasses && necessary here?
If yes, can you put it on the next line, so that the line break goes with the outer-most && rather than the inner-most one?

sjrd · 2016-11-21T17:28:05Z

In general, I'm concerned that we add this for native JS classes without knowing what to do about Scala.js-defined JS classes. I would like both things to be addressed at the same time.

For Scala.js-defined JS classes, it means we need a definition-time access to calling the mentioned symbol. This in turn means that we probably need to create Scala.js-defined JS classes lazily, when they are first required. (this might mean immediately if they are exported).

gzm0 · 2016-11-21T18:46:23Z

In general, I'm concerned that we add this for native JS classes without knowing what to do about Scala.js-defined JS classes. I would like both things to be addressed at the same time.

That is a very good point :-/ I already had the feeling that we should impose some restrictions on the contents of @JSSymbol, but I wasn't able to come up with anything sensible...

scala-jenkins · 2017-01-11T18:32:22Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3601/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4757/
Test FAILed.

scala-jenkins · 2017-01-19T22:07:06Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3646/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4809/
Test FAILed.

scala-jenkins · 2017-02-06T11:42:30Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3670/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4848/
Test FAILed.

scala-jenkins · 2017-02-07T17:01:05Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3675/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4854/
Test FAILed.

scala-jenkins · 2017-02-08T12:18:25Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3676/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4856/
Test FAILed.

scala-jenkins · 2017-02-08T16:31:12Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3677/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4857/
Test FAILed.

scala-jenkins · 2017-02-08T18:34:19Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3678/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4858/
Test FAILed.

scala-jenkins · 2017-02-08T19:40:31Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3679/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4859/
Test PASSed.

sjrd · 2017-02-08T19:44:18Z

Yeah, managed to make the CI pass again. Now on to write more tests!

scala-jenkins · 2017-02-09T18:36:50Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3680/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4861/
Test PASSed.

scala-jenkins · 2017-02-09T19:05:38Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3681/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4862/
Test FAILed.

scala-jenkins · 2017-02-10T12:40:50Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3682/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4863/
Test PASSed.

scala-jenkins · 2017-02-10T13:53:46Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3683/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4864/
Test FAILed.

sjrd · 2017-02-10T17:23:27Z

Since we decided to restrict @JSName not to admit symbols for top-level classes and objects, the changes to the format of JSNativeLoadSpec were not necessary anymore. I have reverted them. I'm keeping a backup commit with the changes in my branch https://github.com/sjrd/scala-js/tree/save-jsnativeloadspec-with-propertynames

scala-jenkins · 2017-02-10T17:28:49Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3684/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4865/
Test PASSed.

gzm0 · 2017-02-10T18:12:10Z

Euh, how do you handle nested classes / objects that have a @JSName(sym)? I thought we wanted to allow these...

sjrd · 2017-02-15T10:18:44Z

Aaannd ... rebased and cleaned up the history!

Sending back the review by @gzm0 now ;)

Edit: GitHub won't let me assign you as a reviewer, probably because you created the PR.

scala-jenkins · 2017-02-15T12:14:56Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3696/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4882/
Test PASSed.

gzm0

Also seems I won't be able to approve my own PR :P

gzm0 · 2017-02-15T18:52:30Z

.../shared/src/main/scala/org/scalajs/core/tools/linker/backend/emitter/KnowledgeGuardian.scala

@@ -12,7 +12,7 @@ package org.scalajs.core.tools.linker.backend.emitter
 import scala.collection.mutable

 import org.scalajs.core.ir.ClassKind
-import org.scalajs.core.ir.Trees.{FieldDef, JSNativeLoadSpec}
+import org.scalajs.core.ir.Trees.{FieldDef, JSNativeLoadSpec, Ident}


Still useless import.

gzm0 · 2017-02-15T18:58:14Z

library/src/main/scala/scala/scalajs/js/Symbol.scala

+    def keyFor(sym: Symbol): js.UndefOr[String] = js.native
+
+    val iterator: Symbol = js.native
+  }


I took this from your PR, but actually: Why do we have the NativeSymbol inside? Can't we just have @JSName('for') for forKey?

Because at some point we realized we would have want js.Array to be non-native, to include custom things in there. I'm mostly anticipating the need to evolve Symbol into more than just its facade.

Also, for 1.0 I think we should turn the type js.Symbol into a hijacked class. We have hijacked classes to represent all other JS primitive types, but not symbol. I think it would make sense. In that case, it would also be weird, possibly problematic, for the companion object to be a native.

But I don't have very strong feelings about this.

Don't you mean turning scala.Symbol into a hijacked class? I do get the story for js.Array, but js.Array is also much closer to the runtime. And I think between 0.6 and 1.0 its ok to have the theoretically source incompatible change of making js.Symbol not extend js.Any anymore.

No I really mean js.Symbol. scala.Symbol we already ruled out, because their semantics are too different from js.Symbol (#1541). However we should be able to do things like

x match { case x: js.Symbol => }

and we can only do that if x.isInstanceOf[js.Symbol] has a meaningful spec, i.e., type of x === 'symbol'. And that, we can only do if js.Symbol is a hijacked class.

I do get the story for js.Array, but js.Array is also much closer to the runtime.

IMO js.Symbol is much closer to the run-time than js.Array. For js.Array we're only optimizing its apply method a bit. js.Symbols creep into the very definition of the language, now that they're supported in @JSName.

And I think between 0.6 and 1.0 its ok to have the theoretically source incompatible change of making js.Symbol not extend js.Any anymore.

OK. Let's have an @js.native object Symbol for now, then.

gzm0 · 2017-02-15T18:58:39Z

library/src/main/scala/scala/scalajs/js/Symbol.scala

+  @inline def forKey(key: String): Symbol = NativeSymbol.`for`(key)
+  @inline def keyFor(sym: Symbol): js.UndefOr[String] = NativeSymbol.keyFor(sym)
+
+  @inline def iterator: Symbol = NativeSymbol.iterator


This must be a stable identifier. Otherwise defining an iterable will be problematic :)

And now that I think of it, we should probably add all well known symbols here.

gzm0 · 2017-02-15T19:02:27Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

+          genApplyJSClassMethod(module, sym, arguments = Nil)
+      } else {
+        genApplyMethod(module, sym, arguments = Nil)
+      }


I found this part particularly ugly when I wrote it. I guess you also tried and failed to consolidate with genApply :-/

I didn't really try that, because things are decomposed early into JS/Scala calls in the genApply giant dispatch. So seeing them all together like this never came up before. I think it's OK like that, if a bit ugly.

Ok. But what we should probably do, is add tests for all code paths here. Currently we only read symbols for definitions from Scala modules.

We probably should also have this nasty test case here:

@ScalaJSDefined object Foo extends js.Any { val sym = js.Symbol() @JSName(sym) def yolo(): Int = 1 }

We probably should also have this nasty test case here:

Well, that one doesn't work. It stack overflows. It makes sense.

Should we warn? Just like bad initialization order? (Not in this PR for sure).

gzm0 · 2017-02-15T19:04:36Z

compiler/src/main/scala/org/scalajs/core/compiler/JSEncoding.scala

@@ -239,6 +239,11 @@ trait JSEncoding extends SubComponent { self: GenJSCode =>
  def needsModuleClassSuffix(sym: Symbol): Boolean =
    sym.isModuleClass && !foreignIsImplClass(sym)

+  def encodeComputedNameIdentity(sym: Symbol): String = {


Maybe we should specify that this is compiler-private: Nothing may make any assumptions about the computed names of anything (except for their identity and starting with __computed). I realized that I have not documented this anywhere (was just documented in my head :P).

An alternative location might be the doc of PropertyName#encodedName.

PropertyName#encodedName seems a reasonable place for that.

gzm0 · 2017-02-16T17:02:56Z

compiler/src/test/scala/org/scalajs/core/compiler/test/JSInteropTest.scala

+        |newSource1.scala:11: error: Implementation restriction: @JSName with a js.Symbol is not supported on nested native classes and objects
+        |        @JSName(Sym.sym)
+        |         ^
+      """


I'm not sure I would make the distinction between this error message and the one above (at this point).

The error message above does not work here, because A really is a member of a JS type (Enclosing).

Yeah. Also since this works for objects, I'm fine with this.

gzm0 · 2017-02-16T17:06:37Z

compiler/src/test/scala/org/scalajs/core/compiler/test/JSInteropTest.scala

+      |
+      |    abstract class C extends A with B
+      |                   ^
+    """


I never realized that this is only a warning. Will this become an error? If no, we should:

Have a valid use case for it.

A way to silence the warning.

Both of these are of course completely orthogonal to this PR.

Basically all warnings of PrepJSInterop will become errors in 1.0. #1111

gzm0 · 2017-02-16T17:13:32Z

test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/JSSymbolTest.scala

+    val asTrait: OverloadedRuntimeDispatchMethodTrait = obj
+    assertEquals(6, asTrait.foo(3))
+    assertEquals("Hello Moon", asTrait.foo("Moon"))
+  }


For all of the sjs defined classes, I think we should also check that accessing them via the dynamic interface works. (meaning for now, a handwritten dynamic interface).

Hum, yes. It's kind of a consequence of the fact that reading via the trait works + the previous test passes. But it's not very self-contained. I'll add a "dynamic"-based test as well.

gzm0 · 2017-02-16T17:20:07Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

+                val value = genApplyMethod(tupRef, js.Ident("$$und2__O"), Nil,
+                    jstpe.AnyType)
+                keyToPropName(key, index) -> value
+            })


Wouldn't you want to retain the literals where you can here?

The optimizer will take care of that, if necessary.

gzm0 · 2017-02-16T17:24:16Z

compiler/src/main/scala/org/scalajs/core/compiler/JSTreeExtractors.scala

+        if (tuples.size == exprs.size)
+          Some(tuples)
+        else
+          None


OMG, I'm turning to the dark side... I just thought that this would be a prime use-case for

exps.sequence(Tuple2.unapply _)

except that Scala's List doesn't have a sequencing operator...

The encodedName is an identity we can use internally to track methods. However, it does not coincide with any real name, as we will see with the introduction of ComputedNames.

sjrd · 2017-02-17T15:03:58Z

Updated.

sjrd · 2017-02-17T15:52:18Z

I locally tested that we can depend on uTest and ScalaTest, and use them to test things with this branch. I believe this should give enough confidence regarding the ability to read old IR.

The `@JSName` annotation could previously be used to specify the JavaScript *string* name of a property/method, independently of its Scala name. This can be used both in native JS classes as well as Scala.js-defined JS classes. This commit extends the capability of `@JSName` to admit a (static, stable) reference to a `js.Symbol`. This applies to both native and Scala.js-defined JS types. At call site, the semantics and compilation scheme are straightforward: it is a `JSBracketSelect` whose `item` part is a call to the symbol accessor. Since it is static by construction, we can call it from anywhere. At definition site for native JS types, there is nothing to do (besides validity checks). At definition site for Scala.js-defined JS classes, things get tricky. We can now have "exported" class members whose name is not a constant string anymore. This requires to extend the IR with `ComputedName`s (a concept that exists in ECMAScript 2015). A computed name is an arbitrary expression `Tree` that is evaluated to obtain the dynamic name of a property. This complicates the Emitter somewhat, but not that much because Scala.js-defined JS classes are created lazily, so we can execute arbitrary user-defined code at class-definition time. For overloading resolution, `ComputedName`s have a `logicalName`, derived from the fully qualified name of the symbol accessors. All `ComputedName`'d properties/methods with the same logical name (i.e., referencing the same static stable `js.Symbol`) are considered to be overloaded. This has an unavoidable caveat: if two `ComputedName`s in the same class have a different logical name, but actually evaluate to the same symbol, things will go wrong at run-time. `JSObjectConstr` is enhanced to support `ComputedName`s as well, for consistency. It is also consistent with Object Initializers in ECMAScript 2015. This actually complicates their treatment, mostly because of evaluation order concerns. Exports (in Scala classes and at the top-level) cannot use symbolic names. This is not expected to change in the future. There is one implementation restriction: `@JSName` with a symbol is not supported on native JS classes and objects that are nested within a native JS object. There is no language reason to disallow this, but the implementation is surprisingly difficult, due to the way scalac flattens such entities.

Now that `JSObjectConstr` admits `ComputedName`s for keys, there are more patterns of calls of `js.Dynamic.literal` that can be emitted as `JSObjectConstr`. This commit leverages that.

scala-jenkins · 2017-02-17T16:04:12Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3706/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4893/
Test FAILed.

scala-jenkins · 2017-02-17T18:12:59Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3707/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4894/
Test PASSed.

gzm0 · 2017-02-20T09:21:28Z

LGTM (I can't approve the PR since it is mine).

gzm0 · 2017-02-20T09:21:50Z

I'll let you approve and merge (unless you have concerns :)).

gzm0 · 2017-02-20T10:03:57Z

Wooot 🎉

This commit implements the entire specification of non-native JS classes, including nested ones, except the behavior of *anonymous* JS classes. Anonymous JS classes are not supposed to have their own class/prototype, but currently they still do. The most important PRs to scala-js/scala-js that define what is implemented here were: 1. scala-js/scala-js#1809 Essentials of Scala.js-defined classes (former name of non-native JS classes) 2. scala-js/scala-js#1982 Support for secondary constructors 3. scala-js/scala-js#2186 Support for default parameters in constructors 4. scala-js/scala-js#2659 Support for `js.Symbol`s in `@JSName` annotations 5. scala-js/scala-js#3161 Nested JS classes However, this commit was written more based on the state of things at Scala.js v1.2.0 rather than as a port of the abovementioned PRs. The support is spread over 4 components: * The `ExplicitJSClasses` phase, which reifies all nested JS classes, and has extensive documentation in the code. * The `JSExportsGen` component of the back-end, which creates dispatchers for run-time overloading, default parameters and variadic arguments (equivalent to `GenJSExports` in Scala 2). * The `JSConstructorGen` component, which massages the constructors of JS classes and their dispatcher into a unique JS constructor. * Bits and pieces in `JSCodeGen`, notably to generate the `js.ClassDef`s for non-native JS classes, orchestrate the two other back-end components, and to adapt calls to the members of non-native JS classes. `JSConstructorGen` in particular is copied quasi-verbatim from pieces of `GenJSCode` in Scala 2, since it works on `js.IRNode`s, without knowledge of scalac/dotc data structures.

gzm0 assigned sjrd Nov 21, 2016

gzm0 force-pushed the symbol-support branch from 9a4c6aa to 27be302 Compare November 21, 2016 15:22

gzm0 changed the title ~~@JSSymbol support for native JS classes~~ Fix #1997: @JSSymbol support for native JS classes Nov 21, 2016

sjrd reviewed Nov 21, 2016

View reviewed changes

gzm0 force-pushed the symbol-support branch from 27be302 to f1852ac Compare January 11, 2017 12:15

gzm0 force-pushed the symbol-support branch from f1852ac to 5cf69c0 Compare January 19, 2017 20:56

sjrd force-pushed the symbol-support branch from 465c33a to f92b14c Compare February 7, 2017 15:48

sjrd force-pushed the symbol-support branch from f92b14c to 2c74a4a Compare February 8, 2017 10:56

sjrd force-pushed the symbol-support branch from d2dfc0d to 6adb0de Compare February 8, 2017 18:05

sjrd and others added 2 commits February 15, 2017 10:30

Add a test ensuring we preserve evaluation order in object literals.

b55b935

Remove javascript.Tree.PropertyName.name.

c3ec9ed

sjrd force-pushed the symbol-support branch from 531f32b to e6fd38d Compare February 15, 2017 10:18

sjrd changed the title ~~Fix #1997: @JSSymbol support for native JS classes~~ Fix #1997: Add support for js.Symbols in @JSName annotations. Feb 15, 2017

gzm0 self-assigned this Feb 15, 2017

gzm0 commented Feb 16, 2017

View reviewed changes

gzm0 and others added 3 commits February 17, 2017 11:04

Rename ir.PropertyName.name to encodedName.

c7d7758

The encodedName is an identity we can use internally to track methods. However, it does not coincide with any real name, as we will see with the introduction of ComputedNames.

Add the API of JavaScript symbols to the library.

b752175

Nicer pretty-printing of JS native load specs in the IR printer.

e9174fe

sjrd force-pushed the symbol-support branch from e6fd38d to 8014d23 Compare February 17, 2017 15:03

gzm0 and others added 2 commits February 17, 2017 17:03

Use JSObjectConstr more often for js.Dynamic.literal.

470f6cf

Now that `JSObjectConstr` admits `ComputedName`s for keys, there are more patterns of calls of `js.Dynamic.literal` that can be emitted as `JSObjectConstr`. This commit leverages that.

sjrd force-pushed the symbol-support branch from 8014d23 to 470f6cf Compare February 17, 2017 16:04

sjrd approved these changes Feb 20, 2017

View reviewed changes

sjrd merged commit da5a07c into scala-js:master Feb 20, 2017

gzm0 deleted the symbol-support branch February 20, 2017 10:03

sjrd mentioned this pull request Sep 18, 2020

Scala.js: Implement non-native JS classes. scala/scala3#9774

Merged

Fix #1997: Add support for js.Symbols in @JSName annotations. #2659

Fix #1997: Add support for js.Symbols in @JSName annotations. #2659

Uh oh!

Conversation

gzm0 commented Nov 21, 2016 • edited by sjrd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Old PR description

PrepJSInterop / Definition Site

PrepJSInterop / Call Site

GenJSCode / Definition Site

GenJSCode / Call Site

Caveat

Uh oh!

gzm0 commented Nov 21, 2016

Uh oh!

scala-jenkins commented Nov 21, 2016

Uh oh!

scala-jenkins commented Nov 21, 2016

Uh oh!

sjrd left a comment

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 Nov 21, 2016

Uh oh!

gzm0 commented Nov 21, 2016

Uh oh!

scala-jenkins commented Jan 11, 2017

Uh oh!

scala-jenkins commented Jan 19, 2017

Uh oh!

scala-jenkins commented Feb 6, 2017

Uh oh!

scala-jenkins commented Feb 7, 2017

Uh oh!

scala-jenkins commented Feb 8, 2017

Uh oh!

scala-jenkins commented Feb 8, 2017

Uh oh!

scala-jenkins commented Feb 8, 2017

Uh oh!

scala-jenkins commented Feb 8, 2017

Uh oh!

sjrd commented Feb 8, 2017

Uh oh!

scala-jenkins commented Feb 9, 2017

Uh oh!

scala-jenkins commented Feb 9, 2017

Uh oh!

scala-jenkins commented Feb 10, 2017

Uh oh!

scala-jenkins commented Feb 10, 2017

Uh oh!

sjrd commented Feb 10, 2017

Uh oh!

scala-jenkins commented Feb 10, 2017

Uh oh!

gzm0 commented Feb 10, 2017

Uh oh!

sjrd commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scala-jenkins commented Feb 15, 2017

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzm0 commented Nov 21, 2016 •

edited by sjrd

Loading

sjrd commented Feb 15, 2017 •

edited

Loading