Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose more node checking / type guard API functions for developers #52727

Open
5 tasks done
JoshuaKGoldberg opened this issue Feb 12, 2023 · 16 comments
Open
5 tasks done
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@JoshuaKGoldberg
Copy link
Contributor

Suggestion

πŸ” Search Terms

type guard internal api functions tsutils ts-api-utils

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

The typescript package contains many node checking / type guard API functions for developers (isAccessor, isArrayBindingPattern, etc.). Many of them are exported for public use. Some are not. Could we expose many more of them?

A bit of history: the tsutils package has existed for many years to fill in those missing type guards & other API pieces. It hasn't been maintained in a couple of years so I'm filling out a successor, ts-api-utils with a lot of help from @RebeccaStevens. We're now discussing which parts of the TypeScript API -internal and public- the package should provide.

πŸ“ƒ Motivating Example

Two categories of @internal functions come to mind:

πŸ’» Use Cases

Developers writing logic to work with the TypeScript AST often want these functions as they're handy utilities. Many of them exist in tsutils for that reason.

import * as ts from "typescript";
import * as tsutils from "tsutils";

declare const node: ts.Node;

tsutils.isExpression(node);

Request: which of those internal / not-exported functions would you be open to a PR exposing to the public?

Related:

@RebeccaStevens
Copy link

On a slight side note: isExpression is defined in utilitiesPublic.ts but isn't publicly exposed. I interpret the filename to imply that all the functions in it are supposed to be publicly exposed. Is this not the case?
The function also isn't annotated with @internal so I'm not sure why it isn't public exposed. I guess this is just my lack of knowledge of how TypeScript get built.

@jakebailey
Copy link
Member

Many helpers including isExpression were made public in #52284, if you're missing it, I would look at a build of 5.0 to look for what's missing. (Pretend the lib folder in main doesn't exist; it's just a pinned compiler we use to build the compiler itself.)

I'm general the filenames are a lie; they were correct for a while but functions were added and moved and so it doesn't really align with anything right now. I believe @rbuckton expressed some interest in moving these around.

Some helpers are problematic in that they use parent pointers, which may not always work. But, none of those are currently public (on purpose), but probably do need some public variant.

@RebeccaStevens
Copy link

Seems this was just addressed, I guess this issue can be closed then.

@JoshuaKGoldberg I was already think this, but no I'm even more sure, we should set TS@5 as the minimum supported version of the library. I don't think it's really worth back supported older TS version for such a new library; v5 is also nice clean version to start from.

@jakebailey
Copy link
Member

Not all of them were made public; I think I only exposed ones that would be useful with the new visitor API as of #49929. In particular I remember only exposing some of the token functions. You might need more than just what would be in a transform, though.

(I didn't remove any so I'm pretty sure you could get away with 4.x too)

@RyanCavanaugh
Copy link
Member

@JoshuaKGoldberg good to close this one or are there more functions that should be exposed?

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Feb 28, 2023
@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Mar 8, 2023

Sorry for the delay @RyanCavanaugh, I was out last week - and then this took a little while to write up.

are there more functions that should be exposed?

Yes, quite a few candidates.

The following table's functions all exist in ts-api-utils and correspond to a built-in TypeScript type (e.g. ts.AbstractKeyword), but aren't publicly exported by typescript@5.0.0-beta.

Tl;dr: the β˜‘οΈ entries are ones we're requesting be made public. Also probably the πŸ”s and ⚠️s.

