Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 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
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
60 changes: 55 additions & 5 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ import {
isPrivateIdentifier,
isPrivateIdentifierClassElementDeclaration,
isPropertyAccessExpression,
isPropertyAssignment,
isPropertyDeclaration,
isPropertyNameLiteral,
isRegularExpressionLiteral,
Expand Down Expand Up @@ -443,6 +444,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 +1686,15 @@ function createCompletionEntry(
hasAction = true;
}

if (completionKind === CompletionKind.ObjectPropertyDeclaration && contextToken &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave a comment above here with an example of what you're trying to capture.

findPrecedingToken(contextToken.pos, sourceFile, contextToken)?.kind !== SyntaxKind.CommaToken &&
(isMethodDeclaration(contextToken.parent.parent) || isSpreadAssignment(contextToken.parent) || findAncestor(contextToken.parent, (node: Node) => isPropertyAssignment(node))?.getLastToken() === contextToken ||
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Jun 23, 2023

Choose a reason for hiding this comment

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

  1. You don't need to annotate the type of node, it'll be inferred (edit: but you can also just write isPropertyAssignment directly.
  2. Feed through the sourceFile when using Node methods so that they don't have to walk back up the tree.
Suggested change
(isMethodDeclaration(contextToken.parent.parent) || isSpreadAssignment(contextToken.parent) || findAncestor(contextToken.parent, (node: Node) => isPropertyAssignment(node))?.getLastToken() === contextToken ||
(isMethodDeclaration(contextToken.parent.parent) || isSpreadAssignment(contextToken.parent) || findAncestor(contextToken.parent, node => isPropertyAssignment(node))?.getLastToken(sourceFile) === contextToken ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd also rather you just broke this into 2 nested ifs, where it looks like

if (completionKind === CompletionKind.ObjectPropertyDeclaration && contextToken && findPrecedingToken(contextToken.pos, sourceFile, contextToken)?.kind !== SyntaxKind.CommaToken) {
    if (isMethodDeclaration(contextToken.parent.parent) ||
        isSpreadAssignment(contextToken.parent) ||
        ...) {
            source = CompletionSource.ObjectLiteralMemberWithComma;
            hasAction = true;
    }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait, you don't need node => isPropertyAssignment, just use isPropertyAssignment directly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the spread assignment case should be handled the same as the property assignment case, because the spread assignment can also contain any expression, like this:

const v: I = {
    ...a.b.c.d
}

so I think the condition here should be something like:
findAncestor(contextToken.parent, node => isPropertyAssignment(node) || isSpreadAssignment(node))?.getLastToken()

isShorthandPropertyAssignment(contextToken.parent) && getLineAndCharacterOfPosition(contextToken.getSourceFile(), contextToken.getEnd()).line !== getLineAndCharacterOfPosition(contextToken.getSourceFile(), position).line)) {

source = CompletionSource.ObjectLiteralMemberWithComma;
hasAction = true;
}

if (preferences.includeCompletionsWithClassMemberSnippets &&
preferences.includeCompletionsWithInsertText &&
completionKind === CompletionKind.MemberLike &&
Expand Down Expand Up @@ -2664,7 +2676,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 +2873,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,","));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
tracker => tracker.insertText(sourceFile, contextToken.end,","));
tracker => tracker.insertText(sourceFile, contextToken.end, ","),
);

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 +4184,7 @@ function getCompletionData(
*/
function tryGetObjectLikeCompletionSymbols(): GlobalsSearch | undefined {
const symbolsStartIndex = symbols.length;
const objectLikeContainer = tryGetObjectLikeCompletionContainer(contextToken);
const objectLikeContainer = tryGetObjectLikeCompletionContainer(contextToken, position);
if (!objectLikeContainer) return GlobalsSearch.Continue;

// We're looking up possible property names from contextual/inferred/declared type.
Expand Down Expand Up @@ -4884,7 +4912,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): ObjectLiteralExpression | ObjectBindingPattern | undefined {
if (contextToken) {
const { parent } = contextToken;
switch (contextToken.kind) {
Expand All @@ -4899,8 +4927,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(contextToken.getSourceFile(), contextToken.getEnd()).line !== getLineAndCharacterOfPosition(contextToken.getSourceFile(), position).line))) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pass through the sourceFile here too and reuse the same sourceFile

return contextToken.parent.parent;
}
const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node));
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Jun 23, 2023

Choose a reason for hiding this comment

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

Suggested change
const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node));
const ancestorNode = findAncestor(parent, isPropertyAssignment);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as my comment above regarding spread assignments, I think you can handle them together with property assignment:
const ancestorNode = findAncestor(parent, node => isPropertyAssignment(node) || isSpreadAssignment(node));

if (ancestorNode && ancestorNode.getLastToken() === contextToken && isObjectLiteralExpression(ancestorNode.parent)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (ancestorNode && ancestorNode.getLastToken() === contextToken && isObjectLiteralExpression(ancestorNode.parent)) {
if (ancestorNode?.getLastToken() === contextToken && isObjectLiteralExpression(ancestorNode.parent)) {

return ancestorNode.parent;
}
}
break;
default:
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.parent.parent && isMethodDeclaration(parent.parent) && isObjectLiteralExpression(parent.parent.parent)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (parent.parent && parent.parent.parent && isMethodDeclaration(parent.parent) && isObjectLiteralExpression(parent.parent.parent)) {
if (parent.parent?.parent && isMethodDeclaration(parent.parent) && isObjectLiteralExpression(parent.parent.parent)) {

return parent.parent.parent;
}
const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node));
const ancestorNode = findAncestor(parent, isPropertyAssignment);

if (contextToken.kind !== SyntaxKind.ColonToken && ancestorNode && ancestorNode.getLastToken() === contextToken &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (contextToken.kind !== SyntaxKind.ColonToken && ancestorNode && ancestorNode.getLastToken() === contextToken &&
if (contextToken.kind !== SyntaxKind.ColonToken && ancestorNode?.getLastToken() === contextToken &&

isObjectLiteralExpression(ancestorNode.parent)) {
return ancestorNode.parent;
}
}
}

Expand Down
43 changes: 43 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions1.ts
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,
},
});
48 changes: 48 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions2.ts
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
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
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,
},
});
39 changes: 39 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions5.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/// <reference path="fourslash.ts" />

//// type E = {}
//// type F = string
//// interface I { e: E, f?: F }
//// const i: I = { e: {}
//// /**/
//// };

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

verify.applyCodeActionFromCompletion("", {
name: "f",
description: `Add missing comma for object member completion 'f'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`type E = {}
type F = string
interface I { e: E, f?: F }
const i: I = { e: {},

};`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});
45 changes: 45 additions & 0 deletions tests/cases/fourslash/completionsObjectLiteralExpressions6.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@


/// <reference path="fourslash.ts" />

//// type E = {}
//// type F = string
//// const i= { e: {} };
//// interface I { e: E, f?: F }
//// const k: I = {
//// ["e"]: i
//// /**/
//// }

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

verify.applyCodeActionFromCompletion("", {
name: "f",
description: `Add missing comma for object member completion 'f'.`,
source: completion.CompletionSource.ObjectLiteralMemberWithComma,
newFileContent:
`type E = {}
type F = string
const i= { e: {} };
interface I { e: E, f?: F }
const k: I = {
["e"]: i,

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