Avoid a crash with `@typedef` in a script file. #33847

mprobst · 2019-10-07T12:25:06Z

Scripts (as opposed to modules) do not have a symbol object. If a script
contains a @typdef defined on a namespace called exports, TypeScript
crashes because it attempts to create an exported symbol on the
(non-existent) symbol of the SourceFile.

This change avoids the crash by explicitly checking if the source file
has a symbol object, i.e. whether it is a module.

mprobst · 2019-10-07T12:31:10Z

@DanielRosenwasser I'm a bit unsure whether this PR should go against release-3.6 or master. Let me know and I can re-cherry-pick/re-base, it applies cleanly against both.

mprobst · 2019-10-07T12:31:45Z

src/compiler/binder.ts

@@ -491,7 +491,7 @@ namespace ts {
                //       and this case is specially handled. Module augmentations should only be merged with original module definition
                //       and should never be merged directly with other augmentation, and the latter case would be possible if automatic merge is allowed.
                if (isJSDocTypeAlias(node)) Debug.assert(isInJSFile(node)); // We shouldn't add symbols for JSDoc nodes if not in a JS file.
-                if ((!isAmbientModule(node) && (hasExportModifier || container.flags & NodeFlags.ExportContext)) || isJSDocTypeAlias(node)) {
+                if ((!isAmbientModule(node) && (hasExportModifier || container.flags & NodeFlags.ExportContext)) || (isJSDocTypeAlias(node) && container.symbol)) {


This avoids the crash, but I'm unsure it's the correct fix here. I'd appreciate guidance (or somebody stealing my test and fixing it properly ;-)).

I have a slightly different fix I'd like to push - container.symbol is undefined here because non-module files do not have an associated symbol. The root cause of the bug is attempting to bind the typedef as a module member in a non-module file. Ideally we'd not think the exports.whatever assignment is a module export assignment when the container isn't a module; but doing that's a little tough and maybe not really worth doing. Do you mind if I push my candidate change to your branch?

DanielRosenwasser · 2019-10-07T14:37:09Z

@typescript-bot cherry-pick this to master

DanielRosenwasser · 2019-10-07T14:38:06Z

I'm not entirely sure about the fix, but I've (hopefully) requested a cherry-pick anyway.

typescript-bot · 2019-10-07T14:41:08Z

Hey @DanielRosenwasser, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into master manually.

mprobst · 2019-10-07T17:45:07Z

@DanielRosenwasser bummer, didn't apply after all. Should I change this PR to be against release-3.6 or leave it like this?

weswigham · 2019-10-07T19:55:34Z

Try retargetting the PR to master and let the bot cherry-pick to release-3.6 - I don't think it's setup to be capable of a clean squash and pick from an older release back to master, but the other way should be OK.

typescript-bot · 2019-10-08T14:14:41Z

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src'. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src'.

mprobst · 2019-10-08T14:15:08Z

@typescript-bot cherry-pick this to release-3.6

mprobst · 2019-10-08T14:19:10Z

@weswigham done, though I think @typescript-bot doesn't listen to my commands (and is wrong about the lib update).

weswigham · 2019-10-09T04:58:03Z

@typescript-bot cherry-pick this to release-3.6

Component commits: dda7cd8 Avoid a crash with `@typedef` in a script file. Scripts (as opposed to modules) do not have a symbol object. If a script contains a `@typdef` defined on a namespace called `exports`, TypeScript crashes because it attempts to create an exported symbol on the (non-existent) symbol of the SourceFile. This change avoids the crash by explicitly checking if the source file has a symbol object, i.e. whether it is a module.

typescript-bot · 2019-10-09T05:02:12Z

Hey @weswigham, I've opened #33888 for you.

DanielRosenwasser · 2019-10-09T20:16:57Z

All good to merge @weswigham @sandersn?

sandersn

One important question, plus @weswigham I'd like you to take a look too since you wrote the support for nameless typedefs.

sandersn · 2019-10-15T23:13:30Z

tests/baselines/reference/checkJsdocTypedefOnlySourceFile.js

+/**
+ * @typedef {string}
+ */
+exports.SomeName;


is there a way this typedef can be used? The test should try, if only so we can see the error if it fails.

No, it cannot be used, see the additional commit I pushed. The typedef essentially goes ignored due to the added check for container.symbol (which is false in files that are not modules).

Scripts (as opposed to modules) do not have a symbol object. If a script contains a `@typdef` defined on a namespace called `exports`, TypeScript crashes because it attempts to create an exported symbol on the (non-existent) symbol of the SourceFile. This change avoids the crash by explicitly checking if the source file has a symbol object, i.e. whether it is a module.

weswigham · 2019-10-24T00:00:55Z

@sandersn would you re-review now that I've pushed a different change up, that tackles the issue closer to the source?

weswigham · 2019-10-24T21:04:58Z

@typescript-bot cherry-pick this to release-3.7 i guess now?

typescript-bot · 2019-10-24T21:09:03Z

Hey @weswigham, I've opened #34720 for you.

Component commits: 97dcbd3 Avoid a crash with `@typedef` in a script file. Scripts (as opposed to modules) do not have a symbol object. If a script contains a `@typdef` defined on a namespace called `exports`, TypeScript crashes because it attempts to create an exported symbol on the (non-existent) symbol of the SourceFile. This change avoids the crash by explicitly checking if the source file has a symbol object, i.e. whether it is a module. f9b7d6e Add usage of exports.SomeName typedef. 92dc69d Fix bug at bind site rather than in declare func

mprobst force-pushed the crash-typedef branch from c0facef to 68a5bd5 Compare October 7, 2019 12:25

mprobst changed the base branch from master to release-3.6 October 7, 2019 12:26

mprobst commented Oct 7, 2019

View reviewed changes

DanielRosenwasser requested review from sandersn and weswigham October 7, 2019 14:36

mprobst mentioned this pull request Oct 7, 2019

Typescript 3.6.4 not on npm #33854

Closed

mprobst force-pushed the crash-typedef branch from 68a5bd5 to dda7cd8 Compare October 8, 2019 14:14

mprobst changed the base branch from release-3.6 to master October 8, 2019 14:14

typescript-bot added the lib update PR modifies files in the `lib` folder label Oct 8, 2019

typescript-bot mentioned this pull request Oct 9, 2019

🤖 Cherry-pick PR #33847 into release-3.6 #33888

Closed

DanielRosenwasser removed the lib update PR modifies files in the `lib` folder label Oct 9, 2019

sandersn requested changes Oct 15, 2019

View reviewed changes

mprobst and others added 3 commits October 23, 2019 16:24

Add usage of exports.SomeName typedef.

f9b7d6e

Fix bug at bind site rather than in declare func

92dc69d

weswigham force-pushed the crash-typedef branch from 6a2bb7b to 92dc69d Compare October 24, 2019 00:00

weswigham requested a review from sandersn October 24, 2019 00:01

sandersn approved these changes Oct 24, 2019

View reviewed changes

weswigham merged commit a03227d into microsoft:master Oct 24, 2019

typescript-bot mentioned this pull request Oct 24, 2019

🤖 Cherry-pick PR #33847 into release-3.7 #34720

Merged

Avoid a crash with @typedef in a script file. #33847

Avoid a crash with @typedef in a script file. #33847

Uh oh!

Conversation

mprobst commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprobst commented Oct 7, 2019

Uh oh!

mprobst Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

weswigham Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Oct 7, 2019

Uh oh!

DanielRosenwasser commented Oct 7, 2019

Uh oh!

typescript-bot commented Oct 7, 2019

Uh oh!

mprobst commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Oct 7, 2019

Uh oh!

typescript-bot commented Oct 8, 2019

Uh oh!

mprobst commented Oct 8, 2019

Uh oh!

mprobst commented Oct 8, 2019

Uh oh!

weswigham commented Oct 9, 2019

Uh oh!

typescript-bot commented Oct 9, 2019

Uh oh!

DanielRosenwasser commented Oct 9, 2019

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

mprobst Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

weswigham commented Oct 24, 2019

Uh oh!

weswigham commented Oct 24, 2019

Uh oh!

typescript-bot commented Oct 24, 2019

Uh oh!

Uh oh!

Avoid a crash with `@typedef` in a script file. #33847

Avoid a crash with `@typedef` in a script file. #33847

mprobst commented Oct 7, 2019 •

edited

Loading

mprobst commented Oct 7, 2019 •

edited

Loading