Skip to content

fix(46402): Add quick fix support for all valid strings for computed properties #46716

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

Merged
merged 1 commit into from
Jan 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 3 additions & 33 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,7 @@ namespace ts {
const location = getParseTreeNode(locationIn);
return location ? getTypeOfSymbolAtLocation(symbol, location) : errorType;
},
getTypeOfSymbol,
getSymbolsOfParameterPropertyDeclaration: (parameterIn, parameterName) => {
const parameter = getParseTreeNode(parameterIn, isParameter);
if (parameter === undefined) return Debug.fail("Cannot get symbols of a synthetic parameter that cannot be resolved to a parse-tree node.");
Expand Down Expand Up @@ -6241,7 +6242,7 @@ namespace ts {
}
const rawName = unescapeLeadingUnderscores(symbol.escapedName);
const stringNamed = !!length(symbol.declarations) && every(symbol.declarations, isStringNamed);
return createPropertyNameNodeForIdentifierOrLiteral(rawName, stringNamed, singleQuote);
return createPropertyNameNodeForIdentifierOrLiteral(rawName, getEmitScriptTarget(compilerOptions), singleQuote, stringNamed);
}

// See getNameForSymbolFromNameType for a stringy equivalent
Expand All @@ -6256,20 +6257,14 @@ namespace ts {
if (isNumericLiteralName(name) && startsWith(name, "-")) {
return factory.createComputedPropertyName(factory.createNumericLiteral(+name));
}
return createPropertyNameNodeForIdentifierOrLiteral(name);
return createPropertyNameNodeForIdentifierOrLiteral(name, getEmitScriptTarget(compilerOptions));
}
if (nameType.flags & TypeFlags.UniqueESSymbol) {
return factory.createComputedPropertyName(symbolToExpression((nameType as UniqueESSymbolType).symbol, context, SymbolFlags.Value));
}
}
}

function createPropertyNameNodeForIdentifierOrLiteral(name: string, stringNamed?: boolean, singleQuote?: boolean) {
return isIdentifierText(name, getEmitScriptTarget(compilerOptions)) ? factory.createIdentifier(name) :
!stringNamed && isNumericLiteralName(name) && +name >= 0 ? factory.createNumericLiteral(+name) :
factory.createStringLiteral(name, !!singleQuote);
}

