Avoid a crash with @typedef in a script file.#33847
Avoid a crash with @typedef in a script file.#33847weswigham merged 3 commits intomicrosoft:masterfrom
@typedef in a script file.#33847Conversation
|
@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. |
| // 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)) { |
There was a problem hiding this comment.
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 ;-)).
There was a problem hiding this comment.
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?
|
@typescript-bot cherry-pick this to master |
|
I'm not entirely sure about the fix, but I've (hopefully) requested a cherry-pick anyway. |
|
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. |
|
@DanielRosenwasser bummer, didn't apply after all. Should I change this PR to be against |
|
Try retargetting the PR to |
|
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'. |
|
@typescript-bot cherry-pick this to release-3.6 |
|
@weswigham done, though I think @typescript-bot doesn't listen to my commands (and is wrong about the lib update). |
|
@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.
|
Hey @weswigham, I've opened #33888 for you. |
|
All good to merge @weswigham @sandersn? |
sandersn
left a comment
There was a problem hiding this comment.
One important question, plus @weswigham I'd like you to take a look too since you wrote the support for nameless typedefs.
| /** | ||
| * @typedef {string} | ||
| */ | ||
| exports.SomeName; |
There was a problem hiding this comment.
is there a way this typedef can be used? The test should try, if only so we can see the error if it fails.
There was a problem hiding this comment.
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.
6a2bb7b to
92dc69d
Compare
|
@sandersn would you re-review now that I've pushed a different change up, that tackles the issue closer to the source? |
|
@typescript-bot cherry-pick this to release-3.7 i guess now? |
|
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
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
Scripts (as opposed to modules) do not have a symbol object. If a script
contains a
@typdefdefined on a namespace calledexports, TypeScriptcrashes 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.