Build file diagnostics #188
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit makes the language server send diagnostics back to build
files, i.e. smithy-build.json. Previously, any issues in build files
would result in the project failing to load, and those errors would be
reported to the client's window. With this change, issues are recomputed
on change, and sent back as diagnostics so you get squigglies. Much
better.
To accomplish this, a number of changes were needed:
1. Reparse build files on change. Previously, we just updated the
document.
2. Have a way to run some sort of validation on build files that can
tolerate errors. This is split into two parts:
1. A regular validation stage that takes the parsed build file and
tries to map it to its specific java representation, collecting
any errors that occur. For example, smithy-build.json is turned
into SmithyBuildConfig.
2. A resolution stage that takes the java representation and tries to
resolve maven dependencies, and recursively find all model paths
from sources and imports.
3. Keep track of events emitted from this validation so they can be sent
back to the client.
2 is the most complicated part. SmithyBuildConfig does some extra work
under the hood when it is deserialized from a Node, like environment
variable replacement. I wanted to make sure there wasn't any drift
between the language server and other Smithy tools, so I kept using
SmithyBuildConfig::fromNode, but now any exception thrown from this will
be mapped to a validation event. Each of the other build files work the
same way. I also kept the same merging logic for aggregating config from
multiple build files.
Next is the resolution part. Maven resolution can fail in multiple ways.
We have to try to map any exceptions back to a source location, because
we don't have access to the original source locations. For finding
source/import files, I wanted to be able to report when files aren't
found (this also helps to make sure assembling the model doesn't fail
due to files not being found), so we have to do the same thing (that is,
map back to a source location). Resolution in general is expensive, as
it could be hitting maven central, but doing this mapping could also be
expensive, so we don't perform the resolution step when build files
change - only when a project is actually loaded. We will have to see how
this validation feels, and make improvements where necessary.
Moved some things around. ProjectConfig no longer stores BuildFiles or validation events. Those are stored on the Project that owns them.
When parsing a node in the idl, commas are treated as whitespace, but they aren't for json.
yefrig
approved these changes
Jan 22, 2025
There was a problem hiding this comment.
LGTM however when testing in Neovim the experience was less than ideal since currently as setup in nvim-lspconfig, the language server is only started and active in .smithy files. In order to actually test the changes I had to update the config for the language server to run in .json filetypes (which is not ideal).
src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java
Outdated
Show resolved
Hide resolved
| .id("DependencyResolver") | ||
| .severity(Severity.ERROR) | ||
| .message("Dependency resolution failed: " + message) | ||
| .sourceLocation(memberNameNode) |
There was a problem hiding this comment.
would it be possible to make the source location of this validation event on the exact dependency member in the array?
There was a problem hiding this comment.
There might be a smarter way to do this. It's tough because we don't have access to the original source location (at least not directly), and DependencyResolverException doesn't have any extra info we can use to figure out what the actual error is. I might be able to poke at it a bit though and see what it does. Revisiting this I definitely don't like how, unlike the other diagnostics, these are all piled up on the "maven" key - it could be confusing.
src/test/java/software/amazon/smithy/lsp/project/ProjectConfigTest.java
Outdated
Show resolved
Hide resolved
joewyz
pushed a commit
to joewyz/smithy-language-server
that referenced
this pull request
Feb 13, 2025
This commit makes the language server send diagnostics back to build
files, i.e. smithy-build.json. Previously, any issues in build files
would result in the project failing to load, and those errors would be
reported to the client's window. With this change, issues are recomputed
on change, and sent back as diagnostics so you get squigglies. Much
better.
To accomplish this, a number of changes were needed:
1. Reparse build files on change. Previously, we just updated the
document.
2. Have a way to run some sort of validation on build files that can
tolerate errors. This is split into two parts:
1. A regular validation stage that takes the parsed build file and
tries to map it to its specific java representation, collecting
any errors that occur. For example, smithy-build.json is turned
into SmithyBuildConfig.
2. A resolution stage that takes the java representation and tries to
resolve maven dependencies, and recursively find all model paths
from sources and imports.
3. Keep track of events emitted from this validation so they can be sent
back to the client.
2 is the most complicated part. SmithyBuildConfig does some extra work
under the hood when it is deserialized from a Node, like environment
variable replacement. I wanted to make sure there wasn't any drift
between the language server and other Smithy tools, so I kept using
SmithyBuildConfig::fromNode, but now any exception thrown from this will
be mapped to a validation event. Each of the other build files work the
same way. I also kept the same merging logic for aggregating config from
multiple build files.
Next is the resolution part. Maven resolution can fail in multiple ways.
We have to try to map any exceptions back to a source location, because
we don't have access to the original source locations. For finding
source/import files, I wanted to be able to report when files aren't
found (this also helps to make sure assembling the model doesn't fail
due to files not being found), so we have to do the same thing (that is,
map back to a source location). Resolution in general is expensive, as
it could be hitting maven central, but doing this mapping could also be
expensive, so we don't perform the resolution step when build files
change - only when a project is actually loaded. We will have to see how
this validation feels, and make improvements where necessary.
Additional changes:
- Report Smithy's json parse errors
- Added a 'use-smithy-build' diagnostic to 'legacy' build files
- Fix json node parsing to properly handle commas in the IDL vs actual
json
milesziemer
added a commit
to milesziemer/smithy-language-server
that referenced
this pull request
Feb 14, 2025
smithy-lang#168 started tracking build file changes via lifecycle methods, didOpen, etc. But it didn't make a distinction between what was a build file, and what was a Smithy file. There are two paths didOpen can take - the first is when the file being opened is known to be part of a project. In this case, the file is already tracked by its owning Project, so its basically a no-op. The second path is when the file does not belong to any project, in which case we created a "detached" project, which is a project with no build files and just a single Smithy file. So if you opened a build file that wasn't known to be part of a Project, the language server tried to make a detached project containing the build file _as a smithy file_. This is obviously wrong, but wasn't observable to clients AFAICT because clients weren't set up to send requests to the server for build files (specifically, you wouldn't get diagnostics or anything, only for .smithy files). However, recent commits, including smithy-lang#188, now want to provide language support for smithy-build.json. In testing these new commits with local changes to have VSCode send requests for smithy-build.json, the issue could be observed. Specifically, the issue happens when you open a new smithy-build.json before we receive the didChangeWatchedFiles notification that tells us a new build file was created. didChangeWatchedFiles is the way we actually updated the state of projects to include new build files, or create new Projects. Since we can receive didOpen for a build file before didChangeWatchedFiles, we needed to do something with the build file on didOpen. This commit addresses the problem by adding a new Project type, `UNRESOLVED`, which is a project containing a single build file that no existing projects are aware of. We do this by modifying the didOpen path when the file isn't known to any project, checking if it is a build file using a PathMatcher, and if it is, creating an unresolved project for it. Then, when we load projects following a didChangeWatchedFiles, we just drop any unresolved projects with the same path as any of the build files in the newly loaded projects (see ServerState::resolveProjects). I also made some (upgrades?) to FilePatterns to better handle the discrepancy between matching behavior of PathMatchers and clients (see smithy-lang#191). Now there are (private) `*PatternOptions` enums that FilePatterns uses to configure the pattern for different use cases. For example, the new FilePatterns::getSmithyFileWatchPathMatchers provides a list of PathMatchers which should match the same paths as the _watcher_ patterns we send back to clients, which is useful for testing.
milesziemer
added a commit
that referenced
this pull request
Feb 14, 2025
#168 started tracking build file changes via lifecycle methods, didOpen, etc. But it didn't make a distinction between what was a build file, and what was a Smithy file. There are two paths didOpen can take - the first is when the file being opened is known to be part of a project. In this case, the file is already tracked by its owning Project, so its basically a no-op. The second path is when the file does not belong to any project, in which case we created a "detached" project, which is a project with no build files and just a single Smithy file. So if you opened a build file that wasn't known to be part of a Project, the language server tried to make a detached project containing the build file as a smithy file. This is obviously wrong, but wasn't observable to clients AFAICT because clients weren't set up to send requests to the server for build files (specifically, you wouldn't get diagnostics or anything, only for .smithy files). However, recent commits, including #188, now want to provide language support for smithy-build.json. In testing these new commits with local changes to have VSCode send requests for smithy-build.json, the issue could be observed. Specifically, the issue happens when you open a new smithy-build.json before we receive the didChangeWatchedFiles notification that tells us a new build file was created. didChangeWatchedFiles is the way we actually updated the state of projects to include new build files, or create new Projects. Since we can receive didOpen for a build file before didChangeWatchedFiles, we needed to do something with the build file on didOpen. This commit addresses the problem by adding a new Project type, UNRESOLVED, which is a project containing a single build file that no existing projects are aware of. We do this by modifying the didOpen path when the file isn't known to any project, checking if it is a build file using a PathMatcher, and if it is, creating an unresolved project for it. Then, when we load projects following a didChangeWatchedFiles, we just drop any unresolved projects with the same path as any of the build files in the newly loaded projects (see ServerState::resolveProjects). I also made some (upgrades?) to FilePatterns to better handle the discrepancy between matching behavior of PathMatchers and clients (see #191). Now there are (private) *PatternOptions enums that FilePatterns uses to configure the pattern for different use cases. For example, the new FilePatterns::getSmithyFileWatchPathMatchers provides a list of PathMatchers which should match the same paths as the watcher patterns we send back to clients, which is useful for testing. I also fixed an issue where parsing an empty build file would cause an NPE when trying to map validation events to ranges. Document::rangeBetween would return null if the document was empty, but I wasn't checking for that in ToSmithyNode (which creates parse events). The reason the range is null is because Document.lineOfIndex returns oob for an index of 0 into an empty document. Makes sense, as an empty document has no lines. I updated a DocumentTest to clarify this behavior. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
#17
Description of changes:
This commit makes the language server send diagnostics back to build
files, i.e. smithy-build.json. Previously, any issues in build files
would result in the project failing to load, and those errors would be
reported to the client's window. With this change, issues are recomputed
on change, and sent back as diagnostics so you get squigglies. Much
better.
To accomplish this, a number of changes were needed:
document.
tolerate errors. This is split into two parts:
tries to map it to its specific java representation, collecting
any errors that occur. For example, smithy-build.json is turned
into SmithyBuildConfig.
resolve maven dependencies, and recursively find all model paths
from sources and imports.
back to the client.
2 is the most complicated part. SmithyBuildConfig does some extra work
under the hood when it is deserialized from a Node, like environment
variable replacement. I wanted to make sure there wasn't any drift
between the language server and other Smithy tools, so I kept using
SmithyBuildConfig::fromNode, but now any exception thrown from this will
be mapped to a validation event. Each of the other build files work the
same way. I also kept the same merging logic for aggregating config from
multiple build files.
Next is the resolution part. Maven resolution can fail in multiple ways.
We have to try to map any exceptions back to a source location, because
we don't have access to the original source locations. For finding
source/import files, I wanted to be able to report when files aren't
found (this also helps to make sure assembling the model doesn't fail
due to files not being found), so we have to do the same thing (that is,
map back to a source location). Resolution in general is expensive, as
it could be hitting maven central, but doing this mapping could also be
expensive, so we don't perform the resolution step when build files
change - only when a project is actually loaded. We will have to see how
this validation feels, and make improvements where necessary.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.