Skip to content

Regression: 3.6 crashes if constructor parameters transformed #33295

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.

Sign up for GitHub

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

Jump to bottom
Closed
evmar opened this issue Sep 7, 2019 · 1 comment · Fixed by #33472
Closed

Regression: 3.6 crashes if constructor parameters transformed #33295

evmar opened this issue Sep 7, 2019 · 1 comment · Fixed by #33472
Labels
Bug A bug in TypeScript Crash For flagging bugs which are compiler or service crashes or unclean exits, rather than bad output Domain: Transforms Relates to the public transform API
Milestone
TypeScript 3.6.4

Comments

@evmar
Copy link
Contributor

evmar commented Sep 7, 2019

TypeScript Version: 3.6.2, and typescript@3.7.0-dev.20190906

Search Terms:
isParameterPropertyDeclaration

Code

Briefly, if a transformer touches the parameters of a constructor via e.g. ts.updateParameter, then the new node loses its parent pointer, which produces the following backtrace once TS runs after the transformer. (The below is 3.6.2 to make the line numbers easier for you to match up, but it also repros on the head version.)

TypeError: Cannot read property 'kind' of undefined
    at isParameterPropertyDeclaration (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:13411:88)
    at Object.filter (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:469:31)
    at transformClassMembers (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70629:20)
    at createClassDeclarationHeadWithDecorators (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70591:27)
    at visitClassDeclaration (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70399:19)
    at visitTypeScript (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70247:28)
    at visitorWorker (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70041:24)
    at sourceElementVisitorWorker (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70066:28)
    at saveStateAndInvoke (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:69979:27)
    at sourceElementVisitor (/home/evanm/projects/ts/36/node_modules/typescript/lib/typescript.js:70051:20)

Here's an example transformer that provokes this crash. Note that it doesn't actually change anything in the AST, it just runs ts.updateParameter with a copy of the input array (using the ... spread) to force ts.updateParameter to not early exit.

function transformer(context: ts.TransformationContext): ts.Transformer<ts.SourceFile> {
  return (sf: ts.SourceFile) => {
    return ts.visitEachChild(sf, visit, context);
  };
  function visit(node: ts.Node): ts.VisitResult<ts.Node> {
    console.log(ts.SyntaxKind[node.kind]);
    switch (node.kind) {
      case ts.SyntaxKind.Constructor:
        const ctor = node as ts.ConstructorDeclaration;
        const params = ctor.parameters.map(param => ts.updateParameter(
          param,
          [...param.decorators!], param.modifiers,
          param.dotDotDotToken, param.name, param.questionToken,
          param.type, param.initializer));
        return ts.updateConstructor(ctor, ctor.decorators, ctor.modifiers,
                                    params, ctor.body);
      default:
        return ts.visitEachChild(node, visit, context);
    }
  }
}

Expected behavior:
This code succeeds under TS3.5.

Actual behavior:
This crashes in 3.6.

Playground Link:

Related Issues:

@evmar
Copy link
Contributor Author

evmar commented Sep 7, 2019

CC @rbuckton

@evmar evmar mentioned this issue Sep 7, 2019
Closed
mprobst added a commit to mprobst/TypeScript that referenced this issue Sep 9, 2019
@mprobst
Handle parentless nodes in isParameterPropertyDeclaration
d3d2895 
Fixes microsoft#33295.

This follows a similar pattern as in microsoft#20314 by requiring an explicit
`parent` parameter. Where possible, it uses the appopriate variable at
the call sites.

In several locations there is no context available though (e.g.
inspecting `valueDeclarations`) and we access `.parent` as the code
previously did. From a cursory inspection this seems correct, these
callpaths originate in phases where there must be a `parent` (i.e. in
checker, binder, etc).

Change-Id: I28e4726777b57237bec776e4001e9e69ac591b11
@mprobst mprobst mentioned this issue Sep 9, 2019
Merged
mprobst added a commit to mprobst/TypeScript that referenced this issue Sep 9, 2019
@mprobst
Handle parentless nodes in isParameterPropertyDeclaration
f272ec2 
Fixes microsoft#33295.

This follows a similar pattern as in microsoft#20314 by requiring an explicit
`parent` parameter. Where possible, it uses the appopriate variable at
the call sites.

In several locations there is no context available though (e.g.
inspecting `valueDeclarations`) and we access `.parent` as the code
previously did. From a cursory inspection this seems correct, these
callpaths originate in phases where there must be a `parent` (i.e. in
checker, binder, etc).

Change-Id: I28e4726777b57237bec776e4001e9e69ac591b11
mprobst added a commit to mprobst/TypeScript that referenced this issue Sep 9, 2019
@mprobst
Handle parentless nodes in isParameterPropertyDeclaration
934947f 
Fixes microsoft#33295.

This follows a similar pattern as in microsoft#20314 by requiring an explicit
`parent` parameter. Where possible, it uses the appopriate variable at
the call sites.

