Don't include completions for current and later parameters #52690

zardoy · 2023-02-09T00:19:25Z

Tried to find oldest completion issue :)

DanielRosenwasser

The test file should be renamed:

tests/cases/fourslash/noIncorrectParamaterCompletions.ts
tests/cases/fourslash/noCompletionsForCurrentOrLaterParameters.ts

DanielRosenwasser · 2023-02-09T00:51:00Z

src/services/completions.ts

    const variableDeclaration = getVariableDeclaration(location);
+    const parameterDeclaration = getParameterDeclaration(contextToken);


Can you just switch the helper function to just be getVariableOrParameterDeclaration and make decisions based on that?

So your decision is to unify variableDeclaration and parameterDeclaration?

eg

const variableOrParameterDeclaration = getVariableOrParameterDeclaration(location, contextToken)

But what about this case then?:

const foo = (a = foo/*no foo suggestion*/) => { console.log(foo) }

BTW its fully valid location for foo suggestion, so maybe you're right

I switched to getVariableOrParameterDeclaration(contextToken: Node | undefined). I'm using only contextToken to fit logic in single findAncestor. I don't really the idea of dropping location as I feel it can introduce a lot of breakage, though I'll look at tests and try to fix them

BTW its fully valid location for foo suggestion, so maybe you're right

FYI

TypeScript/tests/cases/fourslash/completionListWithoutVariableinitializer.ts

Line 50 in 0791bb0

excludes: ["fn"],

DanielRosenwasser · 2023-02-09T01:00:27Z

src/services/completions.ts

+                // Filter out parameters from their own initializers
+                // `function f(a = /* no 'a' here */) { }`
+                if (symbol.valueDeclaration === parameterDeclaration) {
+                    return false;
+                }
+                // Filter out parameters from other parameters' initializers
+                // `function f(a = /* no 'b' here */, b) { }`
+                const parameters = parameterDeclaration.parent.parameters;
+                const currentParamIdx = parameters.indexOf(parameterDeclaration);
+                if (parameters.slice(currentParamIdx).some((p) => symbol.valueDeclaration === p)) {
+                    return false;
+                }


I wonder if you could just get await with using the node positions.

Suggested change

// Filter out parameters from their own initializers

// `function f(a = /* no 'a' here */) { }`

if (symbol.valueDeclaration === parameterDeclaration) {

return false;

}

// Filter out parameters from other parameters' initializers

// `function f(a = /* no 'b' here */, b) { }`

const parameters = parameterDeclaration.parent.parameters;

const currentParamIdx = parameters.indexOf(parameterDeclaration);

if (parameters.slice(currentParamIdx).some((p) => symbol.valueDeclaration === p)) {

return false;

}

const declarationPos = symbol.valueDeclaration.pos;

if (parameterDeclaration.pos <= declarationPos && declarationPos < parameters.end) {

return false;

}

Seems to be good idea! but I still wonder why do we need declarationPos < parameters.end check?

tests/cases/fourslash/noIncorrectParamaterCompletions.ts

zardoy · 2023-02-09T08:05:44Z

Didn't expect so many failing tests 😅 , will look again at them in a few days

though it was enough to do most top contextToken only, but lets be consistent in code

zardoy · 2023-02-09T18:43:05Z

It seems using contextToken in new implementation improved quality in some test cases, I'm happy with result

zardoy · 2023-02-09T18:58:11Z

Also I have a quick question. Speaking of getVariableOrParameterDeclaration I wonder wether the logic can be extracted for codefixes in future. Just for motivation consider this issue:

let fileName

const foo = (displayName = fileNam/*<-*/, fileName2) => {
    // ERROR: Cannot find name 'fileNam'. Did you mean 'fileName2'?

    // super annoying sometimes, would be cool if codefixes were smarter
}

zardoy · 2023-02-17T17:26:02Z

@DanielRosenwasser I'm sorry to bother you, but I wanted to do the same for type parameters. Would it be better to do it in this PR or create another one after its get merged? Its just a matter of adding an extra check 😉.

src/services/completions.ts

jakebailey · 2023-02-18T00:19:03Z

I'm sorry to bother you, but I wanted to do the same for type parameters. Would it be better to do it in this PR or create another one after its get merged? Its just a matter of adding an extra check 😉.

How simple are we talking? If it's not too bad, I don't think it's a big deal to include it here.

pick changes from 3f2a0ff

tests/cases/fourslash/completionListIsGlobalCompletion.ts

zardoy · 2023-02-19T03:16:07Z

How simple are we talking? If it's not too bad, I don't think it's a big deal to include it here.

Okay, I think it's done

sandersn

Very nice and small change. I had some questions and suggestions.

src/services/completions.ts

tests/cases/fourslash/completionListInUnclosedFunction09.ts

src/services/completions.ts

zardoy · 2023-03-20T20:59:36Z

