Skip to content

Better completion for property access of computed properties #56220

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Better completion for property access of computed properties #56220

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b2b138f
Improving completions for computed properties
PranavSenthilnathan Oct 20, 2023
00303d5
test cases
PranavSenthilnathan Oct 25, 2023
c7cb038
symbol to node
PranavSenthilnathan Oct 25, 2023
cf1bf06
add/remove comments
PranavSenthilnathan Oct 25, 2023
e6eccb9
Merge branch 'main' of https://github.com/microsoft/TypeScript into p…
PranavSenthilnathan Oct 25, 2023
f59cd66
fix tests after merge
PranavSenthilnathan Oct 25, 2023
6ed62ed
assert node
PranavSenthilnathan Oct 25, 2023
825f71b
format
PranavSenthilnathan Oct 26, 2023
27dd761
pr feedback
PranavSenthilnathan Nov 1, 2023
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
173 changes: 117 additions & 56 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ import {
isStringLiteralOrTemplate,
isStringTextContainingNode,
isSyntaxList,
isTransientSymbol,
isTypeKeyword,
isTypeKeywordTokenOrIdentifier,
isTypeLiteralNode,
Expand Down Expand Up @@ -3663,71 +3664,131 @@ function getCompletionData(
}

function addPropertySymbol(symbol: Symbol, insertAwait: boolean, insertQuestionDot: boolean) {
// For a computed property with an accessible name like `Symbol.iterator`,
// we'll add a completion for the *name* `Symbol` instead of for the property.
// If this is e.g. [Symbol.iterator], add a completion for `Symbol`.
// For a computed property `x.y` in an exported namespace `n` that is imported
// in another file as `m`, we can access `y` as `m.n.x.y`. To form this access chain, first
// we follow `x` up it symbol parents until we find a symbol that is accessible from the completion
// location. This gives us a property access chain (`m.n.x`) which we can then combine with the original
// computed property expression (`x.y`) by substitution of `m.n.x` for `x` in `x.y` to get `m.n.x.y`.
// If this fails, we will fall back to the literal value of `y`.

const computedPropertyName = firstDefined(symbol.declarations, decl => tryCast(getNameOfDeclaration(decl), isComputedPropertyName));
if (computedPropertyName) {
const leftMostName = getLeftMostName(computedPropertyName.expression); // The completion is for `Symbol`, not `iterator`.
const nameSymbol = leftMostName && typeChecker.getSymbolAtLocation(leftMostName);
// If this is nested like for `namespace N { export const sym = Symbol(); }`, we'll add the completion for `N`.
const firstAccessibleSymbol = nameSymbol && getFirstSymbolInChain(nameSymbol, contextToken, typeChecker);
const firstAccessibleSymbolId = firstAccessibleSymbol && getSymbolId(firstAccessibleSymbol);
if (firstAccessibleSymbolId && addToSeen(seenPropertySymbols, firstAccessibleSymbolId)) {
const index = symbols.length;
symbols.push(firstAccessibleSymbol);
const moduleSymbol = firstAccessibleSymbol.parent;
if (
!moduleSymbol ||
!isExternalModuleSymbol(moduleSymbol) ||
typeChecker.tryGetMemberInModuleExportsAndProperties(firstAccessibleSymbol.name, moduleSymbol) !== firstAccessibleSymbol
) {
symbolToOriginInfoMap[index] = { kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberNoExport) };
}
else {
const fileName = isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? getSourceFileOfModule(moduleSymbol)?.fileName : undefined;
const { moduleSpecifier } = (importSpecifierResolver ||= codefix.createImportSpecifierResolver(sourceFile, program, host, preferences)).getModuleSpecifierForBestExportInfo(
[{
exportKind: ExportKind.Named,
moduleFileName: fileName,
isFromPackageJson: false,
moduleSymbol,
symbol: firstAccessibleSymbol,
targetFlags: skipAlias(firstAccessibleSymbol, typeChecker).flags,
}],
position,
isValidTypeOnlyAliasUseSite(location),
) || {};

if (moduleSpecifier) {
const origin: SymbolOriginInfoResolvedExport = {
kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberExport),
moduleSymbol,
isDefaultExport: false,
symbolName: firstAccessibleSymbol.name,
exportName: firstAccessibleSymbol.name,
fileName,
moduleSpecifier,
};
symbolToOriginInfoMap[index] = origin;
}
if (!computedPropertyName) {
addLiteralSymbol();
return;
}

const computedPropertyNameExpression = computedPropertyName.expression;
const name = isEntityName(computedPropertyNameExpression)
? computedPropertyNameExpression
: isPropertyAccessExpression(computedPropertyNameExpression)
? computedPropertyNameExpression.name
: undefined;
const nameSymbol = name && typeChecker.getSymbolAtLocation(name);
const nameSymbolId = nameSymbol && getSymbolId(nameSymbol);
if (!nameSymbolId) { // Not a property access or entity name
addLiteralSymbol();
return;
}

if (addToSeen(seenPropertySymbols, nameSymbolId)) {
const leftMostName = getLeftMostName(computedPropertyNameExpression);
const leftMostNameSymbol = leftMostName && typeChecker.getSymbolAtLocation(leftMostName);
const firstAccessibleSymbol = leftMostNameSymbol && getFirstSymbolInChain(leftMostNameSymbol, contextToken, typeChecker);
if (!firstAccessibleSymbol) { // Symbol is not accessible from completion location
addLiteralSymbol();
return;
}

const index = symbols.length;
symbols.push(nameSymbol);
const moduleSymbol = firstAccessibleSymbol.parent;
if (
!moduleSymbol ||
!isExternalModuleSymbol(moduleSymbol) ||
typeChecker.tryGetMemberInModuleExportsAndProperties(firstAccessibleSymbol.name, moduleSymbol) !== firstAccessibleSymbol
) {
// If preferences allow insert text, add completion for [<QualifiedSymbolName>]
const node = preferences.includeCompletionsWithInsertText
? createComputedPropertyAccess(nameSymbol, leftMostNameSymbol, computedPropertyNameExpression)
: undefined;

if (!node) {
// Switch to literal symbol if user doesn't want insert text
symbols[index] = symbol;
} else {
const printer = createPrinter({
removeComments: true,
module: compilerOptions.module,
target: compilerOptions.target,
omitTrailingSemicolon: true,
});
const origin: SymbolOriginInfoComputedPropertyName = {
kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberNoExport) | SymbolOriginInfoKind.ComputedPropertyName,
symbolName: printer.printNode(EmitHint.Unspecified, node, contextToken.getSourceFile())
};
symbolToOriginInfoMap[index] = origin;
}
}
else if (preferences.includeCompletionsWithInsertText) {
if (firstAccessibleSymbolId && seenPropertySymbols.has(firstAccessibleSymbolId)) {
return;
else {
const fileName = isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? getSourceFileOfModule(moduleSymbol)?.fileName : undefined;
const { moduleSpecifier } = (importSpecifierResolver ||= codefix.createImportSpecifierResolver(sourceFile, program, host, preferences)).getModuleSpecifierForBestExportInfo(
[{
exportKind: ExportKind.Named,
moduleFileName: fileName,
isFromPackageJson: false,
moduleSymbol,
symbol: firstAccessibleSymbol,
targetFlags: skipAlias(firstAccessibleSymbol, typeChecker).flags,
}],
position,
isValidTypeOnlyAliasUseSite(location),
) || {};

if (moduleSpecifier) {
const origin: SymbolOriginInfoResolvedExport = {
kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberExport),
moduleSymbol,
isDefaultExport: false,
symbolName: firstAccessibleSymbol.name,
exportName: firstAccessibleSymbol.name,
fileName,
moduleSpecifier,
};
symbolToOriginInfoMap[index] = origin;
}
addSymbolOriginInfo(symbol);
addSymbolSortInfo(symbol);
symbols.push(symbol);
}
}
else {

function addLiteralSymbol() {
addSymbolOriginInfo(symbol);
addSymbolSortInfo(symbol);
symbols.push(symbol);
}

/** Combines a property access expression (from the completion location to the left-most name in the computed property expression)
* and the computed property expression itself to return an expression to access the computed property from the completion location.
* Assumes that the left-most symbol is accessible so there should always be a way to access the computed property symbolically. */
function createComputedPropertyAccess(nameSymbol: Symbol, leftMostNameSymbol: Symbol, computedPropertyNameExpression: Expression) {
let node: Node | undefined;
if (!isTransientSymbol(nameSymbol)) {
node = typeChecker.symbolToEntityName(nameSymbol, /*meaning*/ undefined!, contextToken, NodeBuilderFlags.UseAliasDefinedOutsideCurrentScope);
}
else {
// Object literals assigned as const
Copy link
Member

Choose a reason for hiding this comment

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

What's this case for, exactly? I think I don't understand the comment here and how we know this from knowing that nameSymbol is a transient symbol.

Copy link
Member Author

Choose a reason for hiding this comment

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

This scenario goes into this branch:

const x = { a: "foo" } as const;
const y = { [x.a]: 0 };
y.|

It's because the symbol a is associated with the object and not x so walking up the parent symbols will just go to __object - so symbolToEntityName (the other branch) won't give a full access expression to it.

That being said, I don't know if isTransientSymbol is the right check here - are there transient symbols that won't run into this issue? and are there non-transient symbols that will run into this issue? I can be safe an always choose the else branch (get rid of the whole if).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if all transient symbols run into this problem with symbolToEntityName, but I think I was confused by the comment because it sorta implied that if the symbol is transient, then we have that object literal situation, which I think is not always the case.
@weswigham might know the details of symbolToEntityName and symbolToExpression.

const leftMostNodeAccessExpression = typeChecker.symbolToNode(leftMostNameSymbol, /*meaning*/ undefined!, contextToken, NodeBuilderFlags.UseAliasDefinedOutsideCurrentScope);
type OnlyPropertyAccess = Identifier | (PropertyAccessExpression & { expression: OnlyPropertyAccess; });
node = createPropertyAccess(computedPropertyNameExpression as OnlyPropertyAccess);
function createPropertyAccess(n: OnlyPropertyAccess): Expression {
if (isIdentifier(n)) {
return leftMostNodeAccessExpression! as Expression; //TODO ! and cast
}

return factory.createPropertyAccessExpression(createPropertyAccess(n.expression), n.name);
}
}
return node;
}

function addSymbolSortInfo(symbol: Symbol) {
if (isStaticProperty(symbol)) {
symbolToSortTextMap[getSymbolId(symbol)] = SortText.LocalDeclarationPriority;
Expand Down Expand Up @@ -4535,7 +4596,7 @@ function getCompletionData(
if (declaration && isClassElement(declaration) && declaration.name && isComputedPropertyName(declaration.name)) {
const origin: SymbolOriginInfoComputedPropertyName = {
kind: SymbolOriginInfoKind.ComputedPropertyName,
symbolName: typeChecker.symbolToString(symbol),
symbolName: typeChecker.symbolToString(symbol)
};
symbolToOriginInfoMap[index] = origin;
}
Expand Down Expand Up @@ -5175,7 +5236,7 @@ function getCompletionEntryDisplayNameForSymbol(
case CompletionKind.PropertyAccess:
case CompletionKind.Global: // For a 'this.' completion it will be in a global context, but may have a non-identifier name.
// Don't add a completion for a name starting with a space. See https://github.com/Microsoft/TypeScript/pull/20547
return name.charCodeAt(0) === CharacterCodes.space ? undefined : { name, needsConvertPropertyAccess: true };
return name.charCodeAt(0) === CharacterCodes.space ? undefined : { name, needsConvertPropertyAccess: !originIsComputedPropertyName(origin) };
case CompletionKind.None:
case CompletionKind.String:
return validNameResult;
Expand Down
Loading