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.

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

Add exported members of all project files in the global completion list #19069

Merged
45 commits merged into from
Oct 17, 2017

Conversation

ghost
Copy link

@ghost ghost commented Oct 10, 2017

Updated version of #17851

I tested this out in the vscode repository by navigating to actionbar.ts and hitting ctrl-space on an empty line to get completions. Completions time went up from 30 to 50ms, which isn't too bad. (Note that this is after waiting 20 seconds for the project to load.)

However, the length of the output went up from 75054 characters to 476065 -- 6 times as much. This means that "Loading..." will show up for a moment before showing completions. Note that this only happens if no characters have been typed yet and we have to send every completion -- if even a single character has been typed in there is no noticeable delay.

We might want to work on reducing the size of completions output, such as not including sortText every time (it's always 0).

…etSymbolsFromOtherSourceFileExports inside getCompletionData
…ntriesFromSymbols and to return value of getCompletionData
… symbolToOriginInfoMap (meaning there's an import action)
…eActionForImport, update call sites, destructure it, use compilerOptions in getModuleSpecifierForNewImport
@mhegazy
Copy link
Contributor

mhegazy commented Oct 11, 2017

@mjbvz we will need a to give this a test to make sure we are not degrading the overall completion experience in VSCode for large projects.. and whether limiting the number of entries would be a possibility.

@mjbvz
Copy link
Contributor

mjbvz commented Oct 11, 2017

Thanks @jrieken! Yes I forgot about the CompletionList type.

@mhegazy Let's make sure this is tested on the vscode and typescript repos at least. If there are perf isssues, we may have to add a new incomplete flag on CompletionsResponse or similar to tell VSCode to should re-request completions

@mhegazy
Copy link
Contributor

mhegazy commented Oct 12, 2017

We will hold off on merging this untill we hear back from you then.

@jrieken
Copy link
Member

jrieken commented Oct 12, 2017

Rule of thumb is that the completion UI, esp. sorting and ranking, is still fast and responsive with ~10000 elements

@mjbvz
Copy link
Contributor

mjbvz commented Oct 13, 2017

@mhegazy and @andy-ms I just tested a local build of the branch and did not see any performance problems. I also don't see the global completions returned on a empty line however. Is this the expected behavior?

@ghost
Copy link
Author

ghost commented Oct 13, 2017

@mjbvz Did you see any completions at all on the empty line?
We might have different subjective standards of what a performance problem is -- I'm judging this by whether I see Loading... at all. Since TypeScript's computational time is only 50ms, it's possible that the delay is due to VSCode having to do work to render all of those completions, so maybe there's something you could do on your side to speed that up.

We had discussed a strategy where on the TypeScript side we would try and filter out just those completions that start with the identifier text -- but it looks like VSCode usually has a more loose way of matching completions, so that's probably better done on the vscode side. If the issue isn't actually the communication between the editor and language server and is actually just rendering time, then there's no problem with doing the filtering in vscode.

@mjbvz
Copy link
Contributor

mjbvz commented Oct 13, 2017

@andy-ms When triggering on an empty line, I only see the standard global completions currently. Here's a gif of the difference in behavior:

oct-13-2017 14-38-53

In the first example, I trigged suggestions on an empty line and SettingsDocument is not found. In the second, I start typing which triggers the suggestions after the s has already been entered. This time SettingsDocument is found

@ghost
Copy link
Author

ghost commented Oct 13, 2017

@mjbvz Thanks, I've tracked down that behavior to a bug in our filtering completions with startsWith (we were checking if it started with ";"). But see my above comment - maybe we shouldn't have the startsWith call in TypeScript because vscode does more loose completion matching? Is there anyway you could test how long it takes to get the completions from TypeScript vs how long it is taking to render them?

@mjbvz
Copy link
Contributor

mjbvz commented Oct 13, 2017

@andy-ms I think we should keep the startsWith filtering. This has been the existing behavior for a while, correct?


Here's my measurements in the vscode codebase with the latest changes.

Blank line:

ts extension: 51.066ms
suggest e2e: 249.678955078125ms

Start typing:

ts extension: 98.494ms
suggest e2e: 155.81494140625ms

ts extension tracks the time of the completions request within the typescript extension itself. This includes the request to the TS server and receiving the response.

suggest e2e tracks the entire time to compute the completions. This includes the time to ask the ts extension for these completions and processing the response.

@ghost
Copy link
Author

ghost commented Oct 13, 2017

This has been the existing behavior for a while, correct?

No, it was added in this PR just for the completions from module exports.

@jrieken
Copy link
Member

jrieken commented Oct 16, 2017

I wouldn't use startWith as that is a really weak check which prevents any smartness further down the road. Use a check that makes sure every character from the prefix/query text occurs in order in the suggestion text. The in order check could be made optional as we have requests to account for typos, so cno should still match console. Consider that stretch

@ghost ghost force-pushed the exportsincompletionlist4 branch from 86224ff to 9420d28 Compare October 16, 2017 21:11
@ghost ghost force-pushed the exportsincompletionlist4 branch from 9420d28 to 4c607e7 Compare October 16, 2017 21:11
@ghost ghost merged commit 2b566b9 into master Oct 17, 2017
@ghost ghost deleted the exportsincompletionlist4 branch October 17, 2017 17:20
ghost pushed a commit that referenced this pull request Oct 17, 2017
…st (#19069)

* checker.ts: Remove null check on symbols

* tsserverProjectSystem.ts: add two tests

* client.ts, completions.ts, types.ts: Add codeActions member to CompletionEntryDetails

* protocol.ts, session.ts: Add codeActions member to CompletionEntryDetails protocol

* protocol.ts, session.ts, types.ts: add hasAction to CompletionEntry

* session.ts, services.ts, types.ts: Add formattingOptions parameter to getCompletionEntryDetails

* completions.ts: define SymbolOriginInfo type

* completions.ts, services.ts: Add allSourceFiles parameter to getCompletionsAtPosition

* completions.ts, services.ts: Plumb allSourceFiles into new function getSymbolsFromOtherSourceFileExports inside getCompletionData

* completions.ts: add symbolToOriginInfoMap parameter to getCompletionEntriesFromSymbols and to return value of getCompletionData

* utilities.ts: Add getOtherModuleSymbols, getUniqueSymbolIdAsString, getUniqueSymbolId

* completions.ts: Set CompletionEntry.hasAction when symbol is found in symbolToOriginInfoMap (meaning there's an import action)

* completions.ts: Populate list with possible exports (implement getSymbolsFromOtherSourceFileExports)

* completions.ts, services.ts: Plumb host and rulesProvider into getCompletionEntryDetails

* completions.ts: Add TODO comment

* importFixes.ts: Add types ImportDeclarationMap and ImportCodeFixContext

* Move getImportDeclarations into getCodeActionForImport, immediately after the implementation

* importFixes.ts: Move createChangeTracker into getCodeActionForImport, immediately after getImportDeclarations

* importFixes.ts: Add convertToImportCodeFixContext function and reference it from the getCodeActions lambda

* importFixes.ts: Add context: ImportCodeFixContext parameter to getCodeActionForImport, update call sites, destructure it, use compilerOptions in getModuleSpecifierForNewImport

* importFixes.ts: Remove moduleSymbol parameter from getImportDeclarations and use the ambient one

* importFixes.ts: Use cachedImportDeclarations from context in getCodeActionForImport

* importFixes.ts: Move createCodeAction out, immediately above convertToImportCodeFixContext

* Move the declaration for lastImportDeclaration out of the getCodeActions lambda into getCodeActionForImport

* importFixes.ts: Use symbolToken in getCodeActionForImport

* importFixes.ts: Remove useCaseSensitiveFileNames altogether from getCodeActions lambda

* importFixes.ts: Remove local getUniqueSymbolId function and add checker parameter to calls to it

* importFixes.ts: Move getCodeActionForImport out into an export, immediately below convertToImportCodeFixContext

* completions.ts: In getCompletionEntryDetails, if there's symbolOriginInfo, call getCodeActionForImport

* importFixes.ts: Create and use importFixContext within getCodeActions lambda

* importFixes.ts: Use local newLineCharacter instead of context.newLineCharacter in getCodeActionForImport

* importFixes.ts: Use local host instead of context.host in getCodeActionForImport

* importFixes.ts: Remove dummy getCanonicalFileName line

* Filter symbols after gathering exports instead of before

* Lint

* Test, fix bugs, refactor

* Suggestions from code review

* Update api baseline

* Fix bug if previousToken is not an Identifier

* Replace `startsWith` with `stringContainsCharactersInOrder`
mhegazy added a commit that referenced this pull request Oct 19, 2017
* Adding test case where opened file included in project is not added to ref count of configured project

* Fix the way configured project's reference is managed so that the open file

* Do not watch root folders for failed lookup locations and effective type roots
Fixes #19170

* LEGO: check in for master to temporary branch.

* LEGO: check in for master to temporary branch.

* Mark fresh spread objects w/ContainsObjectLiteral

* Test excess property checks of spreads of unions.

* Add exported members of all project files in the global completion list (#19069)

* checker.ts: Remove null check on symbols

* tsserverProjectSystem.ts: add two tests

* client.ts, completions.ts, types.ts: Add codeActions member to CompletionEntryDetails

* protocol.ts, session.ts: Add codeActions member to CompletionEntryDetails protocol

* protocol.ts, session.ts, types.ts: add hasAction to CompletionEntry

* session.ts, services.ts, types.ts: Add formattingOptions parameter to getCompletionEntryDetails

* completions.ts: define SymbolOriginInfo type

* completions.ts, services.ts: Add allSourceFiles parameter to getCompletionsAtPosition

* completions.ts, services.ts: Plumb allSourceFiles into new function getSymbolsFromOtherSourceFileExports inside getCompletionData

* completions.ts: add symbolToOriginInfoMap parameter to getCompletionEntriesFromSymbols and to return value of getCompletionData

* utilities.ts: Add getOtherModuleSymbols, getUniqueSymbolIdAsString, getUniqueSymbolId

* completions.ts: Set CompletionEntry.hasAction when symbol is found in symbolToOriginInfoMap (meaning there's an import action)

* completions.ts: Populate list with possible exports (implement getSymbolsFromOtherSourceFileExports)

* completions.ts, services.ts: Plumb host and rulesProvider into getCompletionEntryDetails

* completions.ts: Add TODO comment

* importFixes.ts: Add types ImportDeclarationMap and ImportCodeFixContext

* Move getImportDeclarations into getCodeActionForImport, immediately after the implementation

* importFixes.ts: Move createChangeTracker into getCodeActionForImport, immediately after getImportDeclarations

* importFixes.ts: Add convertToImportCodeFixContext function and reference it from the getCodeActions lambda

* importFixes.ts: Add context: ImportCodeFixContext parameter to getCodeActionForImport, update call sites, destructure it, use compilerOptions in getModuleSpecifierForNewImport

* importFixes.ts: Remove moduleSymbol parameter from getImportDeclarations and use the ambient one

* importFixes.ts: Use cachedImportDeclarations from context in getCodeActionForImport

* importFixes.ts: Move createCodeAction out, immediately above convertToImportCodeFixContext

* Move the declaration for lastImportDeclaration out of the getCodeActions lambda into getCodeActionForImport

* importFixes.ts: Use symbolToken in getCodeActionForImport

* importFixes.ts: Remove useCaseSensitiveFileNames altogether from getCodeActions lambda

* importFixes.ts: Remove local getUniqueSymbolId function and add checker parameter to calls to it

* importFixes.ts: Move getCodeActionForImport out into an export, immediately below convertToImportCodeFixContext

* completions.ts: In getCompletionEntryDetails, if there's symbolOriginInfo, call getCodeActionForImport

* importFixes.ts: Create and use importFixContext within getCodeActions lambda

* importFixes.ts: Use local newLineCharacter instead of context.newLineCharacter in getCodeActionForImport

* importFixes.ts: Use local host instead of context.host in getCodeActionForImport

* importFixes.ts: Remove dummy getCanonicalFileName line

* Filter symbols after gathering exports instead of before

* Lint

* Test, fix bugs, refactor

* Suggestions from code review

* Update api baseline

* Fix bug if previousToken is not an Identifier

* Replace `startsWith` with `stringContainsCharactersInOrder`

* Dont try to run unit tests with rwc tests again (#19240)

* In tsserver, indent logged JSON (#19080)

* noUnusedLocals: Warn for recursive call to private method (#18920)

* Added test for windows style paths watched directories

* Handle when directory watcher is invoked on file change
Fixes #19206

* Add quickfix and refactoring to install @types packages (#19130)

* Add quickfix and refactoring to install @types packages

* Move `validatePackageName` to `jsTyping.ts`

* Remove combinePaths overloads

* Respond to code review

* Update api baselines

* Use native PromiseConstructor

* Return false instead of undefined

* Remove getProjectRootPath

* Update api

* This wasnt required before... (#19262)

* Collapse jsdoc annotation refactors to one

Previously there were two, and two always fired.

* Update baselines

* Fix #19257: Ensure a generated signature has a return type (#19264)

* Fix #19257: Ensure a generated signature has a return type

* Ensure generated properties have types

* Use the same context for multiple inferences to the same property access

* Respect newLine compiler option in language service output (#19279)

* LEGO: check in for master to temporary branch.

* LEGO: check in for master to temporary branch.

* LEGO: check in for master to temporary branch.

* Disambiguate same-named refactors using description (#19267)

Disambiguate same-named refactors using actionName

* Set the scriptKind from the host configuration if present

* Update API baselines

* Remove erroneous error for JSDoc object literals

appears with checkJS.

* Update annotateWithTypeFromJSDoc tests

1. Object literals are single-line now.
2. Index signatures transform to TS index signatures.
3. The refactoring is only available when it could add types.

* Add a better test for jsdoc index signatures.

The test case shows that the errorenous error no longer appears.

* Fix bugs in jsdoc annotation refactor

1. Transform index signatures to TS index signatures.
2. Print object literals on a single line.
3. Only offer the refactor when it could add types. (There must not be a
type annotation already, and there must be a JSDoc that applies.)

* Move isJSDocIndexSignature to utilities

* Add failing testcase where when d.ts file is in program, the files get emitted multiple times with --out setting

* Modify api to emit affected files using callback instead of generating in memory output
Also marking few apis introduced during watch improvements changes that are suppose to be internal for now

* Use get files affected by internally and hence use file paths as input

* Simplify emit changed files further
Also use source file version as the signature of declaration file instead of computing it from text

* Do not cache the semantic diagnostics when compiler options has --out since we would anyways get all fresh diagnostics

* make getCurrentDirectory required (#19303)

* LEGO: check in for master to temporary branch.

* Actually use cached semantic diagnostics

* Fix tsc-instrumented

1. Make recursiveCreateDirectory correctly handle relative paths.
2. Remove dependency on Harness
3. Correctly increment iocapture0, iocapture1, ... iocaptureN.
4. Stop double-nesting baseline files.

* Fix lint

* Fix #19270: ensure output name is a valid locale name (#19308)

* Fix #19270: ensure output name is a valid locale name

* Use const instead of var

* Add comment

* Fix typo

* Split the concat logic for generatedLCGFile

* findAllRefs: Support anonymous default export (#19302)

* Fix undefined error using `getEffectiveTypeRoots` (#19300)

* Remove extra blank line in logs (#19307)

* Use Promise instead of PromiseLike (#19305)

* Workaround for nonnull operator on indexed accesses (#19275)

* Quick and dirty workaround

* Add third case to show current behavior

* Rename variable, replace elaboration from comment with links

* Remove some unnecessary `undefined` checks in extractSymbol (#19256)

* Fix "noStringLiteral" lint errors (#19310)

* LEGO: check in for master to temporary branch.

* Rename locale outputs

* Update LKG
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants