Skip to content

Exclude literal completions after closing quote and JSX attribute location #52676

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

Merged
merged 4 commits into from
Feb 28, 2023
Merged

Exclude literal completions after closing quote and JSX attribute location #52676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9c3fea
Don't include literal completions in incorrect locations
zardoy Feb 8, 2023
38cdc4e
fix get kind crash
zardoy Feb 8, 2023
48a54d7
add test with number literals
zardoy Feb 9, 2023
3d7a105
Update src/services/completions.ts
zardoy Feb 9, 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
5 changes: 4 additions & 1 deletion src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2905,7 +2905,10 @@ function getCompletionData(
log("getCompletionData: Semantic work: " + (timestamp() - semanticStart));
const contextualType = previousToken && getContextualType(previousToken, position, sourceFile, typeChecker);

const literals = mapDefined(
// exclude literal suggestions after <input type="text" [||] /> (#51667) and after closing quote (#52675)
// for strings getStringLiteralCompletions handles completions
const isLiteralExpected = !tryCast(previousToken, isStringLiteralLike) && !isJsxIdentifierExpected;
const literals = !isLiteralExpected ? [] : mapDefined(
Copy link
Contributor Author

@zardoy zardoy Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Basically it always disallow literal completions in string and identifier locations. I wonder wether it can break any edge cases I don't know about?

Copy link
Member

@DanielRosenwasser DanielRosenwasser Feb 9, 2023

Choose a reason for hiding this comment

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

You could instead use isInStringOrRegularExpressionOrTemplateLiteral which will tell you if you're inside of the node (even if that node is unterminated).

TypeScript/src/services/completions.ts

Line 3694 in 1c822c4

function isInStringOrRegularExpressionOrTemplateLiteral(contextToken: Node): boolean {

Copy link
Contributor Author

@zardoy zardoy Feb 9, 2023

Choose a reason for hiding this comment

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

@DanielRosenwasser so as I understand your proposal is to replace isStringLiteralLike with isInStringOrRegularExpressionOrTemplateLiteral, but whats the difference? Am I missing something?

Copy link
Member

@DanielRosenwasser DanielRosenwasser Feb 9, 2023

Choose a reason for hiding this comment

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

If I understand correctly, the second one checks if the position is inside of the string, whereas the original just checks if the previous token is a string or template string. You might still want to keep isStringLiteralLike, but you want to check if the cursor is actually inside of the string.

Copy link
Contributor Author

@zardoy zardoy Feb 9, 2023

Choose a reason for hiding this comment

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

but you want to check if the cursor is actually inside of the string

But do I need this check? What issue it covers? I'm pretty sure there is already check if I'm inside of the string

TypeScript/src/services/stringCompletions.ts

Lines 198 to 200 in d0938c8

if (isInString(sourceFile, position, contextToken)) {
if (!contextToken || !isStringLiteralLike(contextToken)) return undefined;
const entries = getStringLiteralCompletionEntries(sourceFile, contextToken, position, program.getTypeChecker(), options, host, preferences);

If I understand correctly, the second one checks if the position is inside of the string, whereas the original just checks if the previous token is a string or template string

I have two checks for two issues correspondly, first one checks if the previous token is a string and second checks if jsx attribute identifier expected (see added tests). In the second check token is not necessarily is string: playground.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Feb 10, 2023

Choose a reason for hiding this comment

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

I think I see - it sounds like the idea that the previous token the the one immediately to the left of the original context token, so we could not have been inside of the string. Is that right?

Copy link
Member

@andrewbranch andrewbranch Feb 11, 2023

Choose a reason for hiding this comment

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

previousToken is either the token you’re currently typing or one behind, if you closed out a token but haven’t started typing a new one yet:

"hello|  // previousToken = String Literal
"hello"| // previousToken = String Literal

but, when the cursor is inside the string, a different code path would have already handled that and returned results from stringCompletions.ts. So in this case, we know we’re not inside the string. (Confusingly, stringCompletions.ts doesn’t handle completions of entire string literals including the quotes; it only handles completions inside existing string literals.)

Copy link
Contributor Author

@zardoy zardoy Feb 11, 2023

Choose a reason for hiding this comment

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

Wow this is really great explanation of first check. Thank you so much, I wish I could write same explanations in first place 😄

contextualType && (contextualType.isUnion() ? contextualType.types : [contextualType]),
t => t.isLiteral() && !(t.flags & TypeFlags.EnumLiteral) ? t.value : undefined);

Expand Down
7 changes: 7 additions & 0 deletions tests/cases/fourslash/completionsLiterals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

////const x: 0 | "one" = /**/;
////const y: 0 | "one" | 1n = /*1*/;
////const y2: 0 | "one" | 1n = 'one'/*2*/;

verify.completions({
marker: "",
Expand All @@ -20,3 +21,9 @@ verify.completions({
],
isNewIdentifierLocation: true,
});
verify.completions({
marker: "2",
excludes: [
'"one"'
],
});
7 changes: 6 additions & 1 deletion tests/cases/fourslash/stringLiteralCompletionsInJsxAttributeInitializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@

// @jsx: preserve
// @filename: /a.tsx
////type Props = { a: number } | { b: "somethingelse" };
////type Props = { a: number } | { b: "somethingelse", c: 0 | 1 };
////declare function Foo(args: Props): any
////
////const a1 = <Foo b={"/*1*/"} />
////const a2 = <Foo b="/*2*/" />
////const a3 = <Foo b="somethingelse"/*3*/ />
////const a4 = <Foo b={"somethingelse"} /*4*/ />
////const a5 = <Foo b={"somethingelse"} c={0} /*5*/ />

verify.completions({ marker: ["1", "2"], exact: ["somethingelse"] });
verify.completions({ marker: ["3", "4"], excludes: ['"somethingelse"'], });
Copy link
Contributor Author

@zardoy zardoy Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

didn't use exact: [a], because b gets included in position 3

verify.completions({ marker: ["5"], excludes: ["0", "1"], });