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

feat: support explicit exports inspection #12

Merged
merged 10 commits into from
May 23, 2024
Merged

feat: support explicit exports inspection #12

merged 10 commits into from
May 23, 2024

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented May 16, 2024

Per discussion in the TG3 meeting today, it was determined that exports information would still be beneficial in this proposal for analysis.

This refactors the GetExportedNames concrete method to instead be split into a separate GetExplicitExportedNames and PopulateExportStarSet, where GetExplicitExportedNames can be called before linking.

We then expose a new namedExports() function on the module returning the string list of explicit named exports, and a starExports() function returning the star reexports imports of the form Import[].

This PR still doesn't provide any information about the bindings, to provide a general abstraction that can work for arbitrary cyclic module records.

Rendered spec at https://guybedford.github.io/proposal-module-instance-imports/build/.

@guybedford guybedford changed the title feat: support direct exports inspection feat: support explicit exports inspection May 16, 2024
spec.emu Outdated
</tr>
<tr>
<td>
<ins>PopulateExportStarSet([_exportStarSet_])</ins>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be an abstract method that subclasses have to implement, as given the list of explicit and star exports the behavior of PopulateExportStarSet shouldn't change per module type.

Subclasses only have to provide GetExplicitExportedNames and GetStarExports, and this shuold be a non-overridable abstract operation used somewhere in the module record or cyclic module record logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is correct - PopulateExportStarSet is defined for all cyclic module records, and that is how this is written.

More generally, better separating which methods are subclassed and which are defined on the base class in the spec text might help this ambiguity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps we should turn all abstract methods on abstract base classes that are defined for the base class into abstract operations taking a first argument of the base class type?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I could confused because PopulateExportStarSet's implementation's description says

The PopulateExportStarSet concrete method of a Source Text Module Record module

rather than cyclic

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should turn all abstract methods on abstract base classes that are defined for the base class into abstract operations taking a first argument of the base class type?

I think that if we expect other subclasses to have to re-implement their own logic then they should keep being abstract methods. If we expect only one valid possible implementation, then it should just be an abstract operation rather than something on the hierarchy chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Right now the hierarchy we have (including proposals) is

Module Record -> Synthetic Module Record
Module Record -> Cyclic Module Record -> Source Text Module Record
Module Record -> Cyclic Module Record -> WebAssembly Module Record

And in practice Module Record doesn't support any logic for dependencies (unless it's fully internal to that specific type of module record), while Cyclic Module Record is the one with dependencies support.

I think:

  • GetExplicitExportedNames should be on Module Record
  • GetStarExports should be on Cyclic Module Record, since it's about dependencies.
    • It defaults to returning an empty list, so that for example Wasm module don't have to explicitly define it.
    • Whenever we need to call GetStarExports, we check if we have a Cyclic Module Record. If we don't, we hard-code the result to an empty list.
  • PopulateExportStarSet should be an AO defined inside GetModuleNamespace, since we only need it there and the "valid logic" is only one

Copy link
Member

Choose a reason for hiding this comment

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

Or, as a (maybe better) alternative:

  • We keep GetExportedNames on Module Record
  • Cyclic Module Record has abstract methods GetExplicitExportedNames and GetStarExports
  • Cyclic Module Record provides a concrete implementaton of GetExportedNames by using GetExplicitExportedNames and GetStarExports, rather than introducing PopulateExportStarSet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it is so important to separate the concepts of modules with dependencies in the function structure, since this can be captured by an empty list return. So I've gone with the first suggestion under that refactoring with the only difference that the empty GetStarExports is on the base abstract module record.

spec.emu Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

(deleted)

spec.emu Outdated Show resolved Hide resolved
guybedford and others added 2 commits May 23, 2024 10:06
Co-authored-by: Nicolò Ribaudo <hello@nicr.dev>
@guybedford guybedford merged commit 995bd4e into main May 23, 2024
1 check passed
@guybedford guybedford deleted the direct-exports branch May 23, 2024 21:23
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

3 participants