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

ts.Node.getChildren() returns duplicate copies of the same AST nodes in TS 4.3 #44422

Closed
octogonz opened this issue Jun 3, 2021 · 6 comments · Fixed by #44443
Closed

ts.Node.getChildren() returns duplicate copies of the same AST nodes in TS 4.3 #44422

octogonz opened this issue Jun 3, 2021 · 6 comments · Fixed by #44443
Assignees
Labels
Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fix Available A PR has been opened for this issue Help Wanted You can do this

Comments

@octogonz
Copy link

octogonz commented Jun 3, 2021

Bug Report

Starting with TypeScript 4.3, the ts.Node.getChildren() API may return two copies of the same AST node.

🔎 Search Terms

getChildren

🕗 Version & Regression Information

Repros with TypeScript 4.3.2 and 4.4.0-dev.20210602

Does NOT repro with TypeScript 4.2.4

💻 Code

Here is a complete project that reproduces the problem: repro.zip

The code looks like this:

repro.js

const path = require("path");
const ts = require("typescript");

console.log('Start')
try {
  const inputPath = path.join(__dirname, "input.ts");
  const program = ts.createProgram([inputPath], {});
  program.getSemanticDiagnostics();
  const sourceFile = program.getSourceFile(inputPath);
  const docNode = sourceFile.getChildren()[0].getChildren()[0].getChildren()[4].getChildren()[0].getChildren()[0].getChildren()[0];

  for (const child of docNode.getChildren()) {
    console.log(`[pos=${child.pos}, end=${child.end}] ` + child.getText())
  }

  debugger;
} catch (error) {
  console.error(error.stack);
  debugger;
}

It uses the compiler API to parse an input like this:

input.ts

export class TypeReferencesInAedoc {
  /**
   * Returns a value
   * @param arg1 - The input parameter of type {@link TypeReferencesInAedoc}.
   * @returns An object of type {@link TypeReferencesInAedoc}.
   */
  public getValue(arg1: TypeReferencesInAedoc): TypeReferencesInAedoc {
    return this;
  }
}

🙁 Actual behavior

The printed output looks like this:

[pos=73, end=78] param
[pos=79, end=83] arg1
[pos=84, end=114] - The input parameter of type
[pos=114, end=143] {@link TypeReferencesInAedoc}
[pos=143, end=151] .
   *
[pos=84, end=114] - The input parameter of type
[pos=114, end=143] {@link TypeReferencesInAedoc}
[pos=143, end=151] .
   *

The duplicated pos/end nodes are actually the same object instances, which can be verified in the debugger:

docNode.getChildren[3] === docNode.getChildren[6] 
true

🙂 Expected behavior

ts.Node.getChildren() should not return duplicate nodes.

octogonz added a commit to microsoft/rushstack that referenced this issue Jun 3, 2021
@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation labels Jun 3, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.3.3 milestone Jun 3, 2021
@DanielRosenwasser
Copy link
Member

Seems like we might need to back-port this to 4.3.

@DanielRosenwasser DanielRosenwasser added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Jun 3, 2021
@ShuiRuTian ShuiRuTian mentioned this issue Jun 4, 2021
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Jun 4, 2021
@sandersn
Copy link
Member

sandersn commented Jun 4, 2021

I suspect that these nodes are being added both as jsdoc children and as normal children inside the tree-trivia-reconstruction part of the services code.

@sandersn
Copy link
Member

sandersn commented Jun 4, 2021

ts-ast-viewer shows this off interactively, right down to highlighting two items in the tree pane: https://ts-ast-viewer.com/#code/PQKhCgAIUgBAHAhgJ0QW0ig5gRkgFQAsBTSAb1gBsBLAOwGsCBPeYgJWIDNjljaBjYgGcAkrQCCxACYB7fgF8AdFBDBwANxSQAHuCA

For the smaller program:

/**
 * @param arg1 The {@link TypeReferencesInAedoc}.
 */
var x

Note that this doesn't repro with @return or for top-level comments.

@octogonz
Copy link
Author

octogonz commented Jun 4, 2021

ts-ast-viewer shows this off interactively

(In order to see this, you need to change Tree mode: to be node.getChildren(). The ts-ast-viewer.com URL apparently doesn't preserve that state.)

@sandersn
Copy link
Member

sandersn commented Jun 4, 2021

Anyway, it's just a bug in the visitor code for @param/@property:

            case SyntaxKind.JSDocParameterTag:
            case SyntaxKind.JSDocPropertyTag:
                return visitNode(cbNode, (node as JSDocTag).tagName) ||
                    ((node as JSDocPropertyLikeTag).isNameFirst
                        ? visitNode(cbNode, (node as JSDocPropertyLikeTag).name) ||
                            visitNode(cbNode, (node as JSDocPropertyLikeTag).typeExpression) ||
                            (typeof (node as JSDoc).comment === "string" ? undefined : visitNodes(cbNode, cbNodes, (node as JSDoc).comment as NodeArray<JSDocComment> | undefined))
                        : visitNode(cbNode, (node as JSDocPropertyLikeTag).typeExpression) ||
                            visitNode(cbNode, (node as JSDocPropertyLikeTag).name)) ||
                            (typeof (node as JSDoc).comment === "string" ? undefined : visitNodes(cbNode, cbNodes, (node as JSDoc).comment as NodeArray<JSDocComment> | undefined));

This visits the comment twice, and the test we have to make sure the visitor covers everything doesn't check for double visits.

@octogonz
Copy link
Author

octogonz commented Jun 5, 2021

Thanks for getting this fixed!

octogonz added a commit to microsoft/rushstack that referenced this issue Jul 13, 2021
Qjuh pushed a commit to discordjs/rushstack that referenced this issue Nov 7, 2023
Qjuh pushed a commit to discordjs/rushstack that referenced this issue Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fix Available A PR has been opened for this issue Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants