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

syg · 2018-12-13T19:44:16Z

There is a new [Linkable] attribute extension, which tells the
surface encoding that the [Linkable] attribute must be random
accessible.
The NodeLink type that opaquely references a [Linkable] attribute.
All lazy functions can have a FrozenArray of inner
functions that reference the immediately inner functions (i.e. not
nested within another inner function).
There are Linkable variants of all function forms. Both eager and lazy
functions are linkable, but only lazy functions have the inner
function list.
All inner functions in the list must be found to be actual inner
functions at Delazify time.

@arai-a please take a look as well.

- There is a new [Linkable] attribute extension, which tells the surface encoding that the [Linkable] attribute must be random accessible. - The NodeLink type that opaquely references a [Linkable] attribute. - All lazy functions can have a FrozenArray<NodeLink> of inner functions that reference the immediately inner functions (i.e. not nested within another inner function). - There are Linkable variants of all function forms. Both eager and lazy functions are linkable, but only lazy functions have the inner function list. - All inner functions in the list must be found to be actual inner functions at Delazify time.

arai-a · 2018-12-13T20:28:40Z

"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?

arai-a

partial review here.

mostly looks good, but I'd like to continue after the above questions answered.

arai-a · 2018-12-13T20:34:22Z

spec.html

+          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_.


shouldn't we assert that the linkedNode is direct inner function of funcNode?

Like assert that it is not nested inside another inner function? That's supposed to be taken care of by the very step: if innerFunctions isn't empty, then we throw.

does it mean that such link is allowed per file format?
and even if such link appears in the file, we should check if each entry has corresponding functions and throw SyntaxError for it if there's not?

We'll certainly know when delazifying if there's a Linkable* that doesn't exist, and we can throw then, before exer executing anything.

Is there something in particular that has you worried?

I'm thinking about the order of the error.
like, if there's 2 possible errors, which one should be observed?

maybe I should revisit this (and some other errors) once the surface encoding format is fixed.

arai-a · 2018-12-13T20:36:00Z

spec.html

+              1. Let _linkedNode_ be the node linked to by _link_.
+              1. NOTE: The above step is up to the surface encoding.
+              1. Assert: _linkedNode_ is a `LinkableFunctionDeclaration`, a `LinkableFunctionExpression`, a `LinkableMethod`, a `LinkableGetter`, a `LinkableSetter`, or a `LinkableArrowExpression`.
+              1. Add _linkedNode_ as the last element of _innerFunctions_.


shouldn't we put .fun attribute etc for each?
otherwise candidateInnerFunction below won't match any.

Good catch.

syg · 2018-12-13T20:40:07Z

"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 · 2018-12-17T08:57:40Z

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 · 2018-12-17T17:21:55Z

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 · 2018-12-17T17:42:27Z

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 · 2018-12-17T18:30:43Z

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.

…ckDirectChildFunctions to be non-exhaustive.

arai-a

If the .directChildFunctions doesn't have to contain all direct child functions, what's the expected scenario for storing only subset (or none) of those functions?
is it going to be a tuning parameter or something? or there will be some explicit criteria which functions are supposed to be listed?

also, with current spec, the order of .directChildFunctions doesn't have to be same as the order of those functions in the original source. is it intentional?
IMO requiring the exact same order doesn't introduce any issue, but just simplifies the implementation.

arai-a · 2018-12-17T20:58:00Z

spec.html

+          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_.


does it mean that such link is allowed per file format?
and even if such link appears in the file, we should check if each entry has corresponding functions and throw SyntaxError for it if there's not?

efaust

Looks like @arai-a has done most of the lifting here to polish this, but the results look good to me

syg · 2018-12-18T03:44:36Z

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 ***@***.***> wrote: @efaust commented on this pull request. In spec.html: > @@ -3042,6 +3095,25 @@ <h1>CheckBoundNames ( _expectedBound_, _actualBound_ )</h1> </emu-alg> </emu-clause> + <emu-clause id="sec-checkinnerlazyfunctions" aoid="CheckInnerLazyFunctions"> + <h1>CheckInnerLazyFunctions ( _funcNode_ )</h1> + <emu-alg> + 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 · 2018-12-18T03:58:17Z

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?

arai-a

I'm still interested into the use-case for listing only subset of direct child in the list tho,
otherwise looks good.

arai-a · 2018-12-18T18:28:47Z

spec.html

+          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_.


I'm thinking about the order of the error.
like, if there's 2 possible errors, which one should be observed?

maybe I should revisit this (and some other errors) once the surface encoding format is fixed.

syg requested a review from efaust December 13, 2018 19:44

arai-a reviewed Dec 13, 2018

View reviewed changes

Rename innerLazyFunctions and fix unwrapping in CheckInnerFunctions.

dc7a010

Rename innerFunctions to directChildFunctions for clarity; change Che…

1ea47a4

…ckDirectChildFunctions to be non-exhaustive.

arai-a reviewed Dec 17, 2018

View reviewed changes

efaust approved these changes Dec 18, 2018

View reviewed changes

arai-a approved these changes Dec 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

syg commented Dec 13, 2018

arai-a commented Dec 13, 2018

arai-a left a comment

arai-a Dec 13, 2018

syg Dec 17, 2018

arai-a Dec 17, 2018

efaust Dec 18, 2018 •

edited

Loading

arai-a Dec 18, 2018

arai-a Dec 13, 2018

syg Dec 13, 2018

syg commented Dec 13, 2018

arai-a commented Dec 17, 2018

syg commented Dec 17, 2018 •

edited

Loading

arai-a commented Dec 17, 2018

syg commented Dec 17, 2018

arai-a left a comment

arai-a Dec 17, 2018

efaust left a comment

syg commented Dec 18, 2018 via email

efaust commented Dec 18, 2018

arai-a left a comment

arai-a Dec 18, 2018

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

Are you sure you want to change the base?

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

Conversation

syg commented Dec 13, 2018

arai-a commented Dec 13, 2018

arai-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efaust Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syg commented Dec 13, 2018

arai-a commented Dec 17, 2018

syg commented Dec 17, 2018 • edited Loading

arai-a commented Dec 17, 2018

syg commented Dec 17, 2018

arai-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efaust left a comment

Choose a reason for hiding this comment

syg commented Dec 18, 2018 via email

efaust commented Dec 18, 2018

arai-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efaust Dec 18, 2018 •

edited

Loading

syg commented Dec 17, 2018 •

edited

Loading