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

Fix: checkAliasSymbol crash when checking for @deprecated #42971

Merged
merged 2 commits into from Feb 26, 2021

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Feb 25, 2021

It's possible that we shouldn't be creating symbol with no declarations from non-homomorphic mapped types, but for 4.2, the right fix is to make the @deprecated-check in checkAliasSymbol ensure that target.declarations is defined.

Fixes #42957, test case based on @types/http-errors

It's possible that we shouldn't be creating symbol with no declarations
from non-homomorphic mapped types, but for 4.2, the right fix is to make
the @deprecated-check in checkAliasSymbol ensure that
target.declarations is defined.
@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 25, 2021
@sandersn
Copy link
Member Author

@RyanCavanaugh @DanielRosenwasser you are probably interested in this too.

@sandersn
Copy link
Member Author

We've been creating symbols this way since at least 3.0. The right thing to do is to put undefined into Symbol.declarations' type.

@@ -37124,7 +37124,7 @@ namespace ts {
error(node, Diagnostics.Re_exporting_a_type_when_the_isolatedModules_flag_is_provided_requires_using_export_type);
}

if (isImportSpecifier(node) && every(target.declarations, d => !!(getCombinedNodeFlags(d) & NodeFlags.Deprecated))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our every helper doesn't handle undefined? Most of our other array helpers treat undefined as an empty array input. Maybe we should change the helper?

Copy link
Member Author

@sandersn sandersn Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does handle undefined, but every(undefined, _) ==> true, and then addDeprecatedSuggestion crashes on the next line.

This fix is shorter than target.declarations && every(target.declarations, ... and uses the standard utility, so I think it's better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pffffff okay. Yeah.

@sandersn sandersn merged commit 2c5cee5 into master Feb 26, 2021
@sandersn
Copy link
Member Author

@typescript-bot cherry-pick this to 4.2

@typescript-bot
Copy link
Collaborator

Heya @sandersn, I couldn't find the branch '4.2' on Microsoft/TypeScript. You may need to make it and try again.

@sandersn
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 26, 2021

Heya @sandersn, I've started to run the task to cherry-pick this into release-4.2 on this PR at 75449de. You can monitor the build here.

@sandersn sandersn deleted the fix-checkAliasSymbol-crash-on-checking-deprecated branch February 26, 2021 00:30
@typescript-bot
Copy link
Collaborator

Hey @sandersn, I've opened #42972 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Feb 26, 2021
Component commits:
9f9825a Fix: checkAliasSymbol crash when checking for @deprecated
It's possible that we shouldn't be creating symbol with no declarations
from non-homomorphic mapped types, but for 4.2, the right fix is to make
the @deprecated-check in checkAliasSymbol ensure that
target.declarations is defined.

75449de Add bug number and accept baselines
sandersn added a commit that referenced this pull request Feb 26, 2021
Component commits:
9f9825a Fix: checkAliasSymbol crash when checking for @deprecated
It's possible that we shouldn't be creating symbol with no declarations
from non-homomorphic mapped types, but for 4.2, the right fix is to make
the @deprecated-check in checkAliasSymbol ensure that
target.declarations is defined.

75449de Add bug number and accept baselines

Co-authored-by: Nathan Shively-Sanders <nathansa@microsoft.com>
This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'jsDocCache' of undefined crash on 4.2.2
3 participants