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.

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

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Feb 8, 2023

Fixes #51667
Fixes #52675

contextualType is derived from previous token

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Feb 8, 2023

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

@zardoy zardoy Feb 8, 2023

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

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

@zardoy zardoy Feb 8, 2023

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

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).

function isInStringOrRegularExpressionOrTemplateLiteral(contextToken: Node): boolean {

Copy link
Contributor Author

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

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

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

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

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

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

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 😄

@zardoy zardoy changed the title Don't include literal completions in incorrect locations Exclude literal completions after closing quote and JSX attribute location Feb 9, 2023
@andrewbranch andrewbranch self-assigned this Feb 13, 2023
@sandersn
Copy link
Member

@andrewbranch is this ready to merge now that 5.1 development has started?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

String literal completions right after quote Intellisense suggestion in Typescript after prop with literal string type
5 participants