Skip to content

Opt: Remove useless *Ident indirections in the IR model. #5092

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 3 commits into from
Dec 22, 2024
Merged

Opt: Remove useless *Ident indirections in the IR model. #5092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d65f79
Improve test suite error messages when `HashersTest` fails.
sjrd Dec 21, 2024
5dc4127
Directly store a LocalName inside VarRef, instead of a LocalIdent.
sjrd Dec 21, 2024
2ba9ea7
Remove `LabelIdent`. Directly store `LabelName` instead.
sjrd Dec 21, 2024
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
92 changes: 46 additions & 46 deletions compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala
View file
Open in desktop

Large diffs are not rendered by default.

10 changes: 5 additions & 5 deletions compiler/src/main/scala/org/scalajs/nscplugin/GenJSExports.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ trait GenJSExports[G <: Global with Singleton] extends SubComponent {
val superClass = {
val superClassSym = currentClassSym.superClass
if (isNestedJSClass(superClassSym)) {
js.VarRef(js.LocalIdent(JSSuperClassParamName))(jstpe.AnyType)
js.VarRef(JSSuperClassParamName)(jstpe.AnyType)
} else {
js.LoadJSConstructor(encodeClassName(superClassSym))
}
Expand Down Expand Up @@ -1053,7 +1053,7 @@ trait GenJSExports[G <: Global with Singleton] extends SubComponent {

def genArgRef(index: Int)(implicit pos: Position): js.Tree = {
if (index < minArgc)
js.VarRef(js.LocalIdent(fixedParamNames(index)))(jstpe.AnyType)
js.VarRef(fixedParamNames(index))(jstpe.AnyType)
else
js.JSSelect(genRestArgRef(), js.IntLiteral(index - minArgc))
}
Expand All @@ -1073,16 +1073,16 @@ trait GenJSExports[G <: Global with Singleton] extends SubComponent {
def genRestArgRef()(implicit pos: Position): js.Tree = {
assert(needsRestParam,
s"trying to generate a reference to non-existent rest param at $pos")
js.VarRef(js.LocalIdent(restParamName))(jstpe.AnyType)
js.VarRef(restParamName)(jstpe.AnyType)
}

def genAllArgsRefsForForwarder()(implicit pos: Position): List[js.TreeOrJSSpread] = {
val fixedArgRefs = fixedParamNames.toList.map { paramName =>
js.VarRef(js.LocalIdent(paramName))(jstpe.AnyType)
js.VarRef(paramName)(jstpe.AnyType)
}

if (needsRestParam) {
val restArgRef = js.VarRef(js.LocalIdent(restParamName))(jstpe.AnyType)
val restArgRef = js.VarRef(restParamName)(jstpe.AnyType)
fixedArgRefs :+ js.JSSpread(restArgRef)
} else {
fixedArgRefs
Expand Down
22 changes: 11 additions & 11 deletions compiler/src/main/scala/org/scalajs/nscplugin/JSEncoding.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ trait JSEncoding[G <: Global with Singleton] extends SubComponent {
case None =>
inner
case Some(labelName) =>
js.Labeled(js.LabelIdent(labelName), tpe, inner)
js.Labeled(labelName, tpe, inner)
}
}
}
Expand Down Expand Up @@ -151,29 +151,26 @@ trait JSEncoding[G <: Global with Singleton] extends SubComponent {
private def freshLabelName(base: LabelName): LabelName =
freshNameGeneric(base, usedLabelNames)(_.withSuffix(_))

private def freshLabelName(base: String): LabelName =
def freshLabelName(base: String): LabelName =
freshLabelName(LabelName(base))

def freshLabelIdent(base: String)(implicit pos: ir.Position): js.LabelIdent =
js.LabelIdent(freshLabelName(base))

private def labelSymbolName(sym: Symbol): LabelName =
labelSymbolNames.getOrElseUpdate(sym, freshLabelName(sym.name.toString))

def getEnclosingReturnLabel()(implicit pos: ir.Position): js.LabelIdent = {
def getEnclosingReturnLabel()(implicit pos: Position): LabelName = {
val box = returnLabelName.get
if (box == null)
throw new IllegalStateException(s"No enclosing returnable scope at $pos")
if (box.value.isEmpty)
box.value = Some(freshLabelName("_return"))
js.LabelIdent(box.value.get)
box.value.get
}

// Encoding methods ----------------------------------------------------------

def encodeLabelSym(sym: Symbol)(implicit pos: Position): js.LabelIdent = {
def encodeLabelSym(sym: Symbol): LabelName = {
require(sym.isLabel, "encodeLabelSym called with non-label symbol: " + sym)
js.LabelIdent(labelSymbolName(sym))
labelSymbolName(sym)
}

def encodeFieldSym(sym: Symbol)(implicit pos: Position): js.FieldIdent = {
Expand Down Expand Up @@ -256,7 +253,10 @@ trait JSEncoding[G <: Global with Singleton] extends SubComponent {
}
}

def encodeLocalSym(sym: Symbol)(implicit pos: Position): js.LocalIdent = {
def encodeLocalSym(sym: Symbol)(implicit pos: Position): js.LocalIdent =
js.LocalIdent(encodeLocalSymName(sym))

def encodeLocalSymName(sym: Symbol): LocalName = {
/* The isValueParameter case is necessary to work around an internal bug
* of scalac: for some @varargs methods, the owner of some parameters is
* the enclosing class rather the method, so !sym.owner.isClass fails.
Expand All @@ -266,7 +266,7 @@ trait JSEncoding[G <: Global with Singleton] extends SubComponent {
require(sym.isValueParameter ||
(!sym.owner.isClass && sym.isTerm && !sym.isMethod && !sym.isModule),
"encodeLocalSym called with non-local symbol: " + sym)
js.LocalIdent(localSymbolName(sym))
localSymbolName(sym)
}

def encodeClassType(sym: Symbol): jstpe.Type = {
Expand Down
4 changes: 2 additions & 2 deletions compiler/src/test/scala/org/scalajs/nscplugin/test/OptimizationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ class OptimizationTest extends JSASTTest {
}
}
""".hasNot("non-return labeled block") {
case js.Labeled(name, _, _) if !name.name.nameString.startsWith("_return") =>
case js.Labeled(name, _, _) if !name.nameString.startsWith("_return") =>
}
}

Expand Down Expand Up @@ -323,7 +323,7 @@ class OptimizationTest extends JSASTTest {
}
}
""".hasNot("non-return labeled block") {
case js.Labeled(name, _, _) if !name.name.nameString.startsWith("_return") =>
case js.Labeled(name, _, _) if !name.nameString.startsWith("_return") =>
}
}

Expand Down
13 changes: 4 additions & 9 deletions ir/shared/src/main/scala/org/scalajs/ir/Hashers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ object Hashers {

case Labeled(label, tpe, body) =>
mixTag(TagLabeled)
mixLabelIdent(label)
mixName(label)
mixType(tpe)
mixTree(body)

Expand All @@ -197,7 +197,7 @@ object Hashers {
case Return(expr, label) =>
mixTag(TagReturn)
mixTree(expr)
mixLabelIdent(label)
mixName(label)

case If(cond, thenp, elsep) =>
mixTag(TagIf)
Expand Down Expand Up @@ -524,9 +524,9 @@ object Hashers {
mixTag(TagClassOf)
mixTypeRef(typeRef)

case VarRef(ident) =>
case VarRef(name) =>
mixTag(TagVarRef)
mixLocalIdent(ident)
mixName(name)
mixType(tree.tpe)

case This() =>
Expand Down Expand Up @@ -646,11 +646,6 @@ object Hashers {
mixName(ident.name)
}

def mixLabelIdent(ident: LabelIdent): Unit = {
mixPos(ident.pos)
mixName(ident.name)
}

def mixSimpleFieldIdent(ident: SimpleFieldIdent): Unit = {
mixPos(ident.pos)
mixName(ident.name)
Expand Down
8 changes: 2 additions & 6 deletions ir/shared/src/main/scala/org/scalajs/ir/Printers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ object Printers {
def printAnyNode(node: IRNode): Unit = {
node match {
case node: LocalIdent => print(node)
case node: LabelIdent => print(node)
case node: SimpleFieldIdent => print(node)
case node: FieldIdent => print(node)
case node: MethodIdent => print(node)
Expand Down Expand Up @@ -869,8 +868,8 @@ object Printers {

// Atomic expressions

case VarRef(ident) =>
print(ident)
case VarRef(name) =>
print(name)

case This() =>
print("this")
Expand Down Expand Up @@ -1138,9 +1137,6 @@ object Printers {
def print(ident: LocalIdent): Unit =
print(ident.name)

def print(ident: LabelIdent): Unit =
print(ident.name)

def print(ident: SimpleFieldIdent): Unit =
print(ident.name)

Expand Down
33 changes: 16 additions & 17 deletions ir/shared/src/main/scala/org/scalajs/ir/Serializers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,15 +265,15 @@ object Serializers {

case Labeled(label, tpe, body) =>
writeTagAndPos(TagLabeled)
writeLabelIdent(label); writeType(tpe); writeTree(body)
writeName(label); writeType(tpe); writeTree(body)

case Assign(lhs, rhs) =>
writeTagAndPos(TagAssign)
writeTree(lhs); writeTree(rhs)

case Return(expr, label) =>
writeTagAndPos(TagReturn)
writeTree(expr); writeLabelIdent(label)
writeTree(expr); writeName(label)

case If(cond, thenp, elsep) =>
writeTagAndPos(TagIf)
Expand Down Expand Up @@ -551,9 +551,9 @@ object Serializers {
writeTagAndPos(TagClassOf)
writeTypeRef(typeRef)

case VarRef(ident) =>
case VarRef(name) =>
writeTagAndPos(TagVarRef)
writeLocalIdent(ident)
writeName(name)
writeType(tree.tpe)

case This() =>
Expand Down Expand Up @@ -786,11 +786,6 @@ object Serializers {
writeName(ident.name)
}

def writeLabelIdent(ident: LabelIdent): Unit = {
writePosition(ident.pos)
writeName(ident.name)
}

def writeSimpleFieldIdent(ident: SimpleFieldIdent): Unit = {
writePosition(ident.pos)
writeName(ident.name)
Expand Down Expand Up @@ -1135,7 +1130,7 @@ object Serializers {
case TagVarDef => VarDef(readLocalIdent(), readOriginalName(), readType(), readBoolean(), readTree())
case TagSkip => Skip()
case TagBlock => Block(readTrees())
case TagLabeled => Labeled(readLabelIdent(), readType(), readTree())
case TagLabeled => Labeled(readLabelName(), readType(), readTree())

case TagAssign =>
val lhs0 = readTree()
Expand All @@ -1155,7 +1150,7 @@ object Serializers {

Assign(lhs.asInstanceOf[AssignLhs], rhs)

case TagReturn => Return(readTree(), readLabelIdent())
case TagReturn => Return(readTree(), readLabelName())
case TagIf => If(readTree(), readTree(), readTree())(readType())
case TagWhile => While(readTree(), readTree())

Expand Down Expand Up @@ -1388,7 +1383,10 @@ object Serializers {
case TagClassOf => ClassOf(readTypeRef())

case TagVarRef =>
VarRef(readLocalIdent())(readType())
val name =
if (hacks.use17) readLocalIdent().name
else readLocalName()
VarRef(name)(readType())

case TagThis =>
val tpe = readType()
Expand Down Expand Up @@ -2163,11 +2161,6 @@ object Serializers {
LocalIdent(readLocalName())
}

def readLabelIdent(): LabelIdent = {
implicit val pos = readPosition()
LabelIdent(readLabelName())
}

def readFieldIdent(): FieldIdent = {
// For historical reasons, the className comes *before* the position
val className = readClassName()
Expand Down Expand Up @@ -2396,6 +2389,12 @@ object Serializers {
}

private def readLabelName(): LabelName = {
/* Before 1.18, `LabelName`s were always wrapped in `LabelIdent`s, whose
* encoding was a `Position` followed by the actual `LabelName`.
*/
if (hacks.use17)
readPosition() // intentional discard

val i = readInt()
val existing = labelNames(i)
if (existing ne null) {
Expand Down
13 changes: 5 additions & 8 deletions ir/shared/src/main/scala/org/scalajs/ir/Trees.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ object Trees {
sealed case class LocalIdent(name: LocalName)(implicit val pos: Position)
extends IRNode

sealed case class LabelIdent(name: LabelName)(implicit val pos: Position)
extends IRNode

sealed case class SimpleFieldIdent(name: SimpleFieldName)(implicit val pos: Position)
extends IRNode

Expand Down Expand Up @@ -104,13 +101,13 @@ object Trees {
implicit val pos: Position) extends Tree {
val tpe = VoidType

def ref(implicit pos: Position): VarRef = VarRef(name)(vtpe)
def ref(implicit pos: Position): VarRef = VarRef(name.name)(vtpe)
}

sealed case class ParamDef(name: LocalIdent, originalName: OriginalName,
ptpe: Type, mutable: Boolean)(
implicit val pos: Position) extends IRNode {
def ref(implicit pos: Position): VarRef = VarRef(name)(ptpe)
def ref(implicit pos: Position): VarRef = VarRef(name.name)(ptpe)
}

// Control flow constructs
Expand Down Expand Up @@ -150,7 +147,7 @@ object Trees {
def unapply(block: Block): Some[List[Tree]] = Some(block.stats)
}

sealed case class Labeled(label: LabelIdent, tpe: Type, body: Tree)(
sealed case class Labeled(label: LabelName, tpe: Type, body: Tree)(
implicit val pos: Position) extends Tree

sealed trait AssignLhs extends Tree
Expand All @@ -160,7 +157,7 @@ object Trees {
val tpe = VoidType
}

sealed case class Return(expr: Tree, label: LabelIdent)(
sealed case class Return(expr: Tree, label: LabelName)(
implicit val pos: Position) extends Tree {
val tpe = NothingType
}
Expand Down Expand Up @@ -1088,7 +1085,7 @@ object Trees {

// Atomic expressions

sealed case class VarRef(ident: LocalIdent)(val tpe: Type)(
sealed case class VarRef(name: LocalName)(val tpe: Type)(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I have briefly considered whether VarRef should extend LocalIdent (or even whether we should remove LocalIdent altogether and use VarRef instead.

I do not think we should do this:

  • VarDef / ParamDef could lose their vtpe / ptpe field, but it feels unnatural to have the types in the name.
  • Other trees ForIn currently do not require explicit types, so we'd force an unnecessary AnyType there (which we'd have to IR Check).

implicit val pos: Position) extends AssignLhs

sealed case class This()(val tpe: Type)(implicit val pos: Position)
Expand Down
10 changes: 4 additions & 6 deletions ir/shared/src/test/scala/org/scalajs/ir/HashersTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,9 @@ class HashersTest {
out.close()
out.toByteArray()
}
val actualString = actualBytes.map(b => "%02x".format(b & 0xff)).mkString

val expectedBytes = expected.grouped(2)
.map(Integer.parseInt(_, 16).toByte).toArray

assertArrayEquals(expectedBytes, actualBytes)
assertEquals(expected, actualString)
}

private val bodyWithInterestingStuff = Block(
Expand Down Expand Up @@ -93,7 +91,7 @@ class HashersTest {
)

test(
"309805e5680ffa1804811ff5c9ebc77e91846957",
"82df9d6beb7df0ee9f501380323bdb2038cc50cb",
MethodDef(MemberFlags.empty, mIIMethodName, NON,
List(ParamDef("x", NON, IntType, mutable = false)),
IntType, Some(bodyWithInterestingStuff))(
Expand All @@ -108,7 +106,7 @@ class HashersTest {
}

test(
"c0f1ef1b22fd1cfdc9bba78bf3e0f433e9f82fc1",
"d0fa6c753502e3d1df34e53ca6f6afb5cbdcd9d4",
JSMethodDef(MemberFlags.empty, s("m"),
List(ParamDef("x", NON, AnyType, mutable = false)), None,
bodyWithInterestingStuff)(
Expand Down
Loading
Loading