Fixed declaration emit crash related to enum entity name expressions#58786
Fixed declaration emit crash related to enum entity name expressions#58786weswigham merged 6 commits intomicrosoft:mainfrom
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| } | ||
| else { | ||
| const evaluated = evaluateEntityNameExpression(node.expression); | ||
| const literalNode = typeof evaluated.value === 'string' ? factory.createStringLiteral(evaluated.value, /*isSingleQuote*/ undefined) : |
There was a problem hiding this comment.
creating those objects is wasteful here but it feels like it keeps the code cleaner, I can change this if you request that change
| literal = computedPropertyNameType.literal; | ||
| } | ||
| else { | ||
| const evaluated = evaluateEntityNameExpression(node.expression); |
There was a problem hiding this comment.
the result of this matches the 5.4 behavior, that's why I've reached for evaluating this but perhaps there are reasons why this is wrong
| @@ -8856,8 +8856,18 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| else { | |||
| const type = getWidenedType(getRegularTypeOfExpression(node.expression)); | |||
| const computedPropertyNameType = typeToTypeNodeHelper(type, context); | |||
There was a problem hiding this comment.
this can return ImportType in the enum's case, that's the reason why it crashed - the code here assumed that a literal type node would always be returned but that wasn't true
src/compiler/checker.ts
Outdated
| const literalNode = typeof evaluated.value === "string" ? factory.createStringLiteral(evaluated.value, /*isSingleQuote*/ undefined) : | ||
| typeof evaluated.value === "number" ? factory.createNumericLiteral(evaluated.value, /*numericLiteralFlags*/ 0) : | ||
| undefined; | ||
| Debug.assert(literalNode); |
There was a problem hiding this comment.
I don't think this assertion is legit - unless you make assumptions about the computed property name being "correct" in some way, its' expression could resolve to any type (as the issue prompting this showed). Notably, evaluateEntityNameExpression will just return undefined if the entity name doesn't resolve (or resolves to a non-literal), which'll in turn trigger this assertion. In such a case, we probably want to retain the whole computed name as-is, but mark an error?
There was a problem hiding this comment.
I couldn't cause a crash using invalid computed property names but I managed to find a crashing case with symbols. Could you take another look?
tests/baselines/reference/declarationEmitComputedPropertyNameSymbol1.js
Outdated
Show resolved
Hide resolved
|
Not sure how many of these repos have declaration emit, but @typescript-bot test top400 |
|
@andrewbranch Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
No longer incorrectly asserts, has fallback behaviors when each attempt fails to produce something usable here, seems good to me~
|
@typescript-bot cherry-pick this to release-5.5 |
|
Hey, @jakebailey! I've created #58853 for you. |
…e-5.5 (#58853) Co-authored-by: Mateusz Burzyński <[email protected]>
5 participants
fixes #58781
cc @weswigham @dragomirtitian