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

Editorial: Fix null [[ScriptOrModule]] of function declarations in modules #1670

Open
wants to merge 1 commit into
base: master
from

Conversation

@devsnek
Copy link
Member

commented Aug 15, 2019

There is no context on the stack that contains the current module while modules are setting up their environments, unlike scripts which push their context right before GlobalDeclarationInstantiation. This results in function declarations in modules having null [[ScriptOrModule]] slots. This breaks import.meta and dynamic import inside modules.

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@ljharb suggested adding a [[Module]] slot to module environments and using that in FunctionInitialize.

I'm fine with either approach.

@ljharb ljharb added the spec bug label Aug 15, 2019

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

There is no context on the stack that contains the current module while modules are setting up their environments,

Why don't we fix this instead, i.e. make modules and scripts symmetrical?

@ljharb

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

My understanding is that for exported declarations, the module isn’t evaluating, so it doesn’t belong on the stack - iow, they can’t be symmetrical because they have a linking phase that scripts lack.

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

You could still put it on the stack, even if you're not evaluating anything, in order to get the correct results from various algorithms in the spec.

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@domenic the module's context doesn't exist until evaluation

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

I see. It still seems better to me to move that to linking time. I.e. create the context, and push it onto the stack during linking. (Then pop it off.) Then evaluation can just push/pop the already-created context.

In particular, I find this PR bad because it indicates that there are a lot of mechanisms in the spec that expect a context to be on the stack while setting up functions. We found out that that is not always true, but I'd rather preserve that invariant and restore the assumption, instead of compensating for it.

@allenwb

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@domenic the module's context doesn't exist until evaluation

Yes, and the parts of InitializeEnvironment that correspond to "linking" should not have evaluation-time dependencies because we don't want to preclude doing them with an ahead-of-time linker.

To me, it would seem better to add an scriptOrModule parameter to InstantiateFunctionObject so module InitializeEnvironment can explicitly pass it when instantiating module top-level functions.

@devsnek devsnek force-pushed the devsnek:fix/instantiate-function-in-module branch from 8eba4e1 to ea4cbc2 Aug 19, 2019

@devsnek

This comment was marked as off-topic.

Copy link
Member Author

commented Sep 6, 2019

so we've got:

  • create context ahead of time, push onto stack
  • add a new argument down the chain of InstantiateFunctionObject to FunctionCreate to FunctionAllocate
  • add a [[Module]] slot on environment (what the pr does at the time of this comment)

@devsnek devsnek force-pushed the devsnek:fix/instantiate-function-in-module branch 2 times, most recently from d7891db to 3cf3dc8 Sep 7, 2019

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved

@devsnek devsnek force-pushed the devsnek:fix/instantiate-function-in-module branch from 3cf3dc8 to 05927a9 Sep 7, 2019

@ljharb
ljharb approved these changes Sep 7, 2019

@ljharb ljharb requested review from jmdyck, allenwb and domenic Sep 7, 2019

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

this changeset has been verified with engine262 👍

spec.html Show resolved Hide resolved

@devsnek devsnek force-pushed the devsnek:fix/instantiate-function-in-module branch from 05927a9 to b266fd5 Sep 9, 2019

@devsnek devsnek force-pushed the devsnek:fix/instantiate-function-in-module branch from b266fd5 to 36c17fc Sep 9, 2019

@domenic
domenic approved these changes Sep 9, 2019

@ljharb ljharb self-assigned this Sep 9, 2019

@ljharb ljharb requested a review from jmdyck Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.