Fixed declaration emit crash related to enum entity name expressions #58786
Conversation
typescript-bot
added
the
For Uncommitted Bug
| } | ||
| 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
| 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! |
|
@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 <mateuszburzynski@gmail.com>
fixes #58781
cc @weswigham @dragomirtitian