Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Missing call to FunctionDeclarationInstantion in EvaluateBody #38

Closed
littledan opened this issue Aug 29, 2017 · 10 comments
Closed

Missing call to FunctionDeclarationInstantion in EvaluateBody #38

littledan opened this issue Aug 29, 2017 · 10 comments

Comments

@littledan
Copy link
Member

We need this call, or an analogous lower-level thing, to set up scopes. See this thread for context. Thanks @loganfsmyth for pointing out the issue.

@loganfsmyth
Copy link

loganfsmyth commented Aug 29, 2017

Cool, thanks for clarifying. Yeah this is an interesting one. Instantiating a new function to essentially contain the static field initializations certainly seems like the least drastic approach, e.g.

class Example {
  static prop = "value";
  static prop2 = () => this.prop;
}

becoming kind of like

class Example { }
(function(){
  this.prop = "value";
  this.prop2 = () => this.prop;
}).call(Example);

I'd figure that it should just be around the final new steps that prep the environment and call InitializeStaticFields?

Edit: The rest of this was me misreading the spec, woops!

@littledan One question related to this, for both static and non-static props. What's the expected behavior for computed keys, e.g.

(function(){
  class Example {
    toString(){
      return "innerKey";
    }
    static toString(){
      return "staticKey";
    }

    [this] = "non-static";
    [this](){}
    static [this] = "static";
    static [this](){}
  }
}).call({
  toString(){
    return "outerKey";
  },
});

It seems like with the current spec for non-statics, and the behavior proposed in this issue, this ends up as

class Example {
  innerKey = "non-static";
  outerKey(){}
  static staticKey = "static";
  static outerKey(){}
}

but that behavior for props is pretty inconsistent with the current behavior with methods. Should the execution of property keys be happen at class evaluation time?

@littledan
Copy link
Member Author

Computed field keys are still evaluated once, in the same scope and at the same time where the computed method keys are evaluated.

@littledan
Copy link
Member Author

For example, this in a computed method or field key refers to the outer this.

@loganfsmyth
Copy link

Oh shoot, you are totally right, I misread. Disregard the second half of that then.

@littledan
Copy link
Member Author

To answer the first part, I thought this would be better to add in EvaluateBody, which would match now normal functions do it.

Please, keep the clarifying questions and bug reports coming; I really appreciate the work you're doing here.

@loganfsmyth
Copy link

Could you clarify what you mean about that? Not quite making the jump from ClassDefinitionEvaluation to where FunctionDeclarationInstantion/EvaluateBody would tie in. My psuedo-example isn't super clear, but I was thinking in ClassDefinitionEvaluation would make sense since that was where the static fields and the constructor are initialized.

@littledan
Copy link
Member Author

I think the outer context is captured by the FunctionCreate call from ClassFieldDefinitionEvaluation .

@littledan
Copy link
Member Author

Sorry, looking at all this again, I think the current spec already has the mechanics for the sequence of events, which is:

  • During class evaluation, ClassFieldDefinitionEvaluation calls FunctionCreate which captures the current scope.
  • During [[Construct]], super or later in class evaluation, DefineField invokes Call on each Initializer. PrepareForOrdinaryCall restores the appropriate context.

There are a couple editorial issues in this code path which I'll fix up.

@loganfsmyth
Copy link

Thanks for looking again, this does seem to cover it, so we should be all set. Following the chain of events through the calls was tripping me up a bunch. I think I had just missed that there was that FunctionCreate call in ClassFieldDefinitionEvaluation that would handle it, or at least not followed how the pieces fit together fully.

@littledan
Copy link
Member Author

This is all tricky stuff; you can see that I didn't understand it immediately either!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants