Skip to content

Commit 05308c5

Browse files
sjrdsjrd
committed
Address more review comments.
1 parent a319876 commit 05308c5

File tree

3 files changed

+110
-78
lines changed

3 files changed

+110
-78
lines changed

‎compiler/src/dotty/tools/dotc/transform/sjs/PrepJSInterop.scala

Lines changed: 25 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
167167
else if (enclosingOwner is OwnerKind.JSType)
168168
transformValOrDefDefInJSType(tree)
169169
else
170-
transformScalaValOrDefDef(tree)
170+
super.transform(tree) // There is nothing special to do for a Scala val or def
171171
}
172172
}
173173

@@ -198,11 +198,6 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
198198
}
199199
}
200200

201-
private def transformScalaValOrDefDef(tree: ValOrDefDef)(using Context): Tree = {
202-
// There is nothing special to do for a Scala val or def
203-
super.transform(tree)
204-
}
205-
206201
private def transformTemplate(tree: Template)(using Context): Template = {
207202
// First, recursively transform the template
208203
val transformedTree = super.transform(tree).asInstanceOf[Template]
@@ -245,23 +240,6 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
245240

246241
private def transformStatOrExpr(tree: Tree)(using Context): Tree = {
247242
tree match {
248-
/* This might not be needed in dotty.
249-
/* Anonymous function, need to check that it is not used as a SAM for a
250-
* JS type, unless it is js.FunctionN or js.ThisFunctionN.
251-
* See #2921.
252-
*/
253-
case tree: Function =>
254-
val tpeSym = tree.tpe.typeSymbol
255-
if (isJSAny(tpeSym) && !AllJSFunctionClasses.contains(tpeSym)) {
256-
report.error(
257-
"Using an anonymous function as a SAM for the JavaScript " +
258-
"type " + tpeSym.fullNameString + " is not allowed. " +
259-
"Use an anonymous class instead.",
260-
tree)
261-
}
262-
super.transform(tree)
263-
*/
264-
265243
// Validate js.constructorOf[T]
266244
case TypeApply(ctorOfTree, List(tpeArg))
267245
if ctorOfTree.symbol == jsdefn.JSPackage_constructorOf =>
@@ -310,50 +288,13 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
310288
}
311289
}
312290

313-
/** Performs checks and rewrites specific to classes / objects extending
314-
* js.Any.
315-
*/
291+
/** Performs checks and rewrites specific to classes / objects extending `js.Any`. */
316292
private def transformJSClassDef(classDef: TypeDef)(using Context): Tree = {
317293
val sym = classDef.symbol
294+
val isJSNative = sym.hasAnnotation(jsdefn.JSNativeAnnot)
318295

319296
sym.addAnnotation(jsdefn.JSTypeAnnot)
320297

321-
/*val isJSLambda =
322-
sym.isAnonymousClass && AllJSFunctionClasses.exists(sym.isSubClass(_))
323-
if (isJSLambda)
324-
transformJSLambdaClassDef(classDef)
325-
else*/
326-
transformNonLambdaJSClassDef(classDef)
327-
}
328-
329-
/*
330-
/** Performs checks and rewrites specific to JS lambdas, i.e., anonymous
331-
* classes extending one of the JS function types.
332-
*
333-
* JS lambdas are special because they are completely hijacked by the
334-
* back-end, so although at this phase they look like normal anonymous
335-
* classes, they do not behave like ones.
336-
*/
337-
private def transformJSLambdaClassDef(classDef: TypeDef)(using Context): Tree = {
338-
/* For the purposes of checking inner members, a JS lambda acts as a JS
339-
* native class owner.
340-
*
341-
* TODO This is probably not right, but historically it has always been
342-
* that way. It should be revisited.
343-
*/
344-
enterOwner(OwnerKind.JSNativeClass) {
345-
super.transform(classDef)
346-
}
347-
}
348-
*/
349-
350-
/** Performs checks and rewrites for all JS classes, traits and objects
351-
* except JS lambdas.
352-
*/
353-
private def transformNonLambdaJSClassDef(classDef: TypeDef)(using Context): Tree = {
354-
val sym = classDef.symbol
355-
val isJSNative = sym.hasAnnotation(jsdefn.JSNativeAnnot)
356-
357298
// Forbid @EnableReflectiveInstantiation on JS types
358299
sym.getAnnotation(jsdefn.EnableReflectiveInstantiationAnnot).foreach { annot =>
359300
report.error(
@@ -393,7 +334,7 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
393334
case parentSym if parentSym == defn.DynamicClass =>
394335
/* We have to allow scala.Dynamic to be able to define js.Dynamic
395336
* and similar constructs.
396-
* This causes the unsoundness filed as #1385.
337+
* This causes the unsoundness filed as scala-js/scala-js#1385.
397338
*/
398339
case parentSym =>
399340
/* This is a Scala class or trait other than AnyRef and Dynamic,
@@ -466,7 +407,7 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
466407
errorPos)
467408
}
468409

469-
// Check for overrides with different JS names - issue #1983
410+
// Check for overrides with different JS names - issue scala-js/scala-js#1983
470411
if (overriding.jsName != overridden.jsName)
471412
emitOverrideError("has a different JS name")
472413

@@ -562,20 +503,19 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
562503
case JSName.Literal(jsName) =>
563504
checkGlobalRefName(jsName)
564505
case JSName.Computed(_) =>
565-
() // compile error above
506+
() // compile error above or in `checkJSNameArgument`
566507
}
567508
}
568509
}
569510
} else {
570-
def firstElementOfPath(pathName: String): String = {
511+
def checkGlobalRefPath(pathName: String): Unit = {
571512
val dotIndex = pathName.indexOf('.')
572-
if (dotIndex < 0) pathName
573-
else pathName.substring(0, dotIndex)
513+
val globalRef =
514+
if (dotIndex < 0) pathName
515+
else pathName.substring(0, dotIndex)
516+
checkGlobalRefName(globalRef)
574517
}
575518

576-
def checkGlobalRefPath(pathName: String): Unit =
577-
checkGlobalRefName(firstElementOfPath(pathName))
578-
579519
checkAndGetJSNativeLoadingSpecAnnotOf(pos, sym) match {
580520
case Some(annot) if annot.symbol == jsdefn.JSGlobalScopeAnnot =>
581521
if (!sym.is(Module)) {
@@ -657,15 +597,17 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
657597
}
658598

659599
case nme.equals_ if sym.info.matches(defn.Any_equals.info) =>
660-
report.warning(
661-
"Overriding equals in a JS class does not change how it is compared. " +
662-
"To silence this warning, change the name of the method and optionally add @JSName(\"equals\").",
600+
report.error(
601+
"error overriding method equals(that: Any): Boolean in a JS class;\n" +
602+
" method equals(that: Any): Boolean is considered final in trait js.Any;\n" +
603+
" if you want to define a method named \"equals\" in JavaScript, use a different name and add @JSName(\"equals\").",
663604
sym)
664605

665606
case nme.hashCode_ if sym.info.matches(defn.Any_hashCode.info) =>
666-
report.warning(
667-
"Overriding hashCode in a JS class does not change its hash code. " +
668-
"To silence this warning, change the name of the method and optionally add @JSName(\"hashCode\").",
607+
report.error(
608+
"error overriding method hashCode(): Int in a JS class;\n" +
609+
" method hashCode(): Int is considered final in trait js.Any;\n" +
610+
" if you want to define a method named \"hashCode\" in JavaScript, use a different name and add @JSName(\"hashCode\").",
669611
sym)
670612

671613
case _ =>
@@ -907,7 +849,7 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
907849
// it is not private
908850
!isPrivateMaybeWithin(sym) &&
909851
// it is not a constructor
910-
!sym.isConstructor //&& !sym.isValueParameter && !sym.isParamWithDefault
852+
!sym.isConstructor
911853
}
912854

913855
if (shouldBeExposed)
@@ -971,6 +913,11 @@ object PrepJSInterop {
971913
val AnyClass = ScalaClass | JSNativeClass | JSClass
972914
}
973915

916+
/** Tests if the symbol extend `js.Any`.
917+
*
918+
* This is different from `sym.isJSType` because it returns `false` for the
919+
* pseudo-union type.
920+
*/
974921
def isJSAny(sym: Symbol)(using Context): Boolean =
975922
sym.isSubClass(jsdefn.JSAnyClass)
976923

‎tests/neg-scalajs/js-type-override-equals-hashcode.check

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:5:15 ------------------------------------------------
2+
5 | override def hashCode(): Int = 1 // error
3+
| ^
4+
|error overriding method hashCode(): Int in a JS class;
5+
| method hashCode(): Int is considered final in trait js.Any;
6+
| if you want to define a method named "hashCode" in JavaScript, use a different name and add @JSName("hashCode").
7+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:6:15 ------------------------------------------------
8+
6 | override def equals(obj: Any): Boolean = false // error
9+
| ^
10+
| error overriding method equals(that: Any): Boolean in a JS class;
11+
| method equals(that: Any): Boolean is considered final in trait js.Any;
12+
| if you want to define a method named "equals" in JavaScript, use a different name and add @JSName("equals").
13+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:24:15 -----------------------------------------------
14+
24 | override def hashCode(): Int = js.native // error
15+
| ^
16+
|error overriding method hashCode(): Int in a JS class;
17+
| method hashCode(): Int is considered final in trait js.Any;
18+
| if you want to define a method named "hashCode" in JavaScript, use a different name and add @JSName("hashCode").
19+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:25:15 -----------------------------------------------
20+
25 | override def equals(obj: Any): Boolean = js.native // error
21+
| ^
22+
| error overriding method equals(that: Any): Boolean in a JS class;
23+
| method equals(that: Any): Boolean is considered final in trait js.Any;
24+
| if you want to define a method named "equals" in JavaScript, use a different name and add @JSName("equals").
25+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:30:15 -----------------------------------------------
26+
30 | override def hashCode(): Int = js.native // error
27+
| ^
28+
|error overriding method hashCode(): Int in a JS class;
29+
| method hashCode(): Int is considered final in trait js.Any;
30+
| if you want to define a method named "hashCode" in JavaScript, use a different name and add @JSName("hashCode").
31+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:31:15 -----------------------------------------------
32+
31 | override def equals(obj: Any): Boolean = js.native // error
33+
| ^
34+
| error overriding method equals(that: Any): Boolean in a JS class;
35+
| method equals(that: Any): Boolean is considered final in trait js.Any;
36+
| if you want to define a method named "equals" in JavaScript, use a different name and add @JSName("equals").
37+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:35:15 -----------------------------------------------
38+
35 | override def hashCode(): Int // error
39+
| ^
40+
|error overriding method hashCode(): Int in a JS class;
41+
| method hashCode(): Int is considered final in trait js.Any;
42+
| if you want to define a method named "hashCode" in JavaScript, use a different name and add @JSName("hashCode").
43+
-- Error: tests/neg-scalajs/js-type-override-equals-hashcode.scala:36:15 -----------------------------------------------
44+
36 | override def equals(obj: Any): Boolean // error
45+
| ^
46+
| error overriding method equals(that: Any): Boolean in a JS class;
47+
| method equals(that: Any): Boolean is considered final in trait js.Any;
48+
| if you want to define a method named "equals" in JavaScript, use a different name and add @JSName("equals").

‎tests/neg-scalajs/js-type-override-equals-hashcode.scala

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import scala.scalajs.js
2+
import scala.scalajs.js.annotation._
3+
4+
class A extends js.Object {
5+
override def hashCode(): Int = 1 // error
6+
override def equals(obj: Any): Boolean = false // error
7+
8+
// this one works as expected (so allowed)
9+
override def toString(): String = "frobber"
10+
11+
/* these are allowed, since they are protected in jl.Object.
12+
* as a result, only the overrides can be called. So the fact that they
13+
* do not truly override the methods in jl.Object is not observable.
14+
*/
15+
override def clone(): Object = null
16+
override def finalize(): Unit = ()
17+
18+
// other methods in jl.Object are final.
19+
}
20+
21+
@js.native
22+
@JSGlobal
23+
object B extends js.Object {
24+
override def hashCode(): Int = js.native // error
25+
override def equals(obj: Any): Boolean = js.native // error
26+
}
27+
28+
@js.native
29+
trait C extends js.Any {
30+
override def hashCode(): Int = js.native // error
31+
override def equals(obj: Any): Boolean = js.native // error
32+
}
33+
34+
trait D extends js.Any {
35+
override def hashCode(): Int // error
36+
override def equals(obj: Any): Boolean // error
37+
}

0 commit comments

Comments
 (0)