Cleanups after the big Name refactoring #3833

sjrd · 2019-11-05T18:03:21Z

No description provided.

sjrd · 2019-11-05T18:06:06Z

GitHub is partially down at the moment, so the CI is not triggered. I will try to re-close and re-open later.

Since that object defines Name classes and constant Names, it makes more sense to call it `Names`.

gzm0 · 2019-11-06T17:14:02Z

linker/shared/src/main/scala/org/scalajs/linker/checker/IRChecker.scala

@@ -548,18 +548,18 @@ private final class IRChecker(unit: LinkingUnit, logger: Logger) {

      case Assign(select, rhs) =>
        select match {
-          case Select(This(), ClassRef(cls), FieldIdent(_, _))
+          case Select(This(), cls, FieldIdent(_, _))


Seems kind of inconsistent that we are not renaming cls -> className here. Intended?

Not intended, I forgot it. Fixed.

gzm0 · 2019-11-07T08:56:38Z

ir/src/main/scala/org/scalajs/ir/Names.scala


    /** Returns `true` iff this is the name of a static initializer. */
-    def isStaticInitializer: Boolean = this == StaticInitializerSimpleName
+    def isStaticInitializer: Boolean = encoded(0) == '<' && encoded(1) == 'c'


Why this change? Is it actually faster?

It's clearly faster because:

in the true case, testing against StaticInitializerSimpleName requires at least the same comparisons to happen, but loading the rhs from memory instead of as immediates, plus more comparisons for the subsequent characters.

in the false case, we trade looking up this._hashCode and that._hashCode for looking up just this.encoded(0)

Also this avoids defining some methods of SimpleMethodName in terms of constant instances of SimpleMethodName itself.

I've reverted that change in that commit, and instead I've added another commit later that uses an even better mechanism, using reference equality.

gzm0 · 2019-11-07T09:00:00Z

ir/src/main/scala/org/scalajs/ir/Names.scala

        throw new IllegalArgumentException(
-            "A constructor or static initializer must not have a result type")
+            "A reflective proxy must have a result type of java.lang.Object")


It's a bit strange that the check is against a constant but the name is inlined... It's a detail though.

gzm0 · 2019-11-07T10:53:53Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

      }
-      buildInner(scope.withEnv(scope.env.withLocalDefs(newMappings)), cont1)
+      buildInner(scope.withEnv(newEnv), cont1)


I don't understand why the old code wouldn't work here...

withLocalDefs wants a List[(LocalName, LocalDef)], but now newMappings is a List[(Binding.Name, LocalDef)].

gzm0 · 2019-11-07T11:57:35Z

ir/src/main/scala/org/scalajs/ir/Names.scala

+   *    `equals()` and `hashCode()`, along with `==` and `##`, are just as
+   *    broken for `EncodedName` as for `Array`s. Use the methods in the
+   *    companion object instead.
+   */


Why put them in the companion? Is there a disadvantage to simply overriding them? (IIUC for an AnyVal the byte code will look pretty much the same than a method in the companion when not boxed).

Unfortunately, overriding equals and hashCode is prohibited in an AnyVal. The compiler rejects it. I never understood why that restriction exists, but there's nothing I can do about it now.

gzm0 · 2019-11-07T11:57:51Z

ir/src/main/scala/org/scalajs/ir/Names.scala

+      scala.util.hashing.MurmurHash3.bytesHash(encoded.bytes)
+
+    private[Names] def concat(encoded1: EncodedName,
+        encoded2: EncodedName): EncodedName = {


Define + on EncodedName?

I called it ++ instead. + only means concatenation for Strings, and it has the problem of allowing String + Anything without any check. ++ consistently means concatenation for collections in Scala.

gzm0 · 2019-11-07T12:05:18Z

ir/src/main/scala/org/scalajs/ir/Names.scala

+   *    broken for `EncodedName` as for `Array`s. Use the methods in the
+   *    companion object instead.
+   */
+  final class EncodedName private (private[ir] val bytes: Array[Byte])


I like how this gives a neat separation between base encoding and higher level name guarantees. In this line: Have you considered putting this into a separate file (so the utf8 encoding methods would get closer together without increasing noise in this file).

Also, consider simply calling this UTF8String.

gzm0 · 2019-11-07T12:06:06Z

ir/src/main/scala/org/scalajs/ir/Serializers.scala

@@ -106,14 +106,16 @@ object Serializers {
    final val FormatNoPositionValue = -1
  }

-  private final class ByteString(val bytes: Array[Byte]) {
+  private final class EncodedNameKey(val encoded: EncodedName) {


If EncodedName gets the equals and hash code directly, this class can simply disappear, right?

Yes, but as I said above, EncodedName cannot get equals and hashCode :(

It made sense to use `ClassRef`s when they wrapped bare `String`s, as it provided better type safety. However, now that we have type-safe `ClassName`s, the wrapping `ClassRef` are just noise and useless indirections. We also more consistently use `className` as a variable of type `ClassName`, instead of `cls` which was used for `ClassRef`s. After this commit, `ClassRef`s are only used as one case of `TypeRef`, as they should.

sjrd

Updated.

sjrd · 2019-11-07T12:29:21Z

linker/shared/src/main/scala/org/scalajs/linker/checker/IRChecker.scala

@@ -548,18 +548,18 @@ private final class IRChecker(unit: LinkingUnit, logger: Logger) {

      case Assign(select, rhs) =>
        select match {
-          case Select(This(), ClassRef(cls), FieldIdent(_, _))
+          case Select(This(), cls, FieldIdent(_, _))


Not intended, I forgot it. Fixed.

sjrd · 2019-11-07T12:35:46Z

ir/src/main/scala/org/scalajs/ir/Names.scala


    /** Returns `true` iff this is the name of a static initializer. */
-    def isStaticInitializer: Boolean = this == StaticInitializerSimpleName
+    def isStaticInitializer: Boolean = encoded(0) == '<' && encoded(1) == 'c'


It's clearly faster because:

in the true case, testing against StaticInitializerSimpleName requires at least the same comparisons to happen, but loading the rhs from memory instead of as immediates, plus more comparisons for the subsequent characters.

in the false case, we trade looking up this._hashCode and that._hashCode for looking up just this.encoded(0)

sjrd · 2019-11-07T12:37:50Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

      }
-      buildInner(scope.withEnv(scope.env.withLocalDefs(newMappings)), cont1)
+      buildInner(scope.withEnv(newEnv), cont1)


withLocalDefs wants a List[(LocalName, LocalDef)], but now newMappings is a List[(Binding.Name, LocalDef)].

sjrd · 2019-11-07T13:06:56Z

ir/src/main/scala/org/scalajs/ir/Names.scala

+   *    broken for `EncodedName` as for `Array`s. Use the methods in the
+   *    companion object instead.
+   */
+  final class EncodedName private (private[ir] val bytes: Array[Byte])


sjrd · 2019-11-07T13:08:32Z

ir/src/main/scala/org/scalajs/ir/Names.scala

+      scala.util.hashing.MurmurHash3.bytesHash(encoded.bytes)
+
+    private[Names] def concat(encoded1: EncodedName,
+        encoded2: EncodedName): EncodedName = {


I called it ++ instead. + only means concatenation for Strings, and it has the problem of allowing String + Anything without any check. ++ consistently means concatenation for collections in Scala.

sjrd · 2019-11-07T13:22:16Z

ir/src/main/scala/org/scalajs/ir/Names.scala


    /** Returns `true` iff this is the name of a static initializer. */
-    def isStaticInitializer: Boolean = this == StaticInitializerSimpleName
+    def isStaticInitializer: Boolean = encoded(0) == '<' && encoded(1) == 'c'


Also this avoids defining some methods of SimpleMethodName in terms of constant instances of SimpleMethodName itself.

…ame. In doing so, we can turn `resultTypeRef` from being optional to being mandatory. For constructors and static initializers, it must be set to `void`, whereas for reflective proxies, it must be set to `java.lang.Object`. This is consistent with how they are typechecked both at definition site and call site.

Instead of using a fake `LocalName` for `this`, we now use a dedicated ADT for the `name` of a `Binding`, which be either `Binding.This` or `Binding.Local(name, originalName)`. In `OptEnv`, we put the `LocalDef` for `this` in a separate field.

Instead of being a raw `Array[Byte]`, we now use a dedicated class `UTF8String` which is an immutable valid UTF-8 sequence, validated as such when constructed. This allows to expose it to users of `Name`s in a safe way. We also remove or hide all the constructors that by-pass name validation, so that it is not possible to create invalid names from outside the `ir` package.

gzm0

Consider dropping the "the" in the message of the new commit so it's not too long.

Since we need to validate `<init>` and `<clinit>` separately anyway, using the result of this validation to ensure the uniqueness of the resulting `SimpleMethodName`s is free. This allows to implement `isConstructor` and `isStaticInitializer` with `eq`.

After the recent refactorings, we were left with a number of fields and variables named `encodedName`, even though they have the type `ClassName` or `MethodName`. We have now renamed all of them to `className` and `methodName`, respectively.

sjrd · 2019-11-07T15:10:49Z

Consider dropping the "the" in the message of the new commit so it's not too long.

Good point :)

There were several changes to the IR in 1.0.0-RC1 that have an impact on this codebase: * Primitive types now have dedicated `PrimRef`s when used as `TypeRef`s, so for example `IntRef` instead of `ClassRef("I")`. See scala-js/scala-js#3802 * Names now have dedicated types instead of `String`s, and depend on the kind of symbol: `LocalName`, `LabelName`, `FieldName`, `MethodName` and `ClassName`. There are different kinds of `js.Ident`s to go with each kind of `Name`. See scala-js/scala-js#3804, scala-js/scala-js#3814 and scala-js/scala-js#3833 * Original names are now represented by a dedicated `OriginalName` type, and they are stored only at definition site, not at use site. See scala-js/scala-js#3845

sjrd requested a review from gzm0 November 5, 2019 18:03

sjrd closed this Nov 5, 2019

sjrd reopened this Nov 5, 2019

sjrd closed this Nov 5, 2019

sjrd reopened this Nov 5, 2019

Rename ir.Definitions to ir.Names.

f64cd40

Since that object defines Name classes and constant Names, it makes more sense to call it `Names`.

sjrd force-pushed the cleanup-names branch 3 times, most recently from a6fc468 to fa23839 Compare November 7, 2019 10:08

gzm0 requested changes Nov 7, 2019

View reviewed changes

sjrd force-pushed the cleanup-names branch from fa23839 to b3cb1ec Compare November 7, 2019 13:20

sjrd commented Nov 7, 2019

View reviewed changes

sjrd added 3 commits November 7, 2019 14:49

sjrd force-pushed the cleanup-names branch from b3cb1ec to b0fb220 Compare November 7, 2019 14:09

gzm0 reviewed Nov 7, 2019

View reviewed changes

gzm0 approved these changes Nov 7, 2019

View reviewed changes

sjrd added 2 commits November 7, 2019 16:09

sjrd force-pushed the cleanup-names branch from b0fb220 to 9c88c1b Compare November 7, 2019 15:10

sjrd merged commit 3d653a7 into scala-js:master Nov 7, 2019

sjrd deleted the cleanup-names branch November 7, 2019 21:24

sjrd mentioned this pull request Nov 26, 2019

Upgrade to Scala.js 1.0.0-RC1. scala/scala3#7617

Merged

Cleanups after the big Name refactoring #3833

Cleanups after the big Name refactoring #3833

Uh oh!

Conversation

sjrd commented Nov 5, 2019

Uh oh!

sjrd commented Nov 5, 2019

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!

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!

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 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!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

sjrd commented Nov 7, 2019

Uh oh!

Uh oh!