From b5b9a185c8b8df344c8be8c639887df8826c660e Mon Sep 17 00:00:00 2001 From: reillylm Date: Tue, 28 Jan 2025 16:22:15 -0500 Subject: [PATCH] improve acorn-walk types, add ".d.ts tests" using `tsd` This makes two changes to the TS types for acorn-walk. **Using `AnyNode` in more places** We currently have the discriminated union `AnyNode`, who's getting used in the functions who accept a visitor object. It enables us to receive the actual Node type as an argument to our visitor function: ``` walk.simple(ast, { Literal: (n) => { // works because our visitor function receives a node of type: // Extract console.log(n.value) } }) ``` If we weren't using the discriminated union, `n` would be of type `Node`, and when we try to access `n.value`, we'd get the TS error `TS2339: Property value does not exist on type Node`. With this change, we now use `AnyNode` in the other places where we receive a Node in a callback, like `findNodeAt`: ``` walk.findNodeAt(ast, null, null, (_, n) => { return n.type === "Literal" && n.raw?.includes("someSubstring") }) ``` With the discriminated union, once we confirm `n.type === "Literal"`, TS can "know" that `n` is a `Literal`, and we can access its fields appropriately. This doesn't change `Node` to `AnyNode` in the parameters to the walker functions themselves, since we don't use TS in the library itself, so we have no need to disambiguate between input Nodes. If we pass e.g. a `BlockStatement` as the first argument to `simple`, that's fine for TS because a `BlockStatement` is a `Node`. I don't think it would hurt anything to change those occurrences to `AnyNode` too, though. **Cleaning up some duplication** The visitor object types previously contained a little duplication because visitor objects can contain either Node type names (like `Literal`) or Aggregate type names (like `Pattern`) as keys. Previously, we used an intersection type (`{} & {}`) to account for the Node/Aggregate cases, but now there's a single conditional type who we can share between the visitor object types. Hopefully giving names to all the different parts makes things clearer too. Also, with `NodeOrAggregateTypeName` representing both sets of type names, we can make the typing a little more accurate for the functions where we receive the type name in a callback. **Adding type definition tests** As part of making the change, I wrote some "type definition tests" using the [`tsd` library](https://github.com/tsdjs/tsd), where we can assert things like "when we write a visitor function for the Node type `Literal`, the walker function will call it with a `Literal` as the first argument." I found them helpful in giving me some confidence while making the changes (just as regular tests do), so I feel like they're worth having if we ever need to change the types. We can run the same test with both the `walk.d.ts` before and after the changes, to see which new scenarios are accounted for with the changes. I excluded the test file from ESLint; we could add it, it would just require us adding the [TS ESLint plugin](https://typescript-eslint.io/getting-started/legacy-eslint-setup/), but I wasn't sure if we wanted that (I noticed the `test` directory is also currently excluded from ESLint). We could also make the test run as part of CI. It runs with `npx tsd` in the acorn-walk directory and uses the test in the `test-d` directory to test `dist/walk.d.ts` by default. --- .eslintrc.js | 1 + acorn-walk/src/walk.d.ts | 69 ++++++----- acorn-walk/test-d/walk.test-d.ts | 202 +++++++++++++++++++++++++++++++ package.json | 3 +- 4 files changed, 245 insertions(+), 30 deletions(-) create mode 100644 acorn-walk/test-d/walk.test-d.ts diff --git a/.eslintrc.js b/.eslintrc.js index 1dcbf606..1abae6ab 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -19,6 +19,7 @@ module.exports = { } } ], + ignorePatterns: ["*.ts"], plugins: ["eslint-plugin-import"], rules: { "no-unreachable-loop": "off", diff --git a/acorn-walk/src/walk.d.ts b/acorn-walk/src/walk.d.ts index 4e5199ca..70a6fb97 100644 --- a/acorn-walk/src/walk.d.ts +++ b/acorn-walk/src/walk.d.ts @@ -1,18 +1,5 @@ import * as acorn from "acorn" -export type FullWalkerCallback = ( - node: acorn.Node, - state: TState, - type: string -) => void - -export type FullAncestorWalkerCallback = ( - node: acorn.Node, - state: TState, - ancestors: acorn.Node[], - type: string -) => void - type AggregateType = { Expression: acorn.Expression, Statement: acorn.Statement, @@ -22,31 +9,55 @@ type AggregateType = { ForInit: acorn.VariableDeclaration | acorn.Expression } +type NodeTypeName = acorn.AnyNode["type"] +type AggregateTypeName = keyof AggregateType +type NodeOrAggregateTypeName = NodeTypeName | AggregateTypeName + +export type FullWalkerCallback = ( + node: acorn.AnyNode, + state: TState, + type: NodeOrAggregateTypeName, +) => void + +export type FullAncestorWalkerCallback = ( + node: acorn.AnyNode, + state: TState, + ancestors: acorn.AnyNode[], + type: NodeOrAggregateTypeName, +) => void + +type ActualNodeForType = Name extends AggregateTypeName + ? AggregateType[Name] + : Extract + +type SimpleVisitorFunction = + (node: ActualNodeForType, state: TState) => void + +type AncestorVisitorFunction = + (node: ActualNodeForType, state: TState, ancestors: acorn.AnyNode[]) => void + +export type WalkerCallback = + (node: ActualNodeForType, state: TState) => void + +type RecursiveVisitorFunction = + (node: ActualNodeForType, state: TState, callback: WalkerCallback) => void + export type SimpleVisitors = { - [type in acorn.AnyNode["type"]]?: (node: Extract, state: TState) => void -} & { - [type in keyof AggregateType]?: (node: AggregateType[type], state: TState) => void + [typeName in NodeOrAggregateTypeName]?: SimpleVisitorFunction } export type AncestorVisitors = { - [type in acorn.AnyNode["type"]]?: ( node: Extract, state: TState, ancestors: acorn.Node[] -) => void -} & { - [type in keyof AggregateType]?: (node: AggregateType[type], state: TState, ancestors: acorn.Node[]) => void + [typeName in NodeOrAggregateTypeName]?: AncestorVisitorFunction } -export type WalkerCallback = (node: acorn.Node, state: TState) => void - export type RecursiveVisitors = { - [type in acorn.AnyNode["type"]]?: ( node: Extract, state: TState, callback: WalkerCallback) => void -} & { - [type in keyof AggregateType]?: (node: AggregateType[type], state: TState, callback: WalkerCallback) => void + [typeName in NodeOrAggregateTypeName]?: RecursiveVisitorFunction } -export type FindPredicate = (type: string, node: acorn.Node) => boolean +export type FindPredicate = (type: NodeOrAggregateTypeName, node: acorn.AnyNode) => boolean export interface Found { - node: acorn.Node, + node: acorn.AnyNode, state: TState } @@ -123,7 +134,7 @@ export function findNodeAt( node: acorn.Node, start: number | undefined | null, end?: number | undefined | null, - type?: FindPredicate | string, + type?: FindPredicate | NodeOrAggregateTypeName, base?: RecursiveVisitors, state?: TState ): Found | undefined @@ -134,7 +145,7 @@ export function findNodeAt( export function findNodeAround( node: acorn.Node, start: number | undefined | null, - type?: FindPredicate | string, + type?: FindPredicate | NodeOrAggregateTypeName, base?: RecursiveVisitors, state?: TState ): Found | undefined diff --git a/acorn-walk/test-d/walk.test-d.ts b/acorn-walk/test-d/walk.test-d.ts new file mode 100644 index 00000000..2ca1fafd --- /dev/null +++ b/acorn-walk/test-d/walk.test-d.ts @@ -0,0 +1,202 @@ +import { + expectAssignable, + expectError, + expectNotAssignable, + expectType +} from "tsd" + +import * as acorn from "acorn" +import * as walk from ".." + +type simpleParams = Parameters +type ancestorParams = Parameters +type recursiveParams = Parameters +type makeParams = Parameters +type fullParams = Parameters +type fullAncestorParams = Parameters +type findNodeAtParams = Parameters +type findNodeAroundParams = Parameters +type findNodeAfterParams = Parameters +type findNodeBeforeParams = Parameters + +const ast = acorn.parse("", {ecmaVersion: "latest"}) + +const v = () => {} + +/* +Functions who accept visitor objects or "walker objects" should allow both Node +type names and Aggregate type names in the object. + */ +expectAssignable([ast, + {Super: v, Class: v}, + {Super: v, Class: v} +]) + +expectAssignable([ast, + {Super: v, Class: v}, + {Super: v, Class: v} +]) + +expectAssignable([ast, null, + {Super: v, Class: v}, + {Super: v, Class: v} +]) + +expectAssignable([ + {Super: v, Class: v}, + {Super: v, Class: v} +]) + +expectAssignable([ast, v, + {Super: v, Class: v} +]) + +expectAssignable([ast, v, + {Super: v, Class: v} +]) + +expectAssignable([ast, null, null, "Super", + {Super: v, Class: v} +]) + +expectAssignable([ast, null, "Super", + {Super: v, Class: v} +]) + +expectAssignable([ast, null, "Super", + {Super: v, Class: v} +]) + +expectAssignable([ast, null, "Super", + {Super: v, Class: v} +]) + +/* +Functions who accept visitor objects or "walker objects" should not allow +additional properties who are not valid Node/Aggregate type names + */ +expectNotAssignable([ast, {NotANode: v}]) +expectNotAssignable([ast, {}, {NotANode: v}]) + +expectNotAssignable([ast, {NotANode: v}]) +expectNotAssignable([ast, {}, {NotANode: v}]) + +expectNotAssignable([ast, null, {NotANode: v}]) +expectNotAssignable([ast, null, {}, {NotANode: v}]) + +expectNotAssignable([{NotANode: v}]) +expectNotAssignable([{}, {NotANode: v}]) + +expectNotAssignable([ast, v, {NotANode: v}]) + +expectNotAssignable([ast, v, {NotANode: v}]) + +expectNotAssignable([ast, null, null, "Super", + {NotANode: v} +]) + +expectNotAssignable([ast, null, "Super", + {NotANode: v} +]) + +expectNotAssignable([ast, null, "Super", + {NotANode: v} +]) + +expectNotAssignable([ast, null, "Super", + {NotANode: v} +]) + +/* +When we supply a visitor function to a walker function, and the walker function +calls us with a given Node, the Node should be of the type we specified as the +visitor function name. + +If our visitor function is for an Aggregate, the Node we receive should be a +member of a discriminated union, who allows us to determine the actual type +of the Node. + */ +walk.simple(ast, { + Super: (node) => { expectType(node) }, + Pattern: (node) => { + expectType(node) + if (node.type === "Identifier") { expectType(node) } + } +}) + +walk.ancestor(ast, { + Super: (node) => { expectType(node) }, + Pattern: (node) => { + expectType(node) + if (node.type === "Identifier") { expectType(node) } + } +}) + +walk.recursive(ast, null, { + Super: (node) => { expectType(node) }, + Pattern: (node) => { + expectType(node) + if (node.type === "Identifier") { expectType(node) } + } +}) + +/* +Whenever we receive a Node as a parameter to a callback, it should be a member +of a discriminated union, who allows us to determine the actual type of the Node. + */ +walk.ancestor(ast, { + Super: (_node, _state, ancestors) => { + if (ancestors[0].type === "Super") { expectType(ancestors[0]) } + }, +}) + +walk.full(ast, (node) => { + if (node.type === "Super") { expectType(node) } +}) + +walk.fullAncestor(ast, (node, _, ancestors) => { + if (node.type === "Super") { expectType(node) } + if (ancestors[0].type === "Super") { expectType(ancestors[0]) } +}) + +walk.findNodeAt(ast, null, null, (_, node) => { + if (node.type === "Super") { expectType(node) } + return true; +}) + +/* +Whenever we receive a type name as a parameter to a callback, it should be a +valid Node/Aggregate type name + */ +walk.full(ast, (_node, _state, type) => { + expectAssignable("Super") + expectAssignable("Pattern") + expectNotAssignable("NotANode") +}) + +walk.fullAncestor(ast, (_node, _state, _ancestors, type) => { + expectAssignable("Super") + expectAssignable("Pattern") + expectNotAssignable("NotANode") +}) + +walk.findNodeAt(ast, null, null, (type) => { + expectAssignable("Super") + expectAssignable("Pattern") + expectNotAssignable("NotANode") + return true; +}) + +/* +findNodeAt should reject an input string "type" who is not a valid +Node/Aggregate type name + */ +expectError(walk.findNodeAt(ast, null, null, "NotANode")) +expectError(walk.findNodeAround(ast, null, "NotANode")) + +/* +findNodeAt should return a Node who's a member of a discriminated union, who +allows us to determine the actual type of the Node. + */ +const found = walk.findNodeAt(ast, null, null, () => true) +if (found?.node.type === "Literal") { expectType(found?.node) } diff --git a/package.json b/package.json index 7be2a52a..62da5b93 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "eslint-plugin-n": "^16.6.2", "rollup": "^3.29.4", "test262": "git+https://github.com/tc39/test262.git#867ca540d6cc991a82b41a3b7f9ccb9e93efe803", - "test262-parser-runner": "^0.5.0" + "test262-parser-runner": "^0.5.0", + "tsd": "^0.31.2" } }