Skip to content

Restrict usages of StoreModule even further. #5059

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.

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

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Oct 21, 2024

Alternative to #5057. The first commit is the same as the one in #5057.

@sjrd sjrd requested a review from gzm0 October 21, 2024 11:28
@@ -331,9 +331,7 @@ private final class ClassDefChecker(classDef: ClassDef,
val envJustBeforeSuper = checkBlockStats(body.beforeSuper, startEnv)
checkTreeOrSpreads(body.superCall.args, envJustBeforeSuper)
val envJustAfterSuper = envJustBeforeSuper.withThisType(instanceThisType)
val afterSuperStoreModulesHandled =
handleStoreModulesAfterSuperCtorCall(body.afterSuper)
checkBlockStats(afterSuperStoreModulesHandled, envJustAfterSuper)
Copy link
Contributor

Choose a reason for hiding this comment

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

As I commented on #5057, I don't think we should do this (but start emitting store module for JS modules as well).

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test that tests a StoreModule at the top level after the the super ctor call (but not immediately after) is not allowed.

@sjrd sjrd force-pushed the restrict-store-module-to-ctor-top-level-more branch from 837a819 to b483cb6 Compare November 2, 2024 18:25
@sjrd sjrd requested a review from gzm0 November 2, 2024 18:28
@sjrd sjrd force-pushed the restrict-store-module-to-ctor-top-level-more branch 2 times, most recently from 85a8eb5 to 68bfc6f Compare November 3, 2024 09:45
sjrd added 2 commits November 3, 2024 15:16
* Must appear exactly once, right after the super constructor call,
  in module classes.
* Cannot appear anywhere else.

This was not actually the case for JS module classes. We never
emitted `StoreModule`s in JS constructors. We use a deserialization
hack to introduce them, and add tests for the corner case that
`StoreModule` is meant to solve.

In this commit, we always enable the deserialization hack, and do
not change the compiler yet.
And only enable the deserialization when reading IR < 1.18.
@sjrd sjrd force-pushed the restrict-store-module-to-ctor-top-level-more branch from 85a8eb5 to ff197d8 Compare November 3, 2024 14:17
@sjrd
Copy link
Member Author

sjrd commented Nov 3, 2024

Fixed scalastyle 🤦‍♂️

@sjrd sjrd merged commit f6e247e into scala-js:main Nov 3, 2024
3 checks passed
@sjrd sjrd deleted the restrict-store-module-to-ctor-top-level-more branch November 3, 2024 20:08
sjrd added a commit to dotty-staging/dotty that referenced this pull request Jan 17, 2025
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
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 added a commit to scala/scala3 that referenced this pull request Jan 22, 2025
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants