Refactor for performance improvements

milesziemer commented 3 months ago

Re-writes almost everything in the language server to improve performance (see https://github.com/smithy-lang/smithy-language-server/issues/145), and lay the ground work for further progress and features. Given the scope of these changes, this should be considered a WIP as I may have broken some things.

Overview of performance improvements:

Per-file model updates on change events
Model validation only run on save
Async execution of model building and validation with cancellation
Async execution of some language features like completion and document symbol with cancellation
Incremental file change updates
Reduced file reads from disk and string copies

From the end user's perspective, only one major change has been made what the language server can do. It now uses a smithy-build.json in the root of the workspace as the source of truth for what is part of the project. Previously, the server loaded all Smithy files it found in any subdir of the root. This doesn't scale well with multi-root workspaces, and leads to an issue where Smithy files in the build directory are added to the project, duplicating sources. The new process looks for a smithy-build.json and uses only its sources and imports as files to load into the model (maven deps are still supported, this is just referring to project files). For backward compatibility, the old SmithyBuildExtensions build/smithy-dependencies.json and .smithy.json are still supported. A new file, .smithy-project.json, is being developed which allows projects that are configured outside of a smithy-build.json (such as a Gradle project) to specify their project files and dependencies. Right now these dependencies are local, paths to JARs, but it may make sense to support Maven dependencies in there as well. More to come on how .smithy-project.json works in documentation updates. To support using the language server without a smithy-build.json, a future update is in progress to allow a 'detached' project which loads whatever files you open.

Other updates:

Use smithy-syntax formatter
Report progress to client on load
Add configuration option for the minimum severity of validation events
Update dependencies

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

milesziemer commented 3 months ago

CI failing due to a missing Smithy change I had locally: https://github.com/smithy-lang/smithy/pull/2214

kubukoz commented 3 months ago

I'm trying this out locally... and it doesn't seem to work very well. Probably something obvious happening at project load time that prevents the rest from moving on.

Here's what I'm trying:

./gradlew publishToMavenLocal

Setting my vscode so that it points to

"smithyLsp.lspCoordinates": "software.amazon.smithy:smithy-language-server",
"smithyLsp.version": "0.3.0"

Using the following Smithy file
```
$version: "2"
```

namespace hello

service WeatherService { operations: [GetWeather, CreateCity] }

@http(method: "GET", uri: "/cities/{cityId}/weather") @readonly operation GetWeather { input := { @required @httpLabel cityId: CityId } output := { @required weather: String degrees: Integer } }

@http(method: "POST", uri: "/cities", code: 201) operation CreateCity { input := { @required city: String @required country: String } output := { @required cityId: CityId } }

string CityId


4. Using the following smithy-build.json, just to be sure

{ "version": "1.0", "languageServer": "software.amazon.smithy:smithy-language-server:0.3.0" }



Here's my output tab: https://gist.github.com/kubukoz/d2158c18a10c6969f8dd9dae898d58cf

I'm not seeing a `.smithy.lsp.log` file, any pop-ups or other feedback that the server is running. Using https://github.com/disneystreaming/vscode-smithy/ as the client (which works fine with previous versions, including 0.2.4 from this repo).

milesziemer commented 3 months ago

@kubukoz Thanks for trying it out. I think the reason it's seemingly not working is because the smithy-build.json doesn't define any sources or imports. The lack of feedback from the server that this is the case is a really bad experience I see. I think before this PR can be considered ready I need to add support for a detached project which loads whatever you open, and/or have some indication through a diagnostic if a smithy file isn't connected to the project.

The PR description has more info on this:

From the end user's perspective, only one major change has been made what the language server can do. It now uses a smithy-build.json in the root of the workspace as the source of truth for what is part of the project. Previously, the server loaded all Smithy files it found in any subdir of the root. This doesn't scale well with multi-root workspaces, and leads to an issue where Smithy files in the build directory are added to the project, duplicating sources. The new process looks for a smithy-build.json and uses only its sources and imports as files to load into the model (maven deps are still supported, this is just referring to project files).

I'm open to suggestions on alternative ways to figure out which files the language server should load as part of the project. This method works essentially the same as how the smithy-cli works, only going off of smithy-build.json (or paths passed as arguments in the case of the cli). The intent is to make the language server load the project the same way the build tool would. If the build tool doesn't use smithy-build.json at all, or doesn't use it as its source of truth for what files exist in the smithy project, that's where the .smithy-project.json comes in.

kubukoz commented 3 months ago

Interesting, it didn't work for me in a project with imports either. I'll try again in the minimal setup but with sources/imports.

kubukoz commented 3 months ago

ok, the minimal example works with sources. I have a .smithy.lsp.log now, and features like navigation are working.

I made the following next steps:

renamed my imports to sources in The Big Project (with imports the CLI wasn't having it when calling build, I suppose I'd been using that key incorrectly)
successfully ran build in the smithy CLI 1.45.0 (same one as used here), with no errors

and it's still returning empty sets in all the LSP responses 🤔

any ideas?

milesziemer commented 3 months ago

@kubukoz Hmm, could you paste a version of the smithy-build.json where imports didn't work? For The Big Project, is it being opened in the same directory as the smithy-build.json?

milesziemer commented 3 months ago

Also, if you update your client to send

progressOnInitialization: true

in the initialize request, you should get some indication of the progress of the server loading the project. If you're able to paste any server trace, that would be helpful too, but understand if you can't.

milesziemer commented 3 months ago

Update: latest commit adds support for files not attached to a smithy-build.json or other build file: https://github.com/smithy-lang/smithy-language-server/pull/146/commits/6fce052ee3f7e4825da50f2630d37e24718a6091

kubukoz commented 2 months ago

I've got it working, sources was using "./smithy" rather than "smithy". The former worked in the CLI so I assumed it'd work here 😅

some more thoughts/problems/suggestions:

looks like shape references in traits/metadata don't have navigation anymore. for example, metadata foo = [smithy.api#String] -> that would be nice to fix, but it's absolutely fine to leave it out for another PR. It also doesn't seem to happen in trait nodes, e.g. protocol definitions.
smithy-language-server version 0.3.0 started. is sent as window/logMessage, I suggest turning it into a showMessage as it was before: it's useful to see it as a notification to confirm things are working. "project loaded with N jars" was also very helpful on (re)loads
it's nice to see navigation working in errors: and inline I/O :)

kubukoz commented 2 months ago

Seeing this guy too:

[Error - 23:12:15] Request textDocument/documentSymbol failed.
Error: name must not be falsy
    at Function.validate (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:150:17471)
    at new d (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:150:17766)
    at asDocumentSymbol (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/protocolConverter.js:604:22)
    at convertBatch (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/utils/async.js:193:25)
    at Object.map (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/utils/async.js:202:17)
    at Object.asDocumentSymbols (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/protocolConverter.js:601:22)
    at _provideDocumentSymbols (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/documentSymbol.js:83:76)
    at d.provideDocumentSymbols (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:152:90504)

Could be something wrong with the client, I don't think we've updated vscode-languageclient in that client in a while 🤔

kubukoz commented 2 months ago

Another one: metadata seems to be cached between model reloads.

To reproduce, you can simply create a file with e.g. metadata foo = 2, save, then change the value to anything else and save again.

kubukoz commented 2 months ago

Managed to reproduce some more issues... here's a 2-in-one:

https://github.com/smithy-lang/smithy-language-server/assets/894884/fb2ae9a0-3970-40cd-8bd1-65d4161c6d77

For some reason, when creating new files and typing things before adding a namespace, sometimes the LSP refuses to see that the file is eventually valid. It doesn't seem to happen every time I "simply" start typing, though, so probably a race condition is involved.
reported as a formatter bug: https://github.com/smithy-lang/smithy/issues/2261

kubukoz commented 2 months ago

Found a new regression: references aren't found in use clauses. I don't think it's a blocker, but worth noting somewhere.

etspaceman commented 1 month ago

Seems this PR doesn't like the windows paths due to the colons in the beginning. Any thoughts on how we could fix that one @milesziemer ?

etspaceman commented 1 month ago

https://stackoverflow.com/questions/52755854/java-nio-file-invalidpathexception-illegal-char-when-using-paths-get

Seems that we may just need to remove that leading / character on windows machines.

milesziemer commented 1 month ago

@etspaceman Thanks - I've pushed a few updates that address the windows issues in CI.

@kubukoz I think I've pushed fixes for all the issues and regressions you found (and some other issues I found), except for https://github.com/smithy-lang/smithy-language-server/pull/146#issuecomment-2073454212 - I couldn't reproduce but I added a log that will hopefully give us some more info if it happens again (if not, maybe it was just the client). Thanks for the help.

kubukoz commented 1 month ago

Awesome! Looks like Windows support is much better now. I'm looking forward to seeing this merged!

smithy-lang / smithy-language-server

Refactor for performance improvements #146