Finalize AST API for v3.0

[adams85]

These are the final AST-related changes which I propose for v3.0.

The reasoning behind the changes (each represented by one commit):

1. Weighing the pros and cons discussed here, I suggest we revert Node.Range and Node.Location to public fields as we would gain little from init-only properties at the expense of breaking existing code and causing some degradation in performance.

2. On second thoughts, I suggest scrapping NodeExtensions.SetAdditionalInfo, another idea of mine too. On one hand Statement.LabelSet is set by the constructor of LabeledStatement and we'd better leave this as its responsibility also in the case of AST rewriting. On the other hand, there's not much point in copying range and location because when some child node changes during AST rewriting, in most cases the location info of the parent node won't be valid any more.

3. There are some constructors in non-sealed node classes which are not intended for inheritors. We can hide these from them using private protected.

4. Having a special case class for bigint literals (BigIntLiteral) doesn't seem to provide real value. To match the output of AST Explorer, we can just simply handle the special case in AST->JSON conversion. Thus, I think it's beneficial to remove this class to simplify the class hierarchy.

5. After taking a closer look, I realized that StaticBlock represents the static initialization block of classes. It's definitely not a special case of BlockStatement: it shouldn't be accepted in places where BlockStatement is expected. In other words, no is-a relationship exists between the two classes but rather a has-a one. Interestingly, the spec doesn't model it like that, so I propose changing it accordingly.

6. There are further design problems around property representation. On one hand, the Property class (which describes an object property) inherits from ClassProperty, although it has nothing to do with classes. On the other hand, the aforementioned StaticBlock is a class element but inherits from Node, which implies that ClassBody.Body must be declared as NodeList<Node>. Luckily, the situation can easily be improved:

   • Firstly, I suggest separating Property from ClassProperty. Additionally, we can introduce an interface to support general handling of property-like nodes (Property, MethodDefinition, PropertyDefinition). This way IProperty would serve the purpose which ClassProperty has done before.
   • Secondly, in accordance with the spec, let's introduce the ClassElement class and inherit all kinds of class elements (MethodDefinition, PropertyDefinition, StaticBlock and AccessorProperty(?) - see below) from this. Now ClassBody.Body can be declared as NodeList<ClassElement>.

   With these changes, we'd get a more sensible design at the expense of minimal number of BCs. I'm afraid though that this would interfere somewhat with your work, @lahma. So, if it's easier for you, we can postpone this change.

7. A new AccessorProperty node type has been introduced recently but it's completely unused. No instances are created of this class anywhere in the code. @Xicy Could you chime in and tell us if it can be removed safely?

[lahma]

I'm fine with the proposed changes, they seem to make sense and cleanup again nicely. I've verified them to work on Jint's side. If this is ready to merge we could consider releasing beta2 on NuGet. There probably will be more releases to come but this has already been quite the cleanup.

I one thing to consider is still that should ChildNodes return the null elements or not. Logically they are there and follow the "order of element fields", but as null they don't allow anything and as child nodes can contain N items based on for example class body elements, indexing etc won't work anyway expecting that some index would always have the same kind of thing - or null.

[lahma]

Looks good and can merge If no-one has objections.

[Xicy]

7. A new AccessorProperty node type has been introduced recently but it's completely unused. No instances are created of this class anywhere in the code. @Xicy Could you chime in and tell us if it can be removed safely?

It's seem safety to clean up. I added because estree doc added for decorators. but its never used.

[adams85]

It's seem safety to clean up. I added because estree doc added for decorators. but its never used.

Thanks for the info, I think I managed to find what's this all about:

• Align decorators to 2021-12 spec estree/estree#273
• https://github.com/tc39/proposal-grouped-and-auto-accessors

This feature looks nothing more than a proposal yet. It's not implemented in our parser, so I still think it's best to remove it for now.

If this is ready to merge we could consider releasing beta2 on NuGet.

I agree, a new beta would be nice. Additionally, the recent changes look to sort out #66 completely, so that one may be closed as well.

I one thing to consider is still that should ChildNodes return the null elements or not. Logically they are there and follow the "order of element fields", but as null they don't allow anything and as child nodes can contain N items based on for example class body elements, indexing etc won't work anyway expecting that some index would always have the same kind of thing - or null.

We now have cases like this where one can't make assumptions about fixed indices. So I think it'd be better to exclude null values and, thus, discourage consumers from relying on indices.

I did a search in Jint code and I found only a single case of this kind of stuff there. However, this one demonstrates perfectly why this kind of usage should be discouraged: it's pretty awkward from the perspective of maintainability.

Thus, I, for one, wouldn't mind removing nulls from ChildNodes. But this would also involve a revision of NodeCollection. As a matter of fact, it might deserve that anyway because it's a pretty beefy struct (consisting of 7 pointers + 3 int32s) and despite this design it can't cover every cases any more (see CreateChildNodes methods). We might need to get clever with StructLayout[LayoutKind.Explicit] or something but reworking this would need some consideration and effort for sure.

So I think it's best to proceed with beta 2 for now and address ChildNodes later (hopefully, before v3.0). We may open an issue to track it.

[lahma]

I did a search in Jint code and I found only a single case of this kind of stuff there. However, this one demonstrates perfectly why this kind of usage should be discouraged: it's pretty awkward from the perspective of maintainability.

And I have already removed that usage locally when I integrating this PR branch and checking it out. This is also the reason I brought the issue up again 😆

Thus, I, for one, wouldn't mind removing nulls from ChildNodes. But this would also involve a revision of NodeCollection. As a matter of fact, it might deserve that anyway because it's a pretty beefy struct (consisting of 7 pointers + 3 int32s) and despite this design it can't cover every cases any more (see CreateChildNodes methods). We might need to get clever with StructLayout[LayoutKind.Explicit] or something but reworking this would need some consideration and effort for sure.

So I think it's best to proceed with beta 2 for now and address ChildNodes later (hopefully, before v3.0). We may open an issue to track it.

Agree, I'll try to release beta2 soon'ish.