[wontfix] fix(checker): Allow element access expressions in computed property names if argument is literal #62968
[wontfix] fix(checker): Allow element access expressions in computed property names if argument is literal #62968kairosci wants to merge 11 commits intomicrosoft:mainfrom
Conversation
c7b8d77 to
3784ace
Compare
…ames if argument is literal
3784ace to
e813bb0
Compare
99b7bff to
91424d8
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #25083 by allowing element access expressions (e.g., Enum['key']) as computed property names in type literals when the argument is a literal. Previously, only dot notation (e.g., Enum.key) was accepted for enum members in computed property names.
Changes:
- Modified
isLateBindableASTto use a new helperisLateBindableExpressionthat recursively validates element access chains with literal arguments - Updated utility functions to handle signed numeric literals in computed property names
- Added test case covering enum keys accessed via bracket notation in type literals
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/cases/compiler/enumKeysInTypeLiteral.ts |
New test case demonstrating enum element access with bracket notation in type literals |
tests/baselines/reference/enumKeysInTypeLiteral.* |
Baseline files for the new test showing expected types, symbols, and JS output |
tests/baselines/reference/isolatedDeclarationLazySymbols.* |
Updated baselines reflecting improved type inference for element access expressions |
src/compiler/checker.ts |
Added isLateBindableExpression helper and modified isLateBindableAST to handle element access chains |
src/compiler/utilities.ts |
Updated isComputedNonLiteralName and tryGetTextOfPropertyName to handle signed numeric literals; formatting improvements |
Fixes the bug identified by Copilot AI review where isLateBindableAST was incorrectly passing only node.argumentExpression instead of the entire node to isLateBindableExpression. This ensures proper validation of the full element access chain. The fix makes the behavior more strict: only syntactic literals are accepted in element access expressions (e.g., Type['3x14']), not variables with literal types (e.g., Type[variable]).
| @@ -3179,7 +3187,7 @@ export function getThisContainer(node: Node, includeArrowFunctions: boolean, inc | |||
| if (!includeArrowFunctions) { | |||
| continue; | |||
| } | |||
| // falls through | |||
| // falls through | |||
|
|
|||
| case SyntaxKind.FunctionDeclaration: | |||
| case SyntaxKind.FunctionExpression: | |||
| @@ -3304,7 +3312,7 @@ export function getSuperContainer(node: Node, stopOnFunctions: boolean) { | |||
| if (!stopOnFunctions) { | |||
| continue; | |||
| } | |||
| // falls through | |||
| // falls through | |||
|
|
|||
| case SyntaxKind.PropertyDeclaration: | |||
| case SyntaxKind.PropertySignature: | |||
| @@ -3640,7 +3648,7 @@ export function isExpressionNode(node: Node): boolean { | |||
| if (node.parent.kind === SyntaxKind.TypeQuery || isJSDocLinkLike(node.parent) || isJSDocNameReference(node.parent) || isJSDocMemberName(node.parent) || isJSXTagName(node)) { | |||
| return true; | |||
| } | |||
| // falls through | |||
| // falls through | |||
|
|
|||
| case SyntaxKind.NumericLiteral: | |||
| case SyntaxKind.BigIntLiteral: | |||
| @@ -5036,7 +5044,7 @@ export function getDeclarationFromName(name: Node): Declaration | undefined { | |||
| case SyntaxKind.NoSubstitutionTemplateLiteral: | |||
| case SyntaxKind.NumericLiteral: | |||
| if (isComputedPropertyName(parent)) return parent.parent; | |||
| // falls through | |||
| // falls through | |||
| case SyntaxKind.Identifier: | |||
| if (isDeclaration(parent)) { | |||
| return parent.name === name ? parent : undefined; | |||
| @@ -5281,7 +5289,7 @@ export function getFunctionFlags(node: SignatureDeclaration | undefined): Functi | |||
| if (node.asteriskToken) { | |||
| flags |= FunctionFlags.Generator; | |||
| } | |||
| // falls through | |||
| // falls through | |||
|
|
|||
| case SyntaxKind.ArrowFunction: | |||
| if (hasSyntacticModifier(node, ModifierFlags.Async)) { | |||
| @@ -8393,7 +8401,7 @@ export function getLeftmostExpression(node: Expression, stopAtCallExpressions: b | |||
| if (stopAtCallExpressions) { | |||
| return node; | |||
| } | |||
| // falls through | |||
| // falls through | |||
| case SyntaxKind.AsExpression: | |||
| case SyntaxKind.ElementAccessExpression: | |||
| case SyntaxKind.PropertyAccessExpression: | |||
| @@ -11551,7 +11559,7 @@ export function createNameResolver({ | |||
| switch (location.kind) { | |||
| case SyntaxKind.SourceFile: | |||
| if (!isExternalOrCommonJsModule(location as SourceFile)) break; | |||
| // falls through | |||
| // falls through | |||
| case SyntaxKind.ModuleDeclaration: | |||
| const moduleExports = getSymbolOfDeclaration(location as SourceFile | ModuleDeclaration)?.exports || emptySymbols; | |||
| if (location.kind === SyntaxKind.SourceFile || (isModuleDeclaration(location) && location.flags & NodeFlags.Ambient && !isGlobalScopeAugmentation(location))) { | |||
| @@ -11697,7 +11705,7 @@ export function createNameResolver({ | |||
| if (getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2015) { | |||
| break; | |||
| } | |||
| // falls through | |||
| // falls through | |||
There was a problem hiding this comment.
The fall-through comment indentation changes in this file appear to be unrelated to the main fix for enum keys in type literals. While the new formatting is correct, these changes should ideally be in a separate commit or PR focused on code style/formatting cleanup.
If these changes are intentional cleanup, they should be mentioned in the PR description. If they were accidental changes from an auto-formatter, consider reverting them to keep this PR focused on the bug fix.
| function isLateBindableExpression(expr: Expression): boolean { | ||
| while (isElementAccessExpression(expr)) { | ||
| const argument = skipParentheses(expr.argumentExpression); | ||
| if (!isStringOrNumericLiteralLike(argument) && !isSignedNumericLiteral(argument)) return false; |
There was a problem hiding this comment.
The check in the while loop is too restrictive. It should also allow EntityNameExpression (identifier or property access) as arguments, not just literals.
The current code only accepts string/numeric literals or signed numeric literals, which breaks late-bound property assignments like foo[_private] = "ok" where _private is an identifier variable.
The condition should be:
if (!isStringOrNumericLiteralLike(argument) &&
!isSignedNumericLiteral(argument) &&
!isEntityNameExpression(argument)) {
return false;
}
This allows both the new use case (Enum['key'] with literal arguments) and the existing use case (foo[symbol] with identifier arguments).
|
With 6.0 out as the final release vehicle for this codebase, we're closing all PRs that don't fit the merge criteria for post-6.0 patches. If you think this was a mistake and this PR fits the post-6.0 patch criteria, please post to the 6.0 iteration issue with details (specifically, which PR and which patch criteria it satisfies). Next steps for PRs:
|
Fixes #25083.
This PR relaxes the check for computed property names in type literals (specifically in
isLateBindableAST) to allowElementAccessExpressionwhen the argument is a static literal.Changes
Checker (
src/compiler/checker.ts)isLateBindableASTto use a new helperisLateBindableExpression.isLateBindableExpressionrecursively checks the expression and allows:Enum['key'])Enum[-1])Enum[('key')])Utilities (
src/compiler/utilities.ts)isComputedNonLiteralNameto also check forisSignedNumericLiteraltryGetTextOfPropertyNameto handle signed numeric literals inComputedPropertyName, aligning withgetPropertyNameForPropertyNameNodeExample
Verified with new test case
tests/cases/compiler/enumKeysInTypeLiteral.tscovering these scenarios.