Skip to content

Add optionalReplacementSpan to completions response #40347

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 6 commits into from
Sep 4, 2020
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
7 changes: 7 additions & 0 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,13 @@ namespace FourSlash {
this.raiseError(`Expected 'isGlobalCompletion to be ${options.isGlobalCompletion}, got ${actualCompletions.isGlobalCompletion}`);
}

if (ts.hasProperty(options, "optionalReplacementSpan")) {
assert.deepEqual(
actualCompletions.optionalReplacementSpan && actualCompletions.optionalReplacementSpan,
options.optionalReplacementSpan && ts.createTextSpanFromRange(options.optionalReplacementSpan),
"Expected 'optionalReplacementSpan' properties to match");
}

const nameToEntries = new ts.Map<string, ts.CompletionEntry[]>();
for (const entry of actualCompletions.entries) {
const entries = nameToEntries.get(entry.name);
Expand Down
1 change: 1 addition & 0 deletions src/harness/fourslashInterfaceImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1529,6 +1529,7 @@ namespace FourSlashInterface {
readonly marker?: ArrayOrSingle<string | FourSlash.Marker>;
readonly isNewIdentifierLocation?: boolean; // Always tested
readonly isGlobalCompletion?: boolean; // Only tested if set
readonly optionalReplacementSpan?: FourSlash.Range; // Only tested if set
readonly exact?: ArrayOrSingle<ExpectedCompletionEntry>;
readonly includes?: ArrayOrSingle<ExpectedCompletionEntry>;
readonly excludes?: ArrayOrSingle<string>;
Expand Down
6 changes: 6 additions & 0 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2256,6 +2256,12 @@ namespace ts.server.protocol {
readonly isGlobalCompletion: boolean;
readonly isMemberCompletion: boolean;
readonly isNewIdentifierLocation: boolean;
/**
* In the absence of `CompletionEntry["replacementSpan"]`, the editor may choose whether to use
* this span or its default one. If `CompletionEntry["replacementSpan"]` is defined, that span
* must be used to commit that completion entry.
*/
readonly optionalReplacementSpan?: TextSpan;
readonly entries: readonly CompletionEntry[];
}

Expand Down
1 change: 1 addition & 0 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1783,6 +1783,7 @@ namespace ts.server {

const res: protocol.CompletionInfo = {
...completions,
optionalReplacementSpan: completions.optionalReplacementSpan && toProtocolTextSpan(completions.optionalReplacementSpan, scriptInfo),
entries,
};
return res;
Expand Down
15 changes: 13 additions & 2 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@ namespace ts.Completions {
return { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: false, entries };
}

function getOptionalReplacementSpan(location: Node | undefined) {
// StringLiteralLike locations are handled separately in stringCompletions.ts
return location?.kind === SyntaxKind.Identifier ? createTextSpanFromNode(location) : undefined;
}

function completionInfoFromData(sourceFile: SourceFile, typeChecker: TypeChecker, compilerOptions: CompilerOptions, log: Log, completionData: CompletionData, preferences: UserPreferences): CompletionInfo | undefined {
const {
symbols,
Expand Down Expand Up @@ -241,7 +246,7 @@ namespace ts.Completions {
kindModifiers: undefined,
sortText: SortText.LocationPriority,
};
return { isGlobalCompletion: false, isMemberCompletion: true, isNewIdentifierLocation: false, entries: [entry] };
return { isGlobalCompletion: false, isMemberCompletion: true, isNewIdentifierLocation: false, optionalReplacementSpan: getOptionalReplacementSpan(location), entries: [entry] };
}

const entries: CompletionEntry[] = [];
Expand Down Expand Up @@ -305,7 +310,13 @@ namespace ts.Completions {
entries.push(createCompletionEntryForLiteral(literal, preferences));
}

return { isGlobalCompletion: isInSnippetScope, isMemberCompletion: isMemberCompletionKind(completionKind), isNewIdentifierLocation, entries };
return {
isGlobalCompletion: isInSnippetScope,
isMemberCompletion: isMemberCompletionKind(completionKind),
isNewIdentifierLocation,
optionalReplacementSpan: getOptionalReplacementSpan(location),
entries
};
}

function isUncheckedFile(sourceFile: SourceFile, compilerOptions: CompilerOptions): boolean {
Expand Down
8 changes: 5 additions & 3 deletions src/services/stringCompletions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ namespace ts.Completions.StringCompletions {
}
}

function convertStringLiteralCompletions(completion: StringLiteralCompletion | undefined, contextToken: Node, sourceFile: SourceFile, checker: TypeChecker, log: Log, preferences: UserPreferences): CompletionInfo | undefined {
function convertStringLiteralCompletions(completion: StringLiteralCompletion | undefined, contextToken: StringLiteralLike, sourceFile: SourceFile, checker: TypeChecker, log: Log, preferences: UserPreferences): CompletionInfo | undefined {
if (completion === undefined) {
return undefined;
}

const optionalReplacementSpan = createTextSpanFromStringLiteralLikeContent(contextToken);
switch (completion.kind) {
case StringLiteralCompletionKind.Paths:
return convertPathCompletions(completion.paths);
Expand All @@ -33,7 +35,7 @@ namespace ts.Completions.StringCompletions {
CompletionKind.String,
preferences
); // Target will not be used, so arbitrary
return { isGlobalCompletion: false, isMemberCompletion: true, isNewIdentifierLocation: completion.hasIndexSignature, entries };
return { isGlobalCompletion: false, isMemberCompletion: true, isNewIdentifierLocation: completion.hasIndexSignature, optionalReplacementSpan, entries };
}
case StringLiteralCompletionKind.Types: {
const entries = completion.types.map(type => ({
Expand All @@ -43,7 +45,7 @@ namespace ts.Completions.StringCompletions {
sortText: "0",
replacementSpan: getReplacementSpanForContextToken(contextToken)
}));
return { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: completion.isNewIdentifier, entries };
return { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: completion.isNewIdentifier, optionalReplacementSpan, entries };
}
default:
return Debug.assertNever(completion);
Expand Down
6 changes: 6 additions & 0 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,12 @@ namespace ts {
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
isGlobalCompletion: boolean;
isMemberCompletion: boolean;
/**
* In the absence of `CompletionEntry["replacementSpan"], the editor may choose whether to use
* this span or its default one. If `CompletionEntry["replacementSpan"]` is defined, that span
* must be used to commit that completion entry.
*/
optionalReplacementSpan?: TextSpan;

/**
* true when the current location also allows for a new identifier
Expand Down
1 change: 1 addition & 0 deletions src/testRunner/unittests/tsserver/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ namespace ts.projectSystem {
isGlobalCompletion: true,
isMemberCompletion: false,
isNewIdentifierLocation: false,
optionalReplacementSpan: { start: { line: 1, offset: 1 }, end: { line: 1, offset: 4 } },
entries: [entry],
});

Expand Down
4 changes: 4 additions & 0 deletions src/testRunner/unittests/tsserver/metadataInResponse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ namespace ts.projectSystem {
isGlobalCompletion: false,
isMemberCompletion: true,
isNewIdentifierLocation: false,
optionalReplacementSpan: {
start: { line: 1, offset: aTs.content.indexOf("prop;") + 1 },
end: { line: 1, offset: aTs.content.indexOf("prop;") + 1 + "prop".length }
},
entries: expectedCompletionEntries
});
});
Expand Down
12 changes: 12 additions & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5964,6 +5964,12 @@ declare namespace ts {
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
isGlobalCompletion: boolean;
isMemberCompletion: boolean;
/**
* In the absence of `CompletionEntry["replacementSpan"], the editor may choose whether to use
* this span or its default one. If `CompletionEntry["replacementSpan"]` is defined, that span
* must be used to commit that completion entry.
*/
optionalReplacementSpan?: TextSpan;
/**
* true when the current location also allows for a new identifier
*/
Expand Down Expand Up @@ -8102,6 +8108,12 @@ declare namespace ts.server.protocol {
readonly isGlobalCompletion: boolean;
readonly isMemberCompletion: boolean;
readonly isNewIdentifierLocation: boolean;
/**
* In the absence of `CompletionEntry["replacementSpan"]`, the editor may choose whether to use
* this span or its default one. If `CompletionEntry["replacementSpan"]` is defined, that span
* must be used to commit that completion entry.
*/
readonly optionalReplacementSpan?: TextSpan;
readonly entries: readonly CompletionEntry[];
}
interface CompletionDetailsResponse extends Response {
Expand Down
6 changes: 6 additions & 0 deletions tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5964,6 +5964,12 @@ declare namespace ts {
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
isGlobalCompletion: boolean;
isMemberCompletion: boolean;
/**
* In the absence of `CompletionEntry["replacementSpan"], the editor may choose whether to use
* this span or its default one. If `CompletionEntry["replacementSpan"]` is defined, that span
* must be used to commit that completion entry.
*/
optionalReplacementSpan?: TextSpan;
Copy link

@jasonwilliams jasonwilliams Nov 19, 2021

Choose a reason for hiding this comment

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

@andrewbranch is this correct? The examples above in the unittests show TextSpan being used. But this is pointing to here, is it using the correct interface?

Copy link

@jasonwilliams jasonwilliams Nov 19, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, internally it should be a ts.TextSpan and over the wire it should be a ts.server.protocol.TextSpan. The conversion is done on the way out in session.ts. It’s possible we’re missing a conversion somewhere but the types are right. What’s going on?

Copy link

@jasonwilliams jasonwilliams Nov 19, 2021

Choose a reason for hiding this comment

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

What’s going on?

im having a nightmare with a typescript server plugin not having correct completion data, mainly range data, see here microsoft/vscode#134328

I thought maybe it needs to include This new field but TextSpan didn’t match with the examples here

Choose a reason for hiding this comment

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

I though the culprit was here https://github.com/microsoft/typescript-styled-plugin/blob/main/src/_language-service.ts#L121-L138 because item has the range, but its never converted. But the types seem correct.

Looking at the types for CompletionEntry and CompletionEntryDetails I don't see where the range would be passed through back to VSCode

Copy link
Member Author

Choose a reason for hiding this comment

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

Here’s where it gets converted on its way to VS Code

optionalReplacementSpan: completions.optionalReplacementSpan && toProtocolTextSpan(completions.optionalReplacementSpan, scriptInfo),

Copy link

@jasonwilliams jasonwilliams Nov 20, 2021

Choose a reason for hiding this comment

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

Thanks Andrew.
I don’t suppose you know of any typescript plugin that sets this? I’ve burnt out trying to find a solution for here. I’ve struggled to get the information of the token the cursor is setting next too (like offset and length).

Choose a reason for hiding this comment

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

@andrewbranch in case you were curious, this was the fix, took me days: microsoft/typescript-styled-plugin#156

/**
* true when the current location also allows for a new identifier
*/
Expand Down
13 changes: 13 additions & 0 deletions tests/cases/fourslash/completionsOptionalReplacementSpan1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// <reference path="fourslash.ts" />

// @lib: dom

//// console.[|cl/*0*/ockwork|];
//// type T = Array["[|toS/*1*/paghetti|]"];
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why this has to be optionalReplacementSpan and not just replacementSpan if editor supports it (there is already preference for that no)

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is there is not a preference that allows users to opt out of using replacementSpan—in VS and VS Code, the presence of replacementSpan means the editor will always use it, which is good because it means users can’t end up in a situation where they trigger completions on style.| and end up with a syntax error like style.["background-color"]. This new span will be configurable by a preference.

That said, it does seem like using the new optionalReplacementSpan is almost always better than the current behavior, but I guess @mjbvz and co. have thought through the reasons someone might want to have both options more than I have.

Copy link
Member

Choose a reason for hiding this comment

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

But i dont see this being configured by preference? Even if this is configured by preference, why cant it be returned as part of replacementSpan since replacement span if present is always meant to override this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it’s supposed to be an editor preference, which could potentially be configured across multiple languages. If TS Server adopted this preference and VS Code sent it to us, that would be an option. That’s not something that was discussed in the issue; I personally don’t have strong feelings one way or another. I’ll let @mjbvz weigh in.

Copy link
Contributor

@mjbvz mjbvz Sep 4, 2020

Choose a reason for hiding this comment

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

VS code lets users configure how suggestions are inserted using the editor.suggest.insertMode setting. The options are insert which always inserts the current suggestion without replacing the suffix, and replace which would try replacing the suffix word.

However users can always toggle to the opposite mode of the current one by holding down shift while accepting a completion. This means that TS always needs to return both spans to us in some way. I think we need a clear distinction between:

  • Here's a span that should always be replaced regardless of insert mode (the current replacementSpan property)
  • Here's a replacement span that you should use if the user accepts a completion in replace mode (which would be the new optionalRepalcementSpan)

I think the current proposal covers our needs but let me know if that doesn't sound correct based on my description

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Now understand why you always want this property to be populated and separate. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the clarification 👍


test.ranges().forEach((range, marker) => {
verify.completions({
marker: `${marker}`,
optionalReplacementSpan: range,
});
});
1 change: 1 addition & 0 deletions tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ declare namespace FourSlashInterface {
readonly marker?: ArrayOrSingle<string | Marker>;
readonly isNewIdentifierLocation?: boolean;
readonly isGlobalCompletion?: boolean;
readonly optionalReplacementSpan?: Range;
readonly exact?: ArrayOrSingle<ExpectedCompletionEntry>;
readonly includes?: ArrayOrSingle<ExpectedCompletionEntry>;
readonly excludes?: ArrayOrSingle<string>;
Expand Down