Emoji key:

  • β˜‘οΈ = it exists privately in typescript in roughly the same form as ts-api-utils
    • πŸ‘‰ Proposal: could these be exported please?
  • πŸ” = it exists in typescript as a method on ts.Type
    • πŸ‘‰ Proposal: for consistency, can we deprecate the ts.Type methods, and export functions instead?
    • (I'll file a separate issue if not addressed here)
  • ⚠️ = it exists privately in typescript with some noted differences from ts-api-utils
    • πŸ‘‰ Proposal: can you give advice on which of these could be exported, or otherwise what we should do?
    • (I'll file a separate issue if not addressed here)
  • ❌ = it does not exist in typescript
    • I'm guessing you don't want to add these to TypeScript?
    • (I'll file a separate issue if not addressed here)

Types

Name Status Notes
isBigIntLiteralType ❌
isConditionalType ❌
isEnumType ❌
isEvolvingArrayType ❌
isIndexedAccessType ❌
isIndexType πŸ”
isInstantiableType ❌
isIntersectionType πŸ” instead named isIntersection
isNumberLiteralType ❌
isObjectType ❌
isStringLiteralType ❌
isStringMappingType ❌
isSubstitutionType ❌
isTemplateLiteralType ❌
isTupleType ⚠️ docs note to prefer isTupleLikeType
isTupleTypeReference ❌
isTypeParameter πŸ”
isTypeReference ⚠️ very different implementation
isTypeVariable ❌
isUnionOrIntersectionType πŸ” instead named isUnionOrIntersection
isUnionType πŸ” instead named isUnion
isUniqueESSymbolType ❌

Nodes

Name Status Notes
isAbstractKeyword ⚠️ instead named isAbstractModifier
isAccessExpression β˜‘οΈ
isAccessExpression β˜‘οΈ
isAccessibilityModifier β˜‘οΈ
isAccessibilityModifier ⚠️ takes a SyntaxKind instead of a ts.Node
isAccessorDeclaration ❌
isAccessorKeyword ⚠️ instead named isAccessorModifier
isAnyKeyword ❌
isArrayBindingOrAssignmentPattern β˜‘οΈ
isArrayBindingOrAssignmentPattern β˜‘οΈ
isAssertKeyword ❌
isAssignmentPattern β˜‘οΈ
isAssignmentPattern β˜‘οΈ
isAsyncKeyword ⚠️ instead named isAsyncModifier
isAsyncKeyword ❌
isBindingOrAssignmentElementRestIndicator ❌
isBindingOrAssignmentElementRestIndicator ❌
isBindingOrAssignmentElementTarget ❌
isBindingOrAssignmentPattern β˜‘οΈ
isBindingPattern β˜‘οΈ
isBlockLike β˜‘οΈ
isBooleanKeyword ❌
isBooleanLiteral β˜‘οΈ
isClassLikeDeclaration ❌
isClassMemberModifier β˜‘οΈ
isColonToken β˜‘οΈ
isConstKeyword ❌
isDeclarationName ⚠️ TypeScript's checks are unusual
isDeclarationWithTypeParameterChildren β˜‘οΈ
isDeclarationWithTypeParameters β˜‘οΈ
isDeclareKeyword ❌
isDefaultKeyword ⚠️ instead named isDefaultModifier
isDefaultKeyword ❌
isDestructuringPattern ❌
isDotToken ❌
isEndOfFileToken ❌
isEntityNameExpression β˜‘οΈ
isEntityNameOrEntityNameExpression ❌
isEqualsGreaterThanToken β˜‘οΈ
isEqualsToken ❌
isExclamationToken β˜‘οΈ
isExportKeyword ⚠️ instead named isExportModifier
isExportKeyword ❌
isFalseKeyword ❌
isFalseLiteral ❌
isForInOrOfStatement β˜‘οΈ
isFunctionLikeDeclaration β˜‘οΈ
isHasDecorators ❌
isHasExpressionInitializer ❌
isHasInitializer ❌
isHasJSDoc ❌
isHasModifiers ❌
isHasType ❌
isHasTypeArguments ❌
isImportExpression ⚠️ instead named isImportKeyword
isImportExpression ❌
isImportKeyword ⚠️ type is named ImportExpression
isInKeyword ❌
isInputFiles ❌
isIterationStatement β˜‘οΈ
isJSDocComment ❌
isJSDocNamespaceBody β˜‘οΈ
isJSDocNamespaceDeclaration ❌
isJSDocText ❌
isJSDocTypeReferencingNode ❌
isJsonMinusNumericLiteral ❌
isJsonObjectExpression ❌
isJsxAttributeValue ❌
isJsxTagNamePropertyAccess ❌
isLiteralToken ❌
isNamedImportsOrExports β˜‘οΈ
isNamespaceBody β˜‘οΈ
isNamespaceDeclaration ❌
isNeverKeyword ❌
isNullKeyword ❌
isNullLiteral ❌
isNumberKeyword ❌
isObjectBindingOrAssignmentElement β˜‘οΈ
isObjectBindingOrAssignmentPattern β˜‘οΈ
isObjectKeyword ❌
isObjectTypeDeclaration β˜‘οΈ
isOutKeyword ❌
isOverrideKeyword ⚠️ instead named isOverrideModifier
isOverrideKeyword ❌
isParameterPropertyModifier β˜‘οΈ
isPrivateKeyword ❌
isPropertyAccessEntityNameExpression β˜‘οΈ
isPropertyNameLiteral β˜‘οΈ
isProtectedKeyword ❌
isPseudoLiteralToken ❌
isPublicKeyword ❌
isReadonlyKeyword β˜‘οΈ
isReadonlyKeyword β˜‘οΈ
isSignatureDeclaration ❌
isStaticKeyword ⚠️ instead named isStaticModifier
isStaticKeyword ❌
isStringKeyword ❌
isSuperElementAccessExpression ❌
isSuperExpression ⚠️ instead named isSuperKeyword
isSuperExpression ❌
isSuperKeyword ⚠️ type is named SuperExpression
isSuperProperty β˜‘οΈ
isSuperPropertyAccessExpression ❌
isSymbolKeyword ❌
isSyntaxList β˜‘οΈ
isThisExpression ❌
isThisKeyword ❌
isTrueKeyword ❌
isTrueLiteral ❌
isTypeOnlyCompatibleAliasDeclaration ❌
isTypeReferenceType β˜‘οΈ
isUndefinedKeyword ❌
isUnionOrIntersectionTypeNode ❌
isUnknownKeyword ❌
isUnparsedPrologue ❌ type is marked as deprecated?
isUnparsedSourceText ❌ type is marked as deprecated?
isUnparsedSyntheticReference ❌ type is marked as deprecated?
isVariableLikeDeclaration ❌
isVoidKeyword ❌

Note: there are likely a few mistakes in this chart; it was boring and tedious to generate. Please use it as a rough lay-of-the-land rather than precise technical documentation.

@jakebailey
Copy link
Member

RE: methods on Type and such, these are all declared here (and truthfully, I spend so little time in services that I forgot they existed!): https://github.com/microsoft/TypeScript/blob/main/src/services/types.ts

Notably, isUnion, isIntersection, isUnionOrIntersection are all defined there (but appear to have been noted as being missing above). But, yes, I read the disclaimer πŸ˜…

I haven't looked closely, but my impression from having exported a bunch of stuff in 5.0 is that anything to do with nodes that turn out to be simple kind checks are very safe for us to export, although I'm very surprised that so many of there are marked ❌.

I think I mentioned this above, but, I think there's some intent to try and move all of these helpers to a file and make them complete, which would solve some of this at least.

(If only Node were a union, then most of these checks would be moot!)

@JoshuaKGoldberg
Copy link
Contributor Author

Notably, isUnion, isIntersection, isUnionOrIntersection are all defined there (but appear to have been noted as being missing above). But, yes, I read the disclaimer πŸ˜…

Aha, thanks, I've updated the table.

some intent to try and move all of these helpers to a file and make them complete

Is there a tracking issue? If not, is that statement enough to count as accepting PRs? πŸ˜„

(If only Node were a union, then most of these checks would be moot!)

Is there a tracking issue for this? @bradzacher and I have mentioned this to each other a few times, that it'd be great to make that change at some point. +1

@jakebailey
Copy link
Member

jakebailey commented Mar 8, 2023

Is there a tracking issue? If not, is that statement enough to count as accepting PRs? πŸ˜„

No, I don't there is; I'm going off some commentary from @rbuckton in my strictFunctionTypes PR.

Is there a tracking issue for this? @bradzacher and I have mentioned this to each other a few times, that it'd be great to make that change at some point. +1

Also no; I think that this was prohibitively slow. So much so that a version of the compiler with Node as a union is a benchmark (Compiler-Unions, if you've ever seen the perf runner output).

@jakebailey
Copy link
Member

Do you mind if I restructure your comment a little to stick Node and Type into separate sections? The bulk of them are Node, where I think those are going to be uncontroversial as they're very likely to be all kind checks.

However, for Type, it's very likely that we actually don't want to export things from the checker and instead want people to check flags, e.g. isBigIntLiteralType is when type.flags & TypeFlags.BigIntLiteral. But, maybe we should have top-level type helpers for that? Not sure how much agency we want to leave ourselves in terms of changing these flags, but, they are exported so I doubt they really change.

@JoshuaKGoldberg
Copy link
Contributor Author

Go ahead, thanks for asking!

@jakebailey jakebailey removed the Needs More Info The issue still hasn't been fully clarified label Mar 9, 2023
@RebeccaStevens
Copy link

Could IntrinsicType be made public?

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 30, 2023
@jakebailey
Copy link
Member

jakebailey commented Apr 4, 2023

The notes don't seem to be out yet, but we had a long discussion about this in a design meeting and it seems like we're landing at only de-internalizing the Node-related ones we've already declared, and not necessarily adding any more unless we ourselves use it.

So, that'd roughly translate to anything with a red X above not being created, but everything else is fair game, whatever dropping @internal fixes.

As for Type, I don't think any of those were going to be exported. Type helpers are a little more challenging because they have to be exported on the TypeChecker object.

@Rich-Harris
Copy link

In the interim, is tsutils the recommended way to identify things like export modifiers? In .ts files I suppose we could do node.kind === SyntaxKind.ExportKeyword, but of course this doesn't work in a .js context

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jun 12, 2023

@Rich-Harris tsutils was a commonly-community-recommended way, but it hasn't been updated in a couple of years and the maintainer hasn't responded to our (typescript-eslint team) pings. We've switched to ts-api-utils instead.

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jun 17, 2023

@jakebailey could hasAmbientModifier & other hasSyntacticModifier wrappers be made public too please? It'd be useful for typescript-eslint/typescript-eslint#6723 (comment).

Edit: and/or, nodeCanBeDecorated? For the same thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants