Export more Node tests for use in public API

[jakebailey]

After #49929, we will expect downstream users to use more node tests when visiting the AST; isExpression is one function that is very commonly used in our own transforms and likely should be exported.

I'm not sure if there are any other tests we should export; probably worth a grep.

[rbuckton]

Why would we export isExpressionNode? It is kind of an esoteric function, somewhat like isParameterDeclaration, and doesn't make for a good node test.

[jakebailey]

It's mentioned in isExpression's doc comment.

[rbuckton]

Should we do the same thing for other aggregate node test functions like isStatementOrBlock, isModuleReference, isJsxChild, isLeftHandSideExpression, isUnaryExpression, isClassElement, isTypeElement, and isObjectLiteralElement (and probably some others I've forgotten)?

Originally, isStatement didn't reference parent pointers, but someone changed that at some point unfortunately, which is why there's now also isStatement. Honestly, I'd rather rename both methods so that isStatement isStatementOrBlock is the non-parent-pointer referencing one.

[rbuckton]

When I added factory/nodeTests.ts, the intent was to ensure most Node subtypes had a unique test function. I'd planned to add a lint rule to ensure no function in that file was more complex than return node.kind === SyntaxKind.Foo;, because we have a habit of changing these core functions in subtle ways.

I've been meaning to add a separate file like nodeTests.ts for all of the aggregate node tests we want to make public, with its own lint rule that no function in the file can reference .parent.

[jakebailey]

Yes, definitely; I'm going through now and exporting the ones that are used in visitEachChild, our own transformers, etc.

I'll look into isStatement; probably I can roll that into #52283.

[jakebailey]

Originally, isStatement didn't reference parent pointers, but someone changed that at some point unfortunately, which is why there's now also isStatement. Honestly, I'd rather rename both methods so that isStatement is the non-parent-pointer referencing one.

By "also isStatement", do you mean isStatementOrBlock?

For now while in draft I just exported isStatement, but what's really interesting to me is that isStatementOrBlock (the "no parent" form) is used in just one place which... immediately casts the result to Statement and ignores the Block part.

[jakebailey]

I've been meaning to add a separate file like nodeTests.ts for all of the aggregate node tests we want to make public, with its own lint rule that no function in the file can reference .parent.

That's an interesting idea; we already have nodeTests.ts so perhaps we just need to move things there or something.

[jakebailey]

And if you have a better method you want to do, by all means feel free to do it; I was just sending this PR ASAP just so downstream users have something to use for the new test-requiring stricter API.

[rbuckton]

Originally, isStatement didn't reference parent pointers, but someone changed that at some point unfortunately, which is why there's now also isStatement. Honestly, I'd rather rename both methods so that isStatement is the non-parent-pointer referencing one.

By "also isStatement", do you mean isStatementOrBlock?

For now while in draft I just exported isStatement, but what's really interesting to me is that isStatementOrBlock (the "no parent" form) is used in just one place which... immediately casts the result to Statement and ignores the Block part.

No, I meant isStatement. IIRC, isStatement used to be what isStatementOrBlock is now, but someone added more logic to it that uses parent pointers, so I added isStatementOrBlock to get back the old logic. Almost all of the public node tests are intended to be used with transforms, and transforms cannot rely on parent pointers. Any node test that does rely on parent pointers can only be safely used against a parse tree node, such as inside the checker.

[jakebailey]

I was just confused by the sentence:

Originally, isStatement didn't reference parent pointers, but someone changed that at some point unfortunately, which is why there's now also isStatement

As it says "also isStatement" when the first part mentioned that same function; I was just looking for what I was supposed to be modifying, if anything.

[rbuckton]

I've been meaning to add a separate file like nodeTests.ts for all of the aggregate node tests we want to make public, with its own lint rule that no function in the file can reference .parent.

That's an interesting idea; we already have nodeTests.ts so perhaps we just need to move things there or something.

I'd prefer to keep single-node tests in a separate file from aggregate node tests so I can eventually write lint rules to enforce their structure and avoid issues like the change that spurred the creation of isStatementOrBlock.

[rbuckton]

As it says "also isStatement" when the first part mentioned that same function; I was just looking for what I was supposed to be modifying, if anything.

Ah, I missed that bit. I've edited my comment to reference the correct function.

[rbuckton]

There are still some aggregate node tests we should include that aren't public yet. We should ensure this is limited only to the node tests needed validate child properties of each Node (such as those used explicitly in visitEachChild).

Some node tests we're not exporting, but should, include:

• isClassElement
• isTypeElement
• isObjectLiteralElement
• isDeclaration

[jakebailey]

Some node tests we're not exporting, but should, include:

• isClassElement
• isTypeElement
• isObjectLiteralElement
• isDeclaration

The first three are already exported, but let me export isDeclaration.

[rbuckton]

Aside from my comment on isDeclaration, this looks fine.