function cloneNodeBuilderContext(context: NodeBuilderContext): NodeBuilderContext {
const initial: NodeBuilderContext = { ...context };
// Make type parameters created within this context not consume the name outside this context
Expand Down Expand Up @@ -26950,31 +26945,6 @@ namespace ts {
return isTypeAssignableToKind(checkComputedPropertyName(name), TypeFlags.NumberLike);
}

function isNumericLiteralName(name: string | __String) {
// The intent of numeric names is that
// - they are names with text in a numeric form, and that
// - setting properties/indexing with them is always equivalent to doing so with the numeric literal 'numLit',
// acquired by applying the abstract 'ToNumber' operation on the name's text.
//
// The subtlety is in the latter portion, as we cannot reliably say that anything that looks like a numeric literal is a numeric name.
// In fact, it is the case that the text of the name must be equal to 'ToString(numLit)' for this to hold.
//
// Consider the property name '"0xF00D"'. When one indexes with '0xF00D', they are actually indexing with the value of 'ToString(0xF00D)'
// according to the ECMAScript specification, so it is actually as if the user indexed with the string '"61453"'.
// Thus, the text of all numeric literals equivalent to '61543' such as '0xF00D', '0xf00D', '0170015', etc. are not valid numeric names
// because their 'ToString' representation is not equal to their original text.
// This is motivated by ECMA-262 sections 9.3.1, 9.8.1, 11.1.5, and 11.2.1.
//
// Here, we test whether 'ToString(ToNumber(name))' is exactly equal to 'name'.
// The '+' prefix operator is equivalent here to applying the abstract ToNumber operation.
// Applying the 'toString()' method on a number gives us the abstract ToString operation on a number.
//
// Note that this accepts the values 'Infinity', '-Infinity', and 'NaN', and that this is intentional.
// This is desired behavior, because when indexing with them as numeric entities, you are indexing
// with the strings '"Infinity"', '"-Infinity"', and '"NaN"' respectively.
return (+name).toString() === name;
}

function checkComputedPropertyName(node: ComputedPropertyName): Type {
const links = getNodeLinks(node.expression);
if (!links.resolvedType) {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4153,6 +4153,7 @@ namespace ts {

export interface TypeChecker {
getTypeOfSymbolAtLocation(symbol: Symbol, node: Node): Type;
/* @internal */ getTypeOfSymbol(symbol: Symbol): Type;
getDeclaredTypeOfSymbol(symbol: Symbol): Type;
getPropertiesOfType(type: Type): Symbol[];
getPropertyOfType(type: Type, propertyName: string): Symbol | undefined;
Expand Down
33 changes: 32 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7441,6 +7441,37 @@ namespace ts {
}

export function escapeSnippetText(text: string): string {
return text.replace(/\$/gm, "\\$");
return text.replace(/\$/gm, () => "\\$");
}

export function isNumericLiteralName(name: string | __String) {
// The intent of numeric names is that
// - they are names with text in a numeric form, and that
// - setting properties/indexing with them is always equivalent to doing so with the numeric literal 'numLit',
// acquired by applying the abstract 'ToNumber' operation on the name's text.
//
// The subtlety is in the latter portion, as we cannot reliably say that anything that looks like a numeric literal is a numeric name.
// In fact, it is the case that the text of the name must be equal to 'ToString(numLit)' for this to hold.
//
// Consider the property name '"0xF00D"'. When one indexes with '0xF00D', they are actually indexing with the value of 'ToString(0xF00D)'
// according to the ECMAScript specification, so it is actually as if the user indexed with the string '"61453"'.
// Thus, the text of all numeric literals equivalent to '61543' such as '0xF00D', '0xf00D', '0170015', etc. are not valid numeric names
// because their 'ToString' representation is not equal to their original text.
// This is motivated by ECMA-262 sections 9.3.1, 9.8.1, 11.1.5, and 11.2.1.
//
// Here, we test whether 'ToString(ToNumber(name))' is exactly equal to 'name'.
// The '+' prefix operator is equivalent here to applying the abstract ToNumber operation.
// Applying the 'toString()' method on a number gives us the abstract ToString operation on a number.
//
// Note that this accepts the values 'Infinity', '-Infinity', and 'NaN', and that this is intentional.
// This is desired behavior, because when indexing with them as numeric entities, you are indexing
// with the strings '"Infinity"', '"-Infinity"', and '"NaN"' respectively.
return (+name).toString() === name;
}

export function createPropertyNameNodeForIdentifierOrLiteral(name: string, target: ScriptTarget, singleQuote?: boolean, stringNamed?: boolean) {
return isIdentifierText(name, target) ? factory.createIdentifier(name) :
!stringNamed && isNumericLiteralName(name) && +name >= 0 ? factory.createNumericLiteral(+name) :
factory.createStringLiteral(name, !!singleQuote);
}
}
20 changes: 13 additions & 7 deletions src/services/codefixes/fixAddMissingMember.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ namespace ts.codefix {
}

if (isIdentifier(token) && isJsxOpeningLikeElement(token.parent)) {
const attributes = getUnmatchedAttributes(checker, token.parent);
const target = getEmitScriptTarget(program.getCompilerOptions());
const attributes = getUnmatchedAttributes(checker, target, token.parent);
if (!length(attributes)) return undefined;
return { kind: InfoKind.JsxAttributes, token, attributes, parentDeclaration: token.parent };
}
Expand Down Expand Up @@ -465,8 +466,12 @@ namespace ts.codefix {
const jsxAttributesNode = info.parentDeclaration.attributes;
const hasSpreadAttribute = some(jsxAttributesNode.properties, isJsxSpreadAttribute);
const attrs = map(info.attributes, attr => {
const value = attr.valueDeclaration ? tryGetValueFromType(context, checker, importAdder, quotePreference, checker.getTypeAtLocation(attr.valueDeclaration)) : createUndefined();
return factory.createJsxAttribute(factory.createIdentifier(attr.name), factory.createJsxExpression(/*dotDotDotToken*/ undefined, value));
const value = tryGetValueFromType(context, checker, importAdder, quotePreference, checker.getTypeOfSymbol(attr));
const name = factory.createIdentifier(attr.name);
const jsxAttribute = factory.createJsxAttribute(name, factory.createJsxExpression(/*dotDotDotToken*/ undefined, value));
// formattingScanner requires the Identifier to have a context for scanning attributes with "-" (data-foo).
setParent(name, jsxAttribute);
return jsxAttribute;
});
const jsxAttributes = factory.createJsxAttributes(hasSpreadAttribute ? [...attrs, ...jsxAttributesNode.properties] : [...jsxAttributesNode.properties, ...attrs]);
const options = { prefix: jsxAttributesNode.pos === jsxAttributesNode.end ? " " : undefined };
Expand All @@ -476,10 +481,11 @@ namespace ts.codefix {
function addObjectLiteralProperties(changes: textChanges.ChangeTracker, context: CodeFixContextBase, info: ObjectLiteralInfo) {
const importAdder = createImportAdder(context.sourceFile, context.program, context.preferences, context.host);
const quotePreference = getQuotePreference(context.sourceFile, context.preferences);
const target = getEmitScriptTarget(context.program.getCompilerOptions());
const checker = context.program.getTypeChecker();
const props = map(info.properties, prop => {
const initializer = prop.valueDeclaration ? tryGetValueFromType(context, checker, importAdder, quotePreference, checker.getTypeAtLocation(prop.valueDeclaration)) : createUndefined();
return factory.createPropertyAssignment(prop.name, initializer);
const initializer = tryGetValueFromType(context, checker, importAdder, quotePreference, checker.getTypeOfSymbol(prop));
return factory.createPropertyAssignment(createPropertyNameNodeForIdentifierOrLiteral(prop.name, target, quotePreference === QuotePreference.Single), initializer);
});
const options = {
leadingTriviaOption: textChanges.LeadingTriviaOption.Exclude,
Expand Down Expand Up @@ -571,7 +577,7 @@ namespace ts.codefix {
((getObjectFlags(type) & ObjectFlags.ObjectLiteral) || (type.symbol && tryCast(singleOrUndefined(type.symbol.declarations), isTypeLiteralNode)));
}

function getUnmatchedAttributes(checker: TypeChecker, source: JsxOpeningLikeElement) {
function getUnmatchedAttributes(checker: TypeChecker, target: ScriptTarget, source: JsxOpeningLikeElement) {
const attrsType = checker.getContextualType(source.attributes);
if (attrsType === undefined) return emptyArray;

Expand All @@ -591,6 +597,6 @@ namespace ts.codefix {
}
}
return filter(targetProps, targetProp =>
!((targetProp.flags & SymbolFlags.Optional || getCheckFlags(targetProp) & CheckFlags.Partial) || seenNames.has(targetProp.escapedName)));
isIdentifierText(targetProp.name, target, LanguageVariant.JSX) && !((targetProp.flags & SymbolFlags.Optional || getCheckFlags(targetProp) & CheckFlags.Partial) || seenNames.has(targetProp.escapedName)));
}
}
18 changes: 18 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingAttributes10.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path='fourslash.ts' />

// @jsx: preserve
// @filename: foo.tsx

////type A = 'a' | 'b' | 'c' | 'd' | 'e';
////type B = 1 | 2 | 3;
////type C = '@' | '!';
////type D = `${A}${Uppercase<A>}${B}${C}`;

////const A = (props: { [K in D]: K }) =>
//// <div {...props}></div>;
////
////const Bar = () =>
//// [|<A></A>|]

verify.not.codeFixAvailable("fixMissingAttributes");

20 changes: 20 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingAttributes8.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// <reference path='fourslash.ts' />

// @jsx: preserve
// @filename: foo.tsx

////type A = 'a' | 'b';
////type B = 'd' | 'c';
////type C = `${A}${B}`;

////const A = (props: { [K in C]: K }) =>
//// <div {...props}></div>;
////
////const Bar = () =>
//// [|<A></A>|]

verify.codeFix({
index: 0,
description: ts.Diagnostics.Add_missing_attributes.message,
newRangeContent: `<A ad={"ad"} ac={"ac"} bd={"bd"} bc={"bc"}></A>`
});
20 changes: 20 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingAttributes9.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// <reference path='fourslash.ts' />

// @jsx: preserve
// @filename: foo.tsx

////type A = 'a-' | 'b-';
////type B = 'd' | 'c';
////type C = `${A}${B}`;

////const A = (props: { [K in C]: K }) =>
//// <div {...props}></div>;
////
////const Bar = () =>
//// [|<A></A>|]

verify.codeFix({
index: 0,
description: ts.Diagnostics.Add_missing_attributes.message,
newRangeContent: `<A a-d={"a-d"} a-c={"a-c"} b-d={"b-d"} b-c={"b-c"}></A>`
});
17 changes: 17 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingProperties15.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts' />

////interface Foo {
//// 1: number;
//// 2: number;
////}
////[|const foo: Foo = {}|]

verify.codeFix({
index: 0,
description: ts.Diagnostics.Add_missing_properties.message,
newRangeContent:
`const foo: Foo = {
1: 0,
2: 0
}`
});
Loading