Skip to content
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

Fix(52604): Provide Object member completions without comma; insert a comma #52899

Merged
merged 18 commits into from
Jun 27, 2023
Merged

Fix(52604): Provide Object member completions without comma; insert a comma #52899

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7c72784
Add condition to fix without adding comma
navya9singh Feb 16, 2023
fa517aa
Fix for adding comma
navya9singh Feb 20, 2023
6613dca
fixing test and adding checks for other tests
navya9singh Feb 21, 2023
4f88134
removing commented code
navya9singh Feb 21, 2023
586a2e5
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh Feb 21, 2023
3ab385f
Adressing pr comments
navya9singh Feb 23, 2023
abb5c07
fixing auto-imports
navya9singh Feb 23, 2023
c8c1362
fixing lint error
navya9singh Feb 23, 2023
463a435
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh Jun 14, 2023
54456cc
Adressing pr comments and adding test cases
navya9singh Jun 20, 2023
87b09f8
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh Jun 20, 2023
2fff618
adding test cases
navya9singh Jun 23, 2023
b33aef8
Minor changes and adding a test case
navya9singh Jun 24, 2023
731222c
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh Jun 26, 2023
89d178b
Apply suggestions from code review
DanielRosenwasser Jun 26, 2023
790b30c
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh Jun 26, 2023
c477b17
Merge branch 'fix(52604)' of https://github.com/microsoft/TypeScript …
navya9singh Jun 26, 2023
7e861b6
Adding test for spread assignment
navya9singh Jun 27, 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
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7644,6 +7644,10 @@
"category": "Message",
"code": 95186
},
"Add missing comma for object member completion '{0}'.": {
"category": "Message",
"code": 95187
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
77 changes: 72 additions & 5 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ import {
isFunctionLikeDeclaration,
isFunctionLikeKind,
isFunctionTypeNode,
isGetAccessorDeclaration,
isIdentifier,
isIdentifierText,
isImportableFile,
Expand Down Expand Up @@ -217,9 +218,11 @@ import {
isPrivateIdentifier,
isPrivateIdentifierClassElementDeclaration,
isPropertyAccessExpression,
isPropertyAssignment,
isPropertyDeclaration,
isPropertyNameLiteral,
isRegularExpressionLiteral,
isSetAccessorDeclaration,
isShorthandPropertyAssignment,
isSingleOrDoubleQuote,
isSourceFile,
Expand Down Expand Up @@ -443,6 +446,8 @@ export enum CompletionSource {
ObjectLiteralMethodSnippet = "ObjectLiteralMethodSnippet/",
/** Case completions for switch statements */
SwitchCases = "SwitchCases/",
/** Completions for an Object literal expression */
ObjectLiteralMemberWithComma = "ObjectLiteralMemberWithComma/",
}

/** @internal */
Expand Down Expand Up @@ -1683,6 +1688,30 @@ function createCompletionEntry(
hasAction = true;
}

// Provide object member completions when missing commas, and insert missing commas.
// For example:
//
// interface I {
// a: string;
// b: number
// }
//
// const cc: I = { a: "red" | }
//
// Completion should add a comma after "red" and provide completions for b
if (completionKind === CompletionKind.ObjectPropertyDeclaration && contextToken && findPrecedingToken(contextToken.pos, sourceFile, contextToken)?.kind !== SyntaxKind.CommaToken) {
if (isMethodDeclaration(contextToken.parent.parent) ||
isGetAccessorDeclaration(contextToken.parent.parent) ||
isSetAccessorDeclaration(contextToken.parent.parent) ||
isSpreadAssignment(contextToken.parent) ||
findAncestor(contextToken.parent, isPropertyAssignment)?.getLastToken() === contextToken ||
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
isShorthandPropertyAssignment(contextToken.parent) && getLineAndCharacterOfPosition(sourceFile, contextToken.getEnd()).line !== getLineAndCharacterOfPosition(sourceFile, position).line) {

source = CompletionSource.ObjectLiteralMemberWithComma;
hasAction = true;
}
}

if (preferences.includeCompletionsWithClassMemberSnippets &&
preferences.includeCompletionsWithInsertText &&
completionKind === CompletionKind.MemberLike &&
Expand Down Expand Up @@ -2664,7 +2693,8 @@ function getSymbolCompletionFromEntryId(
return info && info.name === entryId.name && (
entryId.source === CompletionSource.ClassMemberSnippet && symbol.flags & SymbolFlags.ClassMember
|| entryId.source === CompletionSource.ObjectLiteralMethodSnippet && symbol.flags & (SymbolFlags.Property | SymbolFlags.Method)
|| getSourceFromOrigin(origin) === entryId.source)
|| getSourceFromOrigin(origin) === entryId.source
|| entryId.source === CompletionSource.ObjectLiteralMemberWithComma)
? { type: "symbol" as const, symbol, location, origin, contextToken, previousToken, isJsxInitializer, isTypeOnlyLocation }
: undefined;
}) || { type: "none" };
Expand Down Expand Up @@ -2860,6 +2890,21 @@ function getCompletionEntryCodeActionsAndSourceDisplay(
return { codeActions: [codeAction], sourceDisplay: undefined };
}

if (source === CompletionSource.ObjectLiteralMemberWithComma && contextToken) {
const changes = textChanges.ChangeTracker.with(
{ host, formatContext, preferences },
tracker => tracker.insertText(sourceFile, contextToken.end, ","));
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
if (changes) {
return {
sourceDisplay: undefined,
codeActions: [{
changes,
description: diagnosticToString([Diagnostics.Add_missing_comma_for_object_member_completion_0, name]),
}],
};
}
}

if (!origin || !(originIsExport(origin) || originIsResolvedExport(origin))) {
return { codeActions: undefined, sourceDisplay: undefined };
}
Expand Down Expand Up @@ -4156,7 +4201,7 @@ function getCompletionData(
*/
function tryGetObjectLikeCompletionSymbols(): GlobalsSearch | undefined {
const symbolsStartIndex = symbols.length;
const objectLikeContainer = tryGetObjectLikeCompletionContainer(contextToken);
const objectLikeContainer = tryGetObjectLikeCompletionContainer(contextToken, position, sourceFile);
if (!objectLikeContainer) return GlobalsSearch.Continue;

// We're looking up possible property names from contextual/inferred/declared type.
Expand Down Expand Up @@ -4884,7 +4929,7 @@ function getCompletionData(
* Returns the immediate owning object literal or binding pattern of a context token,
* on the condition that one exists and that the context implies completion should be given.
*/
function tryGetObjectLikeCompletionContainer(contextToken: Node | undefined): ObjectLiteralExpression | ObjectBindingPattern | undefined {
function tryGetObjectLikeCompletionContainer(contextToken: Node | undefined, position: number, sourceFile: SourceFile): ObjectLiteralExpression | ObjectBindingPattern | undefined {
if (contextToken) {
const { parent } = contextToken;
switch (contextToken.kind) {
Expand All @@ -4899,8 +4944,30 @@ function tryGetObjectLikeCompletionContainer(contextToken: Node | undefined): Ob
case SyntaxKind.AsyncKeyword:
return tryCast(parent.parent, isObjectLiteralExpression);
case SyntaxKind.Identifier:
return (contextToken as Identifier).text === "async" && isShorthandPropertyAssignment(contextToken.parent)
? contextToken.parent.parent : undefined;
if ((contextToken as Identifier).text === "async" && isShorthandPropertyAssignment(contextToken.parent)) {
return contextToken.parent.parent;
}
else {
if (isObjectLiteralExpression(contextToken.parent.parent) &&
(isSpreadAssignment(contextToken.parent) || isShorthandPropertyAssignment(contextToken.parent) &&
(getLineAndCharacterOfPosition(sourceFile, contextToken.getEnd()).line !== getLineAndCharacterOfPosition(sourceFile, position).line))) {
return contextToken.parent.parent;
}
const ancestorNode = findAncestor(parent, isPropertyAssignment);
if (ancestorNode?.getLastToken() === contextToken && isObjectLiteralExpression(ancestorNode.parent)) {
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
return ancestorNode.parent;
}
}
break;
default:
Copy link
Member

Choose a reason for hiding this comment

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

So, I've been thinking about the difference between contextToken and previousToken and location, and when to use each one, and I found another case that doesn't seem to be working:

interface A {
    a: string;
    b: number;
    foo(): void;
}

const b = 3;
const a: A = {
    foo() {

    },
    b: b
    /**/
}

I have a feeling that we might want to be using previousToken instead of contextToken, but I might be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is not a new test case, it's the same as this one I previously commented:

const k: I = {
    ["e"]: i
    /**/
}

The relevant factor seems to be that there is an identifier in the right side of the property assignment.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC,

foo |  // contextToken: foo, previousToken: foo
foo b| // contextToken: foo, previousToken: b

The idea is in most places, the completions should be the same whether or not you’ve already started typing something at the current location, so contextToken usually provides a stable token between those two similar cases, where previousToken changes. But I could be oversimplifying that, and the typical logic of “just use contextToken” may not apply when checking to see if there’s a comma. But there should definitely be tests both where the beginning of the expected completion has already been typed and where nothing has been typed yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test cases to check when the beginning of the expected completion is typed.

if (parent.parent?.parent && (isMethodDeclaration(parent.parent) || isGetAccessorDeclaration(parent.parent) || isSetAccessorDeclaration(parent.parent)) && isObjectLiteralExpression(parent.parent.parent)) {
return parent.parent.parent;
}
const ancestorNode = findAncestor(parent, isPropertyAssignment);
if (contextToken.kind !== SyntaxKind.ColonToken && ancestorNode?.getLastToken() === contextToken &&
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
isObjectLiteralExpression(ancestorNode.parent)) {
return ancestorNode.parent;
}
}
}

Expand Down
43 changes: 43 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/// <reference path="fourslash.ts" />
//// interface ColorPalette {
//// primary?: string;
//// secondary?: string;
//// }

//// let colors: ColorPalette = {
//// primary: "red"
//// /**/
//// };

verify.completions({
marker: "",
includes: [{
name: "secondary",
sortText: completion.SortText.OptionalMember,
hasAction: true,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});

verify.applyCodeActionFromCompletion("", {
name: "secondary",
description: `Add missing comma for object member completion 'secondary'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`interface ColorPalette {
primary?: string;
secondary?: string;
}
let colors: ColorPalette = {
primary: "red",

};`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});
47 changes: 47 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions10.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/// <reference path="fourslash.ts" />
//// interface TTTT {
//// aaa: string,
//// bbb?: number
//// }
//// const uuu: TTTT = {
//// get aaa() {
//// return ""
//// }
//// /**/
//// }

verify.completions({
marker: "",
includes: [{
name: "bbb",
sortText: completion.SortText.OptionalMember,
hasAction: true,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});

verify.applyCodeActionFromCompletion("", {
name: "bbb",
description: `Add missing comma for object member completion 'bbb'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`interface TTTT {
aaa: string,
bbb?: number
}
const uuu: TTTT = {
get aaa() {
return ""
},

}`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});

48 changes: 48 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/// <reference path="fourslash.ts" />
//// interface ColorPalette {
//// primary?: string;
//// secondary?: string;
//// }

//// interface I {
//// color: ColorPalette;
//// }

//// const a: I = {
//// color: {primary: "red" /**/}
//// }

verify.completions({
marker: "",
includes: [{
name: "secondary",
sortText: completion.SortText.OptionalMember,
hasAction: true,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
}
});

verify.applyCodeActionFromCompletion("", {
name: "secondary",
description: `Add missing comma for object member completion 'secondary'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`interface ColorPalette {
primary?: string;
secondary?: string;
}
interface I {
color: ColorPalette;
}
const a: I = {
color: {primary: "red", }
}`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});
45 changes: 45 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/// <reference path="fourslash.ts" />
////interface T {
//// aaa?: string;
//// foo(): void;
//// }
//// const obj: T = {
//// foo() {
//
//// }
//// /**/
//// }

verify.completions({
marker: "",
includes: [{
name: "aaa",
sortText: completion.SortText.OptionalMember,
hasAction: true,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});

verify.applyCodeActionFromCompletion("", {
name: "aaa",
description: `Add missing comma for object member completion 'aaa'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`interface T {
aaa?: string;
foo(): void;
}
const obj: T = {
foo() {
},

}`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});
42 changes: 42 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions4.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/// <reference path="fourslash.ts" />
////interface T {
//// aaa: number;
//// bbb?: number;
//// }
//// const obj: T = {
//// aaa: 1 * (2 + 3)
//// /**/
//// }

verify.completions({
marker: "",
includes: [{
name: "bbb",
sortText: completion.SortText.OptionalMember,
hasAction: true,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});

verify.applyCodeActionFromCompletion("", {
name: "bbb",
description: `Add missing comma for object member completion 'bbb'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`interface T {
aaa: number;
bbb?: number;
}
const obj: T = {
aaa: 1 * (2 + 3),

}`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});
Loading