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

Fix #354 by not exporting extern resources #379

Merged
merged 3 commits into from
Jan 28, 2024
Merged

Fix #354 by not exporting extern resources #379

merged 3 commits into from
Jan 28, 2024

Conversation

b-studios
Copy link
Collaborator

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

The problem underlying the error

ReferenceError: my_resource_1396 is not defined

observed by @phischu in #354 was that extern resources have been "exported" in the separate-compilation-based Javascript backends. That is, JS code like

module.export = {
  // ...
  my_resource_1396: my_resource_1396
}

was generated, but extern resources don't have a term-level representation so the right-hand side of my_resource_1396 was unbound.

Solution Attempt

First attempt to solve #354 the problem was

  def shouldExport(sym: Symbol)(using D: DeclarationContext): Boolean = sym match {
    // do not export fields, since they are no defined functions
    case fld if D.findField(fld).isDefined => false
    // do not export effect operations, since they are translated to field selection as well.
    case op if D.findProperty(op).isDefined => false
+   // do not export extern resources, since they do not exist on the term-level
+   case r: symbols.ExternResource => false
    // all others are fine
    case _ => true
  }

However, after core we should not match on symbols anymore; so this is not a good solution, since it is not stable under renaming.

In particular, we currently do not include extern resources in the core representation:

// For now we forget about all of the following definitions in core:
case d: source.Def.Extern => Nil

Same holds for ExternType, ExternResource, ExternInterface

Chosen Solution

Instead, now I do not use module.exports on the symbol, anymore. We should try to avoid this after going to core, anyways.

This also helped simplifying the separate compilation aspect of JS a bit (3c26361) since now we do not need to filter twice. I am a little bit afraid that this might introduce a regression, but checked that the tests actually caught this before performing the refactoring.

@b-studios b-studios merged commit 85ac8f3 into master Jan 28, 2024
2 checks passed
@b-studios b-studios deleted the fix/issue354 branch January 28, 2024 10:37
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