In several locations there is no context available though (e.g.
inspecting `valueDeclarations`) and we access `.parent` as the code
previously did. From a cursory inspection this seems correct, these
callpaths originate in phases where there must be a `parent` (i.e. in
checker, binder, etc).
@evmar evmar mentioned this issue Sep 9, 2019
Closed
mprobst added a commit to angular/tsickle that referenced this issue Sep 10, 2019
@mprobst
Move to a patched TypeScript version.
1d00054 
This is based on github.com/mprobst/TypeScript#v3.6.2-p1, resolved to
commit 96c94d1a61cd26b2649c8cbdd0782426c32b2c27.

This is a temporary workaround for
microsoft/TypeScript#33295.

This fixes (for the time being) #1065.
mprobst added a commit to angular/tsickle that referenced this issue Sep 10, 2019
@mprobst
Upgrade to TypeScript 3.6.2
e9b6f06 
This is using a patched TypeScript version from
github.com/mprobst/TypeScript#v3.6.2-p1, resolved to commit
96c94d1a61cd26b2649c8cbdd0782426c32b2c27.

This is a temporary workaround for
microsoft/TypeScript#33295.

This fixes (for the time being) #1065.
mprobst added a commit to angular/tsickle that referenced this issue Sep 10, 2019
@mprobst
Upgrade to TypeScript 3.6.2
9fb6d0d 
This is using a patched TypeScript version from
github.com/mprobst/TypeScript#v3.6.2-p1, resolved to commit
96c94d1a61cd26b2649c8cbdd0782426c32b2c27.

This is a temporary workaround for
microsoft/TypeScript#33295.

This fixes (for the time being) #1065.
@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Crash For flagging bugs which are compiler or service crashes or unclean exits, rather than bad output Domain: Transforms Relates to the public transform API labels Sep 10, 2019
@Griffork Griffork mentioned this issue Sep 10, 2019
Closed
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.6.4 milestone Sep 13, 2019
mprobst added a commit to mprobst/TypeScript that referenced this issue Sep 17, 2019
@mprobst
Handle parentless nodes in isParameterPropertyDeclaration
e8be5e8 
Fixes microsoft#33295.

This follows a similar pattern as in microsoft#20314 by requiring an explicit
`parent` parameter. Where possible, it uses the appopriate variable at
the call sites.

In several locations there is no context available though (e.g.
inspecting `valueDeclarations`) and we access `.parent` as the code
previously did. From a cursory inspection this seems correct, these
callpaths originate in phases where there must be a `parent` (i.e. in
checker, binder, etc).

Change-Id: I28e4726777b57237bec776e4001e9e69ac591b11
DanielRosenwasser pushed a commit that referenced this issue Sep 17, 2019
@mprobst @DanielRosenwasser
Handle parentless nodes in isParameterPropertyDeclaration
6bb7e5c 
Fixes #33295.

This follows a similar pattern as in #20314 by requiring an explicit
`parent` parameter. Where possible, it uses the appopriate variable at
the call sites.

In several locations there is no context available though (e.g.
inspecting `valueDeclarations`) and we access `.parent` as the code
previously did. From a cursory inspection this seems correct, these
callpaths originate in phases where there must be a `parent` (i.e. in
checker, binder, etc).

Change-Id: I28e4726777b57237bec776e4001e9e69ac591b11
@DanielRosenwasser DanielRosenwasser mentioned this issue Sep 17, 2019
Merged
This was referenced Oct 2, 2019
Closed
Closed
mprobst added a commit to angular/tsickle that referenced this issue Oct 7, 2019
@mprobst
Upgrade to TypeScript 3.6.2
f2faf78 
This is using a patched TypeScript version from
github.com/mprobst/TypeScript#v3.6.2-p1, resolved to commit
96c94d1a61cd26b2649c8cbdd0782426c32b2c27.

This is a temporary workaround for
microsoft/TypeScript#33295.

This fixes (for the time being) #1065.
mprobst added a commit to angular/tsickle that referenced this issue Oct 16, 2019
@mprobst
Upgrade to TypeScript 3.6.2
3a56da1 
This is using a patched TypeScript version from
github.com/mprobst/TypeScript#v3.6.2-p1, resolved to commit
96c94d1a61cd26b2649c8cbdd0782426c32b2c27.

This is a temporary workaround for
microsoft/TypeScript#33295.

This fixes (for the time being) #1065.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug A bug in TypeScript Crash For flagging bugs which are compiler or service crashes or unclean exits, rather than bad output Domain: Transforms Relates to the public transform API
Projects
None yet
Milestone
TypeScript 3.6.4
Development

Successfully merging a pull request may close this issue.

Handle parentless nodes in isParameterPropertyDeclaration microsoft/TypeScript
2 participants
@evmar @DanielRosenwasser