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

Syntax only server creates inferred project with all the open files w… #38561

Merged
merged 8 commits into from Jun 16, 2020

Conversation

@sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 13, 2020

…ith noResolve and can handle semantic operations

Fixes #

…ith noResolve and can handle semantic operations
@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented May 13, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented May 13, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 5726838. You can monitor the build here.

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented May 13, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented May 13, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 90d8a96. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented May 14, 2020

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/73860/artifacts?artifactName=tgz&fileId=2BC1D5FE49C2AD5CCA135D57CEE4CA308A5E08ADDD7B5A264EDC56EE6D487BE002&fileName=/typescript-4.0.0-insiders.20200514.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented May 14, 2020

@mjbvz @minestarks @uniqueiniquity @amcasey @DanielRosenwasser @RyanCavanaugh
#38561 (comment) is build where syntaxOnly server can accept semantic commands.. ( i havent added any custom changes for any semantic commands nor have i limited any semantic operations as part of this prototype)

#38564 is PR that is on top of this and makes normal tsserver behave like syntaxOnly (one with semantic operations allowed)

@amcasey
Copy link
Member

@amcasey amcasey commented May 14, 2020

I'll be interested to see what this does to startup perf on the syntax server. Looks pretty cool though.

@amcasey
Copy link
Member

@amcasey amcasey commented May 22, 2020

I've run into a pretty big issue on the VS side: semantic operations tend to be triggered for Roslyn Documents and those don't exist until the project is loaded. So I see operations during project load in already open documents, but not in the newly opened document (i.e. the one that triggered the project load). Obviously, this has nothing to do with the tsserver change.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 9, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Jun 9, 2020

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 90d8a96. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Jun 9, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/75966/artifacts?artifactName=tgz&fileId=BB849547A1DF11A54E5245D89DF5DAFE50EAC3E3C08A813981103A387395A0B802&fileName=/typescript-4.0.0-insiders.20200609.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 9, 2020

I'm not able to see things working better with

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 11, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Jun 11, 2020

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 90d8a96. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Jun 11, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/76193/artifacts?artifactName=tgz&fileId=0AD85EA4EF3D7253C964378CA736CCDB891DD80E981648EE08731F97511241AD02&fileName=/typescript-4.0.0-insiders.20200611.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 11, 2020

I'm finally trying this out with VS Code Insiders! Pretty cool! A couple of thoughts and ideas

From the editor side:

  • the startup time still isn't instantaneous for the syntax server, so I'm really never sure whether I can start editing reliably until I try quick info over and over. I think this is something we'll really need to provide a visual indication for before we make it broadly available.

From the TypeScript side:

  • I think cross-file go-to-definition was one of the key scenarios we'd like to get to users, so I think we should really try to get that to light up, either here or in a follow-up PR.
  • the fact that we show any when things aren't resolved in this mode is kind of confusing. For type display purposes, we might want to consider displaying the error type as something like a loading type.

@CyrusNajmabadi, how does Roslyn work when types haven't been resolved yet in things like quick info and signature help?

For example we have this signature

image

but CodeActionsState.State can't be resolved, so signature help looks like this:

Untitled

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Jun 11, 2020

@CyrusNajmabadi, how does Roslyn work when types haven't been resolved yet in things like quick info and signature help?

We don't have that concept :)

@sheetalkamat sheetalkamat marked this pull request as ready for review Jun 13, 2020
CommandNames.CompileOnSaveEmitFile,
CommandNames.CompilerOptionsDiagnosticsFull,
CommandNames.EncodedSemanticClassificationsFull,
CommandNames.SemanticDiagnosticsSync,

This comment has been minimized.

@sheetalkamat

sheetalkamat Jun 13, 2020
Author Member

SyntacticDiagnosticsSync: This is somewhat project setting dependent ? Atleast at some point parsing errors use to be different based on target .. eg target determines what unicode is considered identifier start... So i am not sure if this should be enabled.. But if we do enable we i was wondering if GetErr should only do syntax checks and skip semantic and suggetion diagnostics on syntax server

Some questionable which i have disabled for now
Reload
ReloadProjects
PrepareCallHierarchy
ProvideCallHierarchyIncomingCalls
ProvideCallHierarchyOutgoingCalls

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented Jun 13, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Jun 13, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 89127a5. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Jun 13, 2020

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/76345/artifacts?artifactName=tgz&fileId=CD3C20B0D73291C13F028F1A072CA740FDD866AD700C026ACA443AFA3B0B0C3002&fileName=/typescript-4.0.0-insiders.20200613.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

Copy link
Member

@amcasey amcasey left a comment

Some comments, but LGTM

Gulpfile.js Outdated Show resolved Hide resolved
src/server/session.ts Outdated Show resolved Hide resolved
@@ -1171,6 +1171,26 @@ namespace ts {
}
}

const invalidOperationsOnSyntaxOnly: readonly (keyof LanguageService)[] = [

This comment has been minimized.

@amcasey

amcasey Jun 16, 2020
Member

How does this relate to the list in session.ts? Do they need to stay in sync? Is one redundant?

if (syntaxOnly) {
invalidOperationsOnSyntaxOnly.forEach(key =>
ls[key] = (...args: any[]) => {
throw new Error(`LanguageService Operation: ${key} not allowed on syntaxServer:: arguments::${JSON.stringify(args)}`);

This comment has been minimized.

@amcasey

amcasey Jun 16, 2020
Member

As above, we probably don't need the arguments.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 16, 2020

Speaking of logs- do editors log from the syntax server?

@amcasey
Copy link
Member

@amcasey amcasey commented Jun 16, 2020

VS logging is opt-in (via env var or regkey), but, yes, it applies to both.

VS Code only has a command to show the semantic log (AFAIK), but I'm pretty sure it enables both.

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented Jun 16, 2020

@mjbvz We definitely want users to be able to share the syntaxOnly server logs easily.

@sheetalkamat sheetalkamat merged commit 25f6232 into master Jun 16, 2020
7 checks passed
7 checks passed
@github-actions
build (10.x)
Details
@github-actions
build (12.x)
Details
@github-actions
build (13.x)
Details
@microsoft-cla
license/cla All CLA requirements met.
Details
@azure-pipelines
node10 Build #76660 succeeded
Details
@azure-pipelines
node12 Build #76658 succeeded
Details
@azure-pipelines
node13 Build #76659 succeeded
Details
@sheetalkamat sheetalkamat deleted the syntaxAsSemanticServer branch Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants