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

TS5.5-beta injects a random @ts-expect-error comment in the generated .d.ts files #58698

Closed
nicolo-ribaudo opened this issue May 29, 2024 · 3 comments Β· Fixed by #58706
Closed

Comments

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented May 29, 2024

πŸ”Ž Search Terms

comment 5.5 invalid injected inlined

πŸ•— Version & Regression Information

  • This changed in version 5.5-beta

⏯ Playground Link

No response

πŸ’» Code

  1. Clone https://github.com/nicolo-ribaudo/babel/tree/ts-5.5-random-comment
  2. Run yarn to install all deps
  3. Run make generate-tsconfig to generate all the tsconfig.json files
  4. Run yarn tsc -b ./packages/babel-helper-plugin-utils/tsconfig.json

If you ever need to reset the TS state (e.g. delete all .tsbuildinfo and .d.ts files), run make clean-dts.

πŸ™ Actual behavior

dts/packages/babel-parser/src/plugins/typescript/index.d.ts, line 540 contains a random @ts-expect-error comment:

        parseExprListItem(this: Parser, allowEmpty?: boolean, refExpressionErrors?: ExpressionErrors | null, allowPlaceholder?: boolean | // @ts-expect-error todo(flow->ts) 0121
        null): N.Expression | null;

That parseExprListItem is inlined there from another file, that doesn't have that comment:
https://github.com/nicolo-ribaudo/babel/blob/e5726bde94899149d60706698a0f0a968208bf60/packages/babel-parser/src/parser/expression.ts#L2711-L2728

Instead, that comment is coming from
https://github.com/nicolo-ribaudo/babel/blob/e5726bde94899149d60706698a0f0a968208bf60/packages/babel-parser/src/plugins/typescript/index.ts#L2637-L2642

πŸ™‚ Expected behavior

The comment shouldn't be there

@nicolo-ribaudo
Copy link
Author

I tried using every-ts bisect, and this regression has been introduced by #58085 (cc @weswigham).

A simple reproduction that doesn't take 20s to run TS is:

  1. Clone https://github.com/nicolo-ribaudo/babel/tree/ts-5.5-random-comment
  2. Run make generate-tsconfig to generate all the tsconfig.json files
  3. Inside ./packages/babel-parser, add the files as defined below.
  4. Inside ./packages/babel-parser, run rm tsconfig.tsbuildinfo && rm -rf ./dts-parser && every-ts tsc -b . and check ./dts-parser/src/plugins/typescript/index.d.ts
packages/babel-parser/tsconfig.json
{
  "extends": "../../tsconfig.base.json",
  "include": ["./src/**/*.ts"],
  "compilerOptions": {
    "rootDir": ".",
    "declarationDir": "./dts-parser"
  }
}
packages/babel-parser/src/globals.d.ts
declare module "@babel/helper-validator-identifier" {
  export function isReservedWord(word: string, inModule: boolean): boolean;
  export function isStrictReservedWord(
    word: string,
    inModule: boolean,
  ): boolean;
  export function isStrictBindOnlyReservedWord(word: string): boolean;
  export function isStrictBindReservedWord(
    word: string,
    inModule: boolean,
  ): boolean;
  export function isKeyword(word: string): boolean;
  export function isIdentifierStart(code: number): boolean;
  export function isIdentifierChar(code: number): boolean;
  export function isIdentifierName(name: string): boolean;
}

declare module "@babel/helper-string-parser" {
  export type StringContentsErrorHandlers = EscapedCharErrorHandlers & {
    unterminated(
      initialPos: number,
      initialLineStart: number,
      initialCurLine: number,
    ): void;
  };
  export function readStringContents(
    type: "single" | "double" | "template",
    input: string,
    pos: number,
    lineStart: number,
    curLine: number,
    errors: StringContentsErrorHandlers,
  ):
    | {
        pos: number;
        str: string;
        firstInvalidLoc: { pos: number; lineStart: number; curLine: number };
        lineStart: number;
        curLine: number;
        containsInvalid?: undefined;
      }
    | {
        pos: number;
        str: string;
        firstInvalidLoc: { pos: number; lineStart: number; curLine: number };
        lineStart: number;
        curLine: number;
        containsInvalid: boolean;
      };
  type EscapedCharErrorHandlers = HexCharErrorHandlers &
    CodePointErrorHandlers & {
      strictNumericEscape(
        pos: number,
        lineStart: number,
        curLine: number,
      ): void;
    };
  type HexCharErrorHandlers = IntErrorHandlers & {
    invalidEscapeSequence(
      pos: number,
      lineStart: number,
      curLine: number,
    ): void;
  };
  export type IntErrorHandlers = {
    numericSeparatorInEscapeSequence(
      pos: number,
      lineStart: number,
      curLine: number,
    ): void;
    unexpectedNumericSeparator(
      pos: number,
      lineStart: number,
      curLine: number,
    ): void;
    invalidDigit(
      pos: number,
      lineStart: number,
      curLine: number,
      radix: number,
    ): boolean;
  };
  export function readInt(
    input: string,
    pos: number,
    lineStart: number,
    curLine: number,
    radix: number,
    len: number | undefined,
    forceLen: boolean,
    allowNumSeparator: boolean | "bail",
    errors: IntErrorHandlers,
    bailOnError: boolean,
  ): {
    n: number;
    pos: number;
  };
  export type CodePointErrorHandlers = HexCharErrorHandlers & {
    invalidCodePoint(pos: number, lineStart: number, curLine: number): void;
  };
  export function readCodePoint(
    input: string,
    pos: number,
    lineStart: number,
    curLine: number,
    throwOnInvalid: boolean,
    errors: CodePointErrorHandlers,
  ): {
    code: number;
    pos: number;
  };
}

@dragomirtitian
Copy link
Contributor

dragomirtitian commented May 29, 2024

Investigating this I found:

  • visitExistingNodeTreeSymbols will visit child nodes. Then we get to:
return result === node ? setTextRange(context, factory.cloneNode(result), node) : result; 
  • If the node is the same, it is cloned and it's position is set to -1 in setTextRange. The problem is sometimes the node is a different instance but it has been updated by the visitor (because it's children might have changed), the visitor will preserve original position of the node on the updated node.
  • This is a problem when we reuse nodes across files, as node emitting considers all nodes that have a !=-1 position as being from the current file and looks for their comment

@dragomirtitian
Copy link
Contributor

Self contained way to reproduce: Workbench

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants