Skip to content
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

Website breaks to missing extern definitions #374

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

b-studios
Copy link
Collaborator

@b-studios b-studios commented Jan 26, 2024

All examples on the website output

println$impl is not defined

The reason is that recently introduced inlining of extern definitions #350 will inline definitions like println

extern io def println[R](value: R): Unit =
"println$impl(${value})"

which in turn rely on extern includes or literals, such as

extern include "effekt_builtins.js"

The problem now is that the website uses separate compiling, where the extern includes are only visible to the one module including them. Previously, println would simply close over the extern includes. But now, since we inline it, it cannot anymore.

Solution implemented in this hotfix

Inlining externs now differs (in JS) between separate and whole program compilation.

  • When compiling whole-program, we inline extern defs.
  • When compiling separate, we do not inline extern defs, since:

Drawback

  • it adds complexity in the implementation of the javascript Transformer
  • the hotfix looses some code-sharing with the direct-style backend, which currently is not used (neither in tests, nor on the website), since I moved some code into the "Monadic..." variants to easier control inlining.

@b-studios b-studios changed the title Add test that reproduces website failure Website breaks to missing extern definitions Jan 26, 2024
@b-studios b-studios marked this pull request as ready for review January 26, 2024 17:36
@b-studios
Copy link
Collaborator Author

b-studios commented Jan 26, 2024

@dvdvgt once the PR is merged, could you update the website again and see whether this resolves the problem? (I clicked the CI button). The test I added should actually reproduce this error, though.

@b-studios b-studios merged commit 49f2166 into master Jan 26, 2024
2 checks passed
@b-studios b-studios deleted the hot-fix/separate-compilation-inlines branch January 26, 2024 17:43
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.

None yet

1 participant