Skip to content

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

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.

Sign up for GitHub

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 7 commits into from
Feb 20, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b55b935
Add a test ensuring we preserve evaluation order in object literals.
sjrd Feb 14, 2017
c3ec9ed
Remove javascript.Tree.PropertyName.name.
gzm0 Jan 5, 2017
c7d7758
Rename ir.PropertyName.name to encodedName.
gzm0 Jan 7, 2017
b752175
Add the API of JavaScript symbols to the library.
gzm0 Nov 21, 2016
e9174fe
Nicer pretty-printing of JS native load specs in the IR printer.
sjrd Feb 17, 2017
360ea48
Fix #1997: Add support for js.Symbols in @JSName annotations.
gzm0 Jan 11, 2017
470f6cf
Use `JSObjectConstr` more often for `js.Dynamic.literal`.
sjrd Feb 14, 2017
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
130 changes: 85 additions & 45 deletions compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ abstract class GenJSCode extends plugins.PluginComponent
import rootMirror._
import definitions._
import jsDefinitions._
import jsInterop.{jsNameOf, compat068FullJSNameOf, jsNativeLoadSpecOf}
import jsInterop.{jsNameOf, compat068FullJSNameOf, jsNativeLoadSpecOf, JSName}
import JSTreeExtractors._

import treeInfo.hasSynthCaseSymbol
Expand Down Expand Up @@ -491,7 +491,7 @@ abstract class GenJSCode extends plugins.PluginComponent

val constructorTrees = new ListBuffer[DefDef]
val generatedMethods = new ListBuffer[js.MethodDef]
val dispatchMethodNames = new ListBuffer[String]
val dispatchMethodNames = new ListBuffer[JSName]

def gen(tree: Tree): Unit = {
tree match {
Expand Down Expand Up @@ -621,15 +621,14 @@ abstract class GenJSCode extends plugins.PluginComponent
assert(mdef.static, "Non-static method in SJS defined JS class")
staticMembers += mdef

case js.StringLiteral(name) =>
case js.StringLiteral("constructor") =>
assert(!mdef.static, "Exported static method")
assert(constructor.isEmpty, "two ctors in class")
constructor = Some(mdef)

if (name == "constructor") {
assert(constructor.isEmpty, "two ctors in class")
constructor = Some(mdef)
} else {
classMembers += mdef
}
case _ =>
assert(!mdef.static, "Exported static method")
classMembers += mdef
}

case property: js.PropertyDef =>
Expand Down Expand Up @@ -667,8 +666,9 @@ abstract class GenJSCode extends plugins.PluginComponent
case fdef: js.FieldDef =>
implicit val pos = fdef.pos
val select = fdef.name match {
case lit: js.StringLiteral => js.JSBracketSelect(selfRef, lit)
case ident: js.Ident => js.JSDotSelect(selfRef, ident)
case ident: js.Ident => js.JSDotSelect(selfRef, ident)
case lit: js.StringLiteral => js.JSBracketSelect(selfRef, lit)
case js.ComputedName(tree, _) => js.JSBracketSelect(selfRef, tree)
}
js.Assign(select, jstpe.zeroOf(fdef.tpe))

Expand Down Expand Up @@ -896,7 +896,7 @@ abstract class GenJSCode extends plugins.PluginComponent
}

val name =
if (isExposed(f)) js.StringLiteral(jsNameOf(f))
if (isExposed(f)) genPropertyName(jsNameOf(f))
else encodeFieldSym(f)

val irTpe = {
Expand Down Expand Up @@ -1100,7 +1100,7 @@ abstract class GenJSCode extends plugins.PluginComponent
else (subConstructors.head.overrideNumBounds._1, overrideNum)

def get(methodName: String): Option[ConstructorTree] = {
if (methodName == this.method.name.name) {
if (methodName == this.method.name.encodedName) {
Some(this)
} else {
subConstructors.iterator.map(_.get(methodName)).collectFirst {
Expand Down Expand Up @@ -1370,7 +1370,7 @@ abstract class GenJSCode extends plugins.PluginComponent

var overrideNum = -1
def mkConstructorTree(method: js.MethodDef): ConstructorTree = {
val methodName = method.name.name
val methodName = method.name.encodedName
val subCtrTrees = ctorToChildren(methodName).map(mkConstructorTree)
overrideNum += 1
new ConstructorTree(overrideNum, method, subCtrTrees)
Expand Down Expand Up @@ -1844,7 +1844,7 @@ abstract class GenJSCode extends plugins.PluginComponent
} else if (isScalaJSDefinedJSClass(sym.owner)) {
val genQual = genExpr(qualifier)
val boxed = if (isExposed(sym))
js.JSBracketSelect(genQual, js.StringLiteral(jsNameOf(sym)))
js.JSBracketSelect(genQual, genExpr(jsNameOf(sym)))
else
js.JSDotSelect(genQual, encodeFieldSym(sym))
unboxFieldValue(boxed)
Expand Down Expand Up @@ -1929,7 +1929,7 @@ abstract class GenJSCode extends plugins.PluginComponent

if (isScalaJSDefinedJSClass(sym.owner)) {
val genLhs = if (isExposed(sym))
js.JSBracketSelect(genQual, js.StringLiteral(jsNameOf(sym)))
js.JSBracketSelect(genQual, genExpr(jsNameOf(sym)))
else
js.JSDotSelect(genQual, encodeFieldSym(sym))
js.Assign(genLhs, genBoxedRhs)
Expand Down Expand Up @@ -3908,9 +3908,10 @@ abstract class GenJSCode extends plugins.PluginComponent
* {name1: arg1, name2: arg2, ... }
*/

def warnIfDuplicatedKey(pairs: List[(js.StringLiteral, js.Tree)]): Unit = {
val allKeys = pairs.collect { case (js.StringLiteral(keyName), _) => keyName }
val keyCounts = allKeys.distinct.map(key => key -> allKeys.count(_ == key))
def warnIfDuplicatedKey(keys: List[js.StringLiteral]): Unit = {
val keyNames = keys.map(_.value)
val keyCounts =
keyNames.distinct.map(key => key -> keyNames.count(_ == key))
val duplicateKeyCounts = keyCounts.filter(1 < _._2)
if (duplicateKeyCounts.nonEmpty) {
reporter.warning(pos,
Expand All @@ -3923,12 +3924,21 @@ abstract class GenJSCode extends plugins.PluginComponent
}
}

def keyToPropName(key: js.Tree, index: Int): js.PropertyName = key match {
case key: js.StringLiteral => key
case _ => js.ComputedName(key, "local" + index)
}

// Extract first arg to future proof against varargs
extractFirstArg(genArgs) match {
// case js.Dynamic.literal("name1" -> ..., "name2" -> ...)
case (js.StringLiteral("apply"), jse.LitNamed(pairs)) =>
warnIfDuplicatedKey(pairs)
js.JSObjectConstr(pairs)
// case js.Dynamic.literal("name1" -> ..., nameExpr2 -> ...)
case (js.StringLiteral("apply"), jse.Tuple2List(pairs)) =>
warnIfDuplicatedKey(pairs.collect {
case (key: js.StringLiteral, _) => key
})
js.JSObjectConstr(pairs.zipWithIndex.map {
case ((key, value), index) => (keyToPropName(key, index), value)
})

/* case js.Dynamic.literal(x: _*)
* Even though scalac does not support this notation, it is still
Expand All @@ -3950,31 +3960,27 @@ abstract class GenJSCode extends plugins.PluginComponent
// case js.Dynamic.literal(x, y)
case (js.StringLiteral("apply"), tups) =>
// Check for duplicated explicit keys
val pairs = jse.LitNamedExtractor.extractFrom(tups)
warnIfDuplicatedKey(pairs)

// Create tmp variable
val resIdent = freshLocalIdent("obj")
val resVarDef = js.VarDef(resIdent, jstpe.AnyType, mutable = false,
js.JSObjectConstr(Nil))
val res = resVarDef.ref
warnIfDuplicatedKey(jse.extractLiteralKeysFrom(tups))

// Assign fields
// Evaluate all tuples first
val tuple2Type = encodeClassType(TupleClass(2))
val assigns = tups flatMap {
// special case for literals
case jse.Tuple2(name, value) =>
js.Assign(js.JSBracketSelect(res, name), value) :: Nil
case tupExpr =>
val tupIdent = freshLocalIdent("tup")
val tup = js.VarRef(tupIdent)(tuple2Type)
js.VarDef(tupIdent, tuple2Type, mutable = false, tupExpr) ::
js.Assign(js.JSBracketSelect(res,
genApplyMethod(tup, js.Ident("$$und1__O"), Nil, jstpe.AnyType)),
genApplyMethod(tup, js.Ident("$$und2__O"), Nil, jstpe.AnyType)) :: Nil
val evalTuples = tups.map { tup =>
js.VarDef(freshLocalIdent("tup"), tuple2Type, mutable = false,
tup)(tup.pos)
}

js.Block(resVarDef +: assigns :+ res: _*)
// Build the resulting object
val result = js.JSObjectConstr(evalTuples.zipWithIndex.map {
case (evalTuple, index) =>
val tupRef = evalTuple.ref
val key = genApplyMethod(tupRef, js.Ident("$$und1__O"), Nil,
jstpe.AnyType)
val value = genApplyMethod(tupRef, js.Ident("$$und2__O"), Nil,
jstpe.AnyType)
keyToPropName(key, index) -> value
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

The optimizer will take care of that, if necessary.


js.Block(evalTuples :+ result)

// case where another method is called
case (js.StringLiteral(name), _) if name != "apply" =>
Expand Down Expand Up @@ -4199,7 +4205,7 @@ abstract class GenJSCode extends plugins.PluginComponent
js.JSFunctionApply(receiver, args)

case _ =>
def jsFunName = js.StringLiteral(jsNameOf(sym))
def jsFunName: js.Tree = genExpr(jsNameOf(sym))

def genSuperReference(propName: js.Tree): js.Tree = {
superIn.fold[js.Tree] {
Expand Down Expand Up @@ -5144,6 +5150,40 @@ abstract class GenJSCode extends plugins.PluginComponent
(patchedParams, patchedBody)
}

// Methods to deal with JSName ---------------------------------------------

def genPropertyName(name: JSName)(implicit pos: Position): js.PropertyName = {
name match {
case JSName.Literal(name) => js.StringLiteral(name)

case JSName.Computed(sym) =>
js.ComputedName(genComputedJSName(sym), encodeComputedNameIdentity(sym))
}
}

def genExpr(name: JSName)(implicit pos: Position): js.Tree = name match {
case JSName.Literal(name) => js.StringLiteral(name)
case JSName.Computed(sym) => genComputedJSName(sym)
}

private def genComputedJSName(sym: Symbol)(implicit pos: Position): js.Tree = {
/* By construction (i.e. restriction in PrepJSInterop), we know that sym
* must be a static method.
* Therefore, at this point, we can invoke it by loading its owner and
* calling it.
*/
val module = genLoadModule(sym.owner)

if (isRawJSType(sym.owner.tpe)) {
if (!isScalaJSDefinedJSClass(sym.owner) || isExposed(sym))
genJSCallGeneric(sym, module, args = Nil, isStat = false)
else
genApplyJSClassMethod(module, sym, arguments = Nil)
} else {
genApplyMethod(module, sym, arguments = Nil)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
}

Copy link
Member

Choose a reason for hiding this comment

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

We probably should also have this nasty test case here:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

#2778

}

// Utilities ---------------------------------------------------------------

/** Generate a literal "zero" for the requested type */
Expand Down
42 changes: 24 additions & 18 deletions compiler/src/main/scala/org/scalajs/core/compiler/GenJSExports.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
import jsAddons._
import definitions._
import jsDefinitions._
import jsInterop.jsNameOf
import jsInterop.{jsNameOf, JSName}

trait JSExportsPhase { this: JSCodePhase =>

Expand All @@ -54,7 +54,7 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
}

def genJSClassDispatchers(classSym: Symbol,
dispatchMethodsNames: List[String]): List[js.Tree] = {
dispatchMethodsNames: List[JSName]): List[js.Tree] = {
dispatchMethodsNames
.map(genJSClassDispatcher(classSym, _))
}
Expand Down Expand Up @@ -91,7 +91,8 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
implicit val pos = ctors.head.pos

val js.MethodDef(_, _, args, _, body) =
withNewLocalNameScope(genExportMethod(ctors, jsName, static = false))
withNewLocalNameScope(genExportMethod(ctors, JSName.Literal(jsName),
static = false))

js.ConstructorExportDef(jsName, args, body.get)
}
Expand Down Expand Up @@ -225,8 +226,8 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>

// Generate the export

val exportedMember = genMemberExportOrDispatcher(classSym, jsName,
isProp, alts, static = true)
val exportedMember = genMemberExportOrDispatcher(classSym,
JSName.Literal(jsName), isProp, alts, static = true)

val exportDef =
if (destination == ExportDestination.Static) exportedMember
Expand Down Expand Up @@ -317,11 +318,11 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
s"Exported $kind $jsName conflicts with ${alts.head.fullName}")
}

genMemberExportOrDispatcher(classSym, jsName, isProp, alts,
static = false)
genMemberExportOrDispatcher(classSym, JSName.Literal(jsName), isProp,
alts, static = false)
}

private def genJSClassDispatcher(classSym: Symbol, name: String): js.Tree = {
private def genJSClassDispatcher(classSym: Symbol, name: JSName): js.Tree = {
var alts: List[Symbol] = Nil
for {
sym <- classSym.info.members
Expand All @@ -348,7 +349,7 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
}
}

def genMemberExportOrDispatcher(classSym: Symbol, jsName: String,
def genMemberExportOrDispatcher(classSym: Symbol, jsName: JSName,
isProp: Boolean, alts: List[Symbol], static: Boolean): js.Tree = {
withNewLocalNameScope {
if (isProp)
Expand All @@ -358,10 +359,12 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
}
}

def genJSConstructorExport(alts: List[Symbol]): js.MethodDef =
genExportMethod(alts.map(ExportedSymbol), "constructor", static = false)
def genJSConstructorExport(alts: List[Symbol]): js.MethodDef = {
genExportMethod(alts.map(ExportedSymbol), JSName.Literal("constructor"),
static = false)
}

private def genExportProperty(alts: List[Symbol], jsName: String,
private def genExportProperty(alts: List[Symbol], jsName: JSName,
static: Boolean): js.PropertyDef = {
assert(!alts.isEmpty)
implicit val pos = alts.head.pos
Expand All @@ -379,7 +382,7 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
s"Found more than one instance getter to export for name $jsName.")
for (duplicate <- getter.tail) {
reporter.error(duplicate.pos,
s"Duplicate static getter export with name '$jsName'")
s"Duplicate static getter export with name '${jsName.displayName}'")
}
}

Expand All @@ -398,13 +401,13 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
}
}

js.PropertyDef(static, js.StringLiteral(jsName), getterBody,
js.PropertyDef(static, genPropertyName(jsName), getterBody,
setterArgAndBody)
}

/** generates the exporter function (i.e. exporter for non-properties) for
* a given name */
private def genExportMethod(alts0: List[Exported], jsName: String,
private def genExportMethod(alts0: List[Exported], jsName: JSName,
static: Boolean): js.MethodDef = {
assert(alts0.nonEmpty,
"need at least one alternative to generate exporter method")
Expand All @@ -414,7 +417,10 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
val alts = {
// toString() is always exported. We might need to add it here
// to get correct overloading.
if (jsName == "toString" && alts0.forall(_.params.nonEmpty))
val needsToString =
jsName == JSName.Literal("toString") && alts0.forall(_.params.nonEmpty)

if (needsToString)
ExportedSymbol(Object_toString) :: alts0
else
alts0
Expand Down Expand Up @@ -526,7 +532,7 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>
}
}

js.MethodDef(static, js.StringLiteral(jsName),
js.MethodDef(static, genPropertyName(jsName),
formalArgs, jstpe.AnyType, Some(body))(OptimizerHints.empty, None)
}

Expand Down Expand Up @@ -698,7 +704,7 @@ trait GenJSExports extends SubComponent { self: GenJSCode =>

val cls = jstpe.ClassType(encodeClassFullName(currentClassSym))
val receiver = js.This()(jstpe.AnyType)
val nameString = js.StringLiteral(jsNameOf(sym))
val nameString = genExpr(jsNameOf(sym))

if (jsInterop.isJSGetter(sym)) {
assert(allArgs.isEmpty)
Expand Down
5 changes: 5 additions & 0 deletions compiler/src/main/scala/org/scalajs/core/compiler/JSEncoding.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,11 @@ trait JSEncoding extends SubComponent { self: GenJSCode =>
def needsModuleClassSuffix(sym: Symbol): Boolean =
sym.isModuleClass && !foreignIsImplClass(sym)

def encodeComputedNameIdentity(sym: Symbol): String = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

PropertyName#encodedName seems a reasonable place for that.

assert(sym.owner.isModuleClass)
encodeClassFullName(sym.owner) + "__" + encodeMemberNameInternal(sym)
}

private def encodeMemberNameInternal(sym: Symbol): String =
sym.name.toString.replace("_", "$und")

Expand Down
Loading