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

Jump to bottom

fix: no variable suggestions without semicolon #54656

Merged
merged 5 commits into from
Jun 26, 2023
Merged

fix: no variable suggestions without semicolon #54656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d77caa
fix: no variable suggestions without semicolon
zardoy Jun 15, 2023
9c87f3a
tests!
zardoy Jun 15, 2023
f67c9a3
lint
zardoy Jun 15, 2023
862807b
make sure parameter takes priority
zardoy Jun 15, 2023
4c2bcc9
better formatting
zardoy Jun 23, 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
16 changes: 11 additions & 5 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2439,7 +2439,7 @@ export function getCompletionEntriesFromSymbols(
includeSymbol = false
): UniqueNameSet {
const start = timestamp();
const variableOrParameterDeclaration = getVariableOrParameterDeclaration(contextToken);
const variableOrParameterDeclaration = getVariableOrParameterDeclaration(contextToken, location);
const useSemicolons = probablyUsesSemicolons(sourceFile);
const typeChecker = program.getTypeChecker();
// Tracks unique names.
Expand Down Expand Up @@ -5516,14 +5516,20 @@ function isModuleSpecifierMissingOrEmpty(specifier: ModuleReference | Expression
return !tryCast(isExternalModuleReference(specifier) ? specifier.expression : specifier, isStringLiteralLike)?.text;
}

function getVariableOrParameterDeclaration(contextToken: Node | undefined) {
function getVariableOrParameterDeclaration(contextToken: Node | undefined, location: Node) {
if (!contextToken) return;
Copy link
Member

Choose a reason for hiding this comment

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

Should we still return early if contextToken is undefined? Can we have a valid case where contextToken is undefined but location is defined?

Copy link
Contributor Author

@zardoy zardoy Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I didn't think about it when I opened this PR. Though for me its obvious that it can be undefined only when there is no tokens behind the requesting position at all (as essentially contextToken is previousToken). Otherwise it will be something like OpenBraceToken.


const declaration = findAncestor(contextToken, node =>
const possiblyParameterDeclaration = findAncestor(contextToken, node =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the existing code already uses findAncestor, but is this really needed @DanielRosenwasser? Can't we restrict the scope of this search?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean do we really need two walks? I'm not sure; we could probably rewrite this to only do the second walk if the first fails though.

Copy link
Member

Choose a reason for hiding this comment

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

Another way to do this is to check whether the contextToken ever becomes the location, and only then to start checking for variable declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean do we really need two walks? I'm not sure; we could probably rewrite this to only do the second walk if the first fails though.

I'm not sure I'm following initial request here. Why we can't have two walks here? (since it uses existing ast and we also do it only once per completionInfo request)

isFunctionBlock(node) || isArrowFunctionBody(node) || isBindingPattern(node)
? "quit"
: isVariableDeclaration(node) || ((isParameter(node) || isTypeParameterDeclaration(node)) && !isIndexSignatureDeclaration(node.parent)));
return declaration as ParameterDeclaration | TypeParameterDeclaration | VariableDeclaration | undefined;
: ((isParameter(node) || isTypeParameterDeclaration(node)) && !isIndexSignatureDeclaration(node.parent)));

const possiblyVariableDeclaration = findAncestor(location, node =>
isFunctionBlock(node) || isArrowFunctionBody(node) || isBindingPattern(node)
? "quit"
: isVariableDeclaration(node));

return (possiblyParameterDeclaration || possiblyVariableDeclaration) as ParameterDeclaration | TypeParameterDeclaration | VariableDeclaration | undefined;
}

function isArrowFunctionBody(node: Node) {
Expand Down
10 changes: 8 additions & 2 deletions tests/cases/fourslash/completionEntryForConst.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
///<reference path="fourslash.ts" />

////const c = "s";
/////**/
/////*1*/
////const d = 1
////d/*2*/
////const e = 1
/////*3*/

verify.completions({ marker: "", includes: { name: "c", text: 'const c: "s"', kind: "const" } });
verify.completions({ marker: ["1"], includes: { name: "c", text: 'const c: "s"', kind: "const" } });
verify.completions({ marker: ["2"], includes: { name: "d", text: 'const d: 1', kind: "const" } });
verify.completions({ marker: ["3"], includes: { name: "e", text: 'const e: 1', kind: "const" } });