I see it's in waiting on author. Is anything else required here from my side?

sandersn · 2023-04-13T21:57:41Z

I was waiting on a review from @DanielRosenwasser since he previously requested changes.

zardoy added 2 commits February 9, 2023 03:03

add impl to ensure its not lost

f259b14

add test that seems okay

7c1fdfb

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

hope to fix lint

f69d9d7

zardoy changed the title ~~add impl to ensure its not lost~~ Don't include completion for self-referencing and next parameters Feb 9, 2023

fix lint (more chances)

4749713

DanielRosenwasser requested changes Feb 9, 2023

View reviewed changes

zardoy added 7 commits February 9, 2023 04:46

be surprised with the number of tests to fix

09caed3

fix don't include index signature

d520a31

fix my tests

e27faed

add positive testcases

fee4e96

simplify implementation

f3154b0

refactor to getVariableOrParameterDeclaration (simplify)

36ed7b7

fix remaining test?

c1d02f2

zardoy changed the title ~~Don't include completion for self-referencing and next parameters~~ Don't include completions for current and later parameters Feb 9, 2023

rename test file

b204b1d

zardoy added 3 commits February 9, 2023 21:25

again fix bunch of tests (imporve quality)

b6eafdd

fix const a = () => a

038e953

though it was enough to do most top contextToken only, but lets be consistent in code

add more complex test case

a0aea62

zardoy requested a review from DanielRosenwasser February 9, 2023 18:43

zardoy mentioned this pull request Feb 11, 2023

Exclude completions of binding pattern variable initializers #52723

Merged

DanielRosenwasser requested review from sandersn and jakebailey February 17, 2023 18:46

DanielRosenwasser reviewed Feb 17, 2023

View reviewed changes

src/services/completions.ts Outdated Show resolved Hide resolved

avoid casting

85e35e0

zardoy added 2 commits February 19, 2023 04:40

do the same for type parameters

0b281e2

pick changes from 3f2a0ff

fix tests

dced6bc

zardoy commented Feb 19, 2023

View reviewed changes

tests/cases/fourslash/completionListIsGlobalCompletion.ts Outdated Show resolved Hide resolved

sandersn requested changes Mar 7, 2023

View reviewed changes

proposed changes

d1666e8

sandersn self-assigned this Mar 9, 2023

sandersn approved these changes Mar 15, 2023

View reviewed changes

DanielRosenwasser approved these changes Apr 15, 2023

View reviewed changes

DanielRosenwasser merged commit c70fa51 into microsoft:main Apr 15, 2023

Andarist mentioned this pull request Jun 15, 2023

Code comlpetion not working for variables #54654

Closed

Andarist mentioned this pull request Nov 20, 2023

Latter type parameters are not suggested in constraints of the preceding ones #56474

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't include completions for current and later parameters #52690

Don't include completions for current and later parameters #52690

zardoy commented Feb 9, 2023 •

edited

Loading

DanielRosenwasser left a comment

DanielRosenwasser Feb 9, 2023

zardoy Feb 9, 2023

zardoy Feb 9, 2023

zardoy Feb 9, 2023

zardoy Feb 9, 2023

DanielRosenwasser Feb 9, 2023

zardoy Feb 9, 2023

zardoy commented Feb 9, 2023

zardoy commented Feb 9, 2023

zardoy commented Feb 9, 2023

zardoy commented Feb 17, 2023

jakebailey commented Feb 18, 2023

zardoy commented Feb 19, 2023

sandersn left a comment

zardoy commented Mar 20, 2023

sandersn commented Apr 13, 2023

		const variableDeclaration = getVariableDeclaration(location);
		const parameterDeclaration = getParameterDeclaration(contextToken);

Don't include completions for current and later parameters #52690

Don't include completions for current and later parameters #52690

Conversation

zardoy commented Feb 9, 2023 • edited Loading

DanielRosenwasser left a comment

Choose a reason for hiding this comment

DanielRosenwasser Feb 9, 2023

Choose a reason for hiding this comment

zardoy Feb 9, 2023

Choose a reason for hiding this comment

zardoy Feb 9, 2023

Choose a reason for hiding this comment

zardoy Feb 9, 2023

Choose a reason for hiding this comment

zardoy Feb 9, 2023

Choose a reason for hiding this comment

DanielRosenwasser Feb 9, 2023

Choose a reason for hiding this comment

zardoy Feb 9, 2023

Choose a reason for hiding this comment

zardoy commented Feb 9, 2023

zardoy commented Feb 9, 2023

zardoy commented Feb 9, 2023

zardoy commented Feb 17, 2023

jakebailey commented Feb 18, 2023

zardoy commented Feb 19, 2023

sandersn left a comment

Choose a reason for hiding this comment

zardoy commented Mar 20, 2023

sandersn commented Apr 13, 2023

zardoy commented Feb 9, 2023 •

edited

Loading