tc39 / proposal-binary-ast

Binary AST proposal for ECMAScript
965 stars 23 forks source link

Give lazy functions ability to assert list of inner functions. #69

Open syg opened 5 years ago

syg commented 5 years ago

@arai-a please take a look as well.

arai-a commented 5 years ago

"random accessible" means that the reader doesn't have to iterate over all statements to reach the function, but just by following the NodeLink with 1 step, right?

What's the purpose / expected usage of this? given CheckInnerLazyFunctions does the iteration and all checks for the path between them, I guess the merit of this exists in earlier stage, but not sure.

Then, is it correct that lazyInnerFunctions should be an exhaustive list of all directly inner functions, with 1:1 match? I was wondering what "can" in "All lazy functions can have ..." means, either it's optional thing or required thing.

Also, why is the field named lazyInnerFunctions while the link target can be eager function?

syg commented 5 years ago

"random accessible" means that the reader doesn't have to iterate over all statements to reach the function, but just by following the NodeLink with 1 step, right?

Yep!

What's the purpose / expected usage of this? The motivation for this was so that an implementation can choose to allocate Function objects and e.g. LazyScript instances up front if they choose. Some data from @efaust suggested that this would help performance.

Then, is it correct that lazyInnerFunctions should be an exhaustive list of all directly inner functions, with 1:1 match?

Good question, I went back and forth on this. It's not required to be exhaustive currently. My reasoning being it's not a correctness issue if the list is not exhaustive -- this list has no bearings on scopes and bindings.

Also, why is the field named lazyInnerFunctions while the link target can be eager function?

Oops, good catch. I'll rename that to just innerFunctions.

arai-a commented 5 years ago

Thanks, makes sense.

Then, is it correct that lazyInnerFunctions should be an exhaustive list of all directly inner functions, with 1:1 match?

Good question, I went back and forth on this. It's not required to be exhaustive currently. My reasoning being it's not a correctness issue if the list is not exhaustive -- this list has no bearings on scopes and bindings.

the current change requires it to be an exhaustive list (no duplication, no unmatched items), and in that case I think it's fine. if this becomes optional, I'm not sure if it's beneficial (given the implementation needs to handle unmatched cases separately)

syg commented 5 years ago

the current change requires it to be an exhaustive list (no duplication, no unmatched items), and in that case I think it's fine.

Ah, I think I misunderstood what you meant by exhaustive. The current PR is exhaustive in the sense you say here: that all inner functions listed in .innerFunctions must be accounted for. The current PR is not exhaustive in the sense that all actual inner functions be listed in .innerFunctions. It's fine to not list some inner functions ahead of time.

arai-a commented 5 years ago

The current PR is not exhaustive in the sense that all actual inner functions be listed in .innerFunctions. It's fine to not list some inner functions ahead of time.

by "inner function", do you also mean nested functions?

if not, that doesn't sound right. if actual inner function is missing from .innerFunctions list, the error is thrown in the following step:

If candidateInnerFunction is not in innerFunctions, then throw a SyntaxError exception.

anyway, I think we should carefully use "inner function" term to make sure which set it means.

syg commented 5 years ago

if not, that doesn't sound right. if actual inner function is missing from .innerFunctions list, the error is thrown in the following step:

Ahh, oops, you are correct. The current algorithm isn't at all what I intended, will fix.

anyway, I think we should carefully use "inner function" term to make sure which set it means.

Will reword.

syg commented 5 years ago

The current PR doesn’t handle surface encoding errors like linking to nonexistent functions. I should put a note.

The checking at delazify time is about verifying that the functions are in fact direct children, which you wouldn’t know without actually parsing the whole function.

On Dec 17, 2018, at 20:43, Eric Faust notifications@github.com wrote:

@efaust commented on this pull request.

In spec.html:

@@ -3042,6 +3095,25 @@

CheckBoundNames ( expectedBound, actualBound )

  • CheckInnerLazyFunctions ( _funcNode_ )

    1. Assert: funcNode is a LazyFunctionDeclaration, a LazyFunctionExpression, a LazyMethod, a LazyGetter, a LazySetter, or a LazyArrowExpression.
    1. NOTE: All asserted immediately inner (i.e. not nested within another inner function) lazy functions in innerLazyFunctions must be found.
    1. Let innerFunctions be a new empty List.
    1. For each link in funcNode.innerLazyFunctions, do
    1. Let linkedNode be the node linked to by link. We'll certainly know when delazifying if there's a Linked* that doesn't exist, and we can throw then, before exer executing anything.

Is there something in particular that has you worried?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

efaust commented 5 years ago

The current PR doesn’t handle surface encoding errors like linking to nonexistent functions. I should put a note.

The way you have this written, it'll be residual in the check, since it won't be removed as an immediate child if you point to bogus, right?