Skip to content

Merge the IR node This into VarRef, with a magic LocalName. #5090

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

Jump to bottom
Merged
merged 1 commit into from
Dec 27, 2024
Merged

Merge the IR node This into VarRef, with a magic LocalName. #5090

merged 1 commit into from
Dec 27, 2024

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Dec 17, 2024 •
edited
Loading

Everywhere we handle VarRefs, we applied the same treatment to This nodes. In some places, like in the optimizer's Bindings, we also had an additional abstraction to represent either an actual VarRef or a this value.

We now merge This as a particular case of VarRef. We use a magic LocalName that is otherwise invalid, namely .this.

A variable of that name cannot be declared, either in a ParamDef or in a VarDef. It is only introduced for receivers, and is always immutable.

Several areas of the code get simpler with the merge. The optimizer's Bindings is the most obvious example. The FunctionEmitters also benefit from the changes. Other areas get nothing but a reduction of alternatives in pattern matches.

this values still hold particular meaning in many situations. Therefore, we keep a source compatible constructor/extractor object for This().

The only difference in the eventual output is:

@@ -185322,8 +185327,7 @@
     return this.jl_Throwable__f_e;
   }
   fillInStackTrace__jl_Throwable() {
-    var $x_1 = this;
-    var reference = (($x_1 instanceof $c_sjs_js_JavaScriptException) ? $x_1.sjs_js_JavaScriptException__f_exception : $n($x_1));
+    var reference = ((this instanceof $c_sjs_js_JavaScriptException) ? this.sjs_js_JavaScriptException__f_exception : $n(this));
     var identifyingString = Object.prototype.toString.call(reference);
     this.jl_Throwable__f_jsErrorForStackTrace = ((identifyingString === "[object Error]") ? reference : (((Error.captureStackTrace === (void 0)) || $uZ(Object.isSealed(this))) ? new Error() : (Error.captureStackTrace(this), this)));
     return this;

@sjrd sjrd requested a review from gzm0 December 17, 2024 11:00
@sjrd sjrd force-pushed the ir-merge-this-into-varref branch from 9c6d33f to 30f2bc0 Compare December 17, 2024 12:19
@sjrd sjrd force-pushed the ir-merge-this-into-varref branch 2 times, most recently from 035a4a5 to dc5febf Compare December 18, 2024 14:53
@sjrd
Copy link
Member Author

sjrd commented Dec 21, 2024

This PR will look better when/if #5092 gets merged first.

@sjrd sjrd mentioned this pull request Dec 21, 2024
Merged
@sjrd sjrd force-pushed the ir-merge-this-into-varref branch from dc5febf to 29c4439 Compare December 22, 2024 14:32
@sjrd
Copy link
Member Author

sjrd commented Dec 22, 2024

Rebased.

@sjrd sjrd force-pushed the ir-merge-this-into-varref branch from 29c4439 to 2202503 Compare December 22, 2024 22:08
gzm0
gzm0 requested changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Nice! Just some comments about details.

@sjrd sjrd force-pushed the ir-merge-this-into-varref branch from 2202503 to c47cd54 Compare December 25, 2024 15:59
sjrd
sjrd commented Dec 25, 2024
View reviewed changes
Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Updated.

@sjrd sjrd requested a review from gzm0 December 25, 2024 16:00
gzm0
gzm0 approved these changes Dec 27, 2024
View reviewed changes
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Just one nit RE naming (optional).

linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala Outdated
copy(thisType = thisType)
withLocal(LocalDef(LocalName.This, thisType, mutable = false))

def withMaybeThisType(static: Boolean, thisType: Type): Env =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: static should maybe better be called hasThis here? (or omitThis).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yes. Fixed. (and rebased just for safety)

@sjrd
Merge the IR node This into VarRef, with a magic LocalName.
eac8bf7 
Everywhere we handle `VarRef`s, we applied the same treatment to
`This` nodes. In some places, like in the optimizer's `Binding`s,
we also had an additional abstraction to represent either an actual
`VarRef` or a `this` value.

We now merge `This` as a particular case of `VarRef`. We use a
magic `LocalName` that is otherwise invalid, namely `.this`.

A variable of that name cannot be declared, either in a `ParamDef`
or in a `VarDef`. It is only introduced for receivers, and is
always immutable.

Several areas of the code get simpler with the merge. The
optimizer's `Binding`s is the most obvious example. The
`FunctionEmitter`s also benefit from the changes. Other areas get
nothing but a reduction of alternatives in pattern matches.

`this` values still hold particular meaning in many situations.
Therefore, we keep a source compatible constructor/extractor object
for `This()`.
@sjrd sjrd force-pushed the ir-merge-this-into-varref branch from c47cd54 to eac8bf7 Compare December 27, 2024 11:01
@sjrd sjrd merged commit 25b2725 into scala-js:main Dec 27, 2024
3 checks passed
@sjrd sjrd deleted the ir-merge-this-into-varref branch December 27, 2024 15:46
sjrd added a commit to dotty-staging/dotty that referenced this pull request Jan 17, 2025
@sjrd
Scala.js: Upgrade to Scala.js 1.18.1.
86c304c 
And port the changes that were made to the compiler backend.
Most nobably, the changes from the following upstream PRs:

* Introduce non-nullable reference types in the IR.
  scala-js/scala-js#5018
* Opt: Remove useless *Ident indirections in the IR model.
  scala-js/scala-js#5092
* Merge the IR node This into VarRef, with a magic LocalName.
  scala-js/scala-js#5090
* Merge several IR nodes into UnaryOp.
  scala-js/scala-js#5088
* Restrict usages of StoreModule even further.
  scala-js/scala-js#5059
* Allow Return arg to be a void if the target Labeled is a void.
  scala-js/scala-js#5074
* Rename NoType to VoidType and print it as "void".
  scala-js/scala-js#5061
sjrd added a commit to dotty-staging/dotty that referenced this pull request Jan 17, 2025
@sjrd
Scala.js: Upgrade to Scala.js 1.18.1.
1b30bdc 
And port the changes that were made to the compiler backend.
Most notably, the changes from the following upstream PRs:

* Introduce non-nullable reference types in the IR.
  scala-js/scala-js#5018
* Opt: Remove useless *Ident indirections in the IR model.
  scala-js/scala-js#5092
* Merge the IR node This into VarRef, with a magic LocalName.
  scala-js/scala-js#5090
* Merge several IR nodes into UnaryOp.
  scala-js/scala-js#5088
* Restrict usages of StoreModule even further.
  scala-js/scala-js#5059
* Allow Return arg to be a void if the target Labeled is a void.
  scala-js/scala-js#5074
* Rename NoType to VoidType and print it as "void".
  scala-js/scala-js#5061
@sjrd sjrd mentioned this pull request Jan 17, 2025
Merged
sjrd added a commit to scala/scala3 that referenced this pull request Jan 22, 2025
@sjrd
Scala.js: Upgrade to Scala.js 1.18.1. (#22397)
270832c 
And port the changes that were made to the compiler backend.
Most notably, the changes from the following upstream PRs:

* scala-js/scala-js#5018
* scala-js/scala-js#5092
* scala-js/scala-js#5090
* scala-js/scala-js#5088
* scala-js/scala-js#5059
* scala-js/scala-js#5074
* scala-js/scala-js#5061
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekrich ekrich ekrich left review comments

@gzm0 gzm0 gzm0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sjrd @ekrich @gzm0