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 5 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
31 changes: 30 additions & 1 deletion src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
Expression,
ExpressionWithTypeArguments,
factory,
FileTextChanges,
filter,
find,
findAncestor,
Expand Down Expand Up @@ -293,6 +294,7 @@ import {
Program,
programContainsModules,
PropertyAccessExpression,
PropertyAssignment,
PropertyDeclaration,
PropertyName,
PropertySignature,
Expand Down Expand Up @@ -1346,7 +1348,7 @@ function createCompletionEntry(
}
}

if (origin?.kind === SymbolOriginInfoKind.TypeOnlyAlias) {
if ((origin?.kind === SymbolOriginInfoKind.TypeOnlyAlias) || (contextToken && contextToken.kind !== SyntaxKind.OpenBraceToken && completionKind === CompletionKind.ObjectPropertyDeclaration && preferences.includeCompletionsWithInsertText)) {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
hasAction = true;
}

Expand Down Expand Up @@ -2464,6 +2466,29 @@ function getCompletionEntryCodeActionsAndSourceDisplay(
return { codeActions: [codeAction], sourceDisplay: undefined };
}

if (contextToken && isObjectLiteralExpression(contextToken.parent.parent) && previousToken?.kind !== SyntaxKind.ColonToken) {
let changes: FileTextChanges[];
if (getLineAndCharacterOfPosition(sourceFile, contextToken.getEnd()).line !== getLineAndCharacterOfPosition(sourceFile, position).line) {
changes = textChanges.ChangeTracker.with(
{ host, formatContext, preferences },
tracker=>tracker.replacePropertyAssignment(sourceFile, contextToken.parent as PropertyAssignment ,contextToken.parent as PropertyAssignment));
}
else {
changes = textChanges.ChangeTracker.with(
{ host, formatContext, preferences },
tracker=>tracker.replacePropertyAssignmentOnSameLine(sourceFile, contextToken.parent as PropertyAssignment ,contextToken.parent as PropertyAssignment));
}
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
if (changes) {
return {
sourceDisplay: undefined,
codeActions: [{
changes,
description: diagnosticToString([Diagnostics.Includes_imports_of_types_referenced_by_0, name]),
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
}],
};
}
}

if (!origin || !(originIsExport(origin) || originIsResolvedExport(origin))) {
return { codeActions: undefined, sourceDisplay: undefined };
}
Expand Down Expand Up @@ -4500,6 +4525,10 @@ function tryGetObjectLikeCompletionContainer(contextToken: Node | undefined): Ob
case SyntaxKind.Identifier:
return (contextToken as Identifier).text === "async" && isShorthandPropertyAssignment(contextToken.parent)
? contextToken.parent.parent : undefined;
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 && isObjectLiteralExpression(parent.parent) && contextToken.kind !== SyntaxKind.ColonToken) {
return parent.parent;
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/services/textChanges.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,11 @@ export class ChangeTracker {
this.replaceNode(sourceFile, oldNode, newNode, { suffix });
}

public replacePropertyAssignmentOnSameLine(sourceFile: SourceFile, oldNode: PropertyAssignment, newNode: PropertyAssignment): void {
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer used.

const suffix = this.nextCommaToken(sourceFile, oldNode) ? "" : ",";
this.replaceNode(sourceFile, oldNode, newNode, { suffix });
}

public insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}): void {
this.replaceRange(sourceFile, createRange(pos), newNode, options);
}
Expand Down
42 changes: 42 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,42 @@
/// <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,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});

verify.applyCodeActionFromCompletion("", {
name: "secondary",
description: `Includes imports of types referenced by 'secondary'`,
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/completionsObjectLiteralExpressions2.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 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,
}],
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
}
});

verify.applyCodeActionFromCompletion("", {
name: "secondary",
description: `Includes imports of types referenced by 'secondary'`,
newFileContent:
`interface ColorPalette {
primary?: string;
secondary?: string;
}
interface I {
color: ColorPalette;
}
const a: I = {
color: {primary: "red", }
}`,
preferences: {
allowIncompleteCompletions: true,
includeInsertTextCompletions: true,
},
});