Make the LSP more robust against invalid projects#16
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the LSP’s resilience when opening invalid or broken Gradle projects by preventing unhandled exceptions in TAPI calls and enabling on-the-fly resynchronization when settings.gradle[.kts] files change.
- Introduce
DeclarativeModelStoreto wrap all Tooling API interactions in a failure-safe store. - Refactor text/workspace services and tests to use the new model store and a simplified
MutationRegistry. - Add a
didSavehandler that triggersupdateModel()on settings file saves and cover with end-to-end tests.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lsp/src/test/kotlin/.../VersionedDocumentStoreTest.kt | Added VersionedDocumentStore import to exercise the relocated class. |
| lsp/src/test/kotlin/.../AbstractDeclarativeTextDocumentServiceTest.kt | Switched test helpers to return DeclarativeModelStore and simplified MutationRegistry constructor. |
| lsp/src/main/kotlin/.../valueFactoryIndex.kt | Removed obsolete import now that VersionedDocumentStore lives in org.gradle.declarative.lsp. |
| lsp/src/main/kotlin/.../mutation/MutationRegistry.kt | Moved class to new package, dropped direct dependency on DeclarativeResourcesModel. |
| lsp/src/main/kotlin/.../extension/DeclarativeResourcesModelExtensions.kt | Added helper extension to build a SimpleAnalysisEvaluator from a DeclarativeResourcesModel. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/VersionedDocumentStore.kt | Changed package from .service to root org.gradle.declarative.lsp. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/ToolingApiConnector.kt | Replaced TapiConnectionHandler with a failure-safe ToolingApiConnector.withToolingApi helper. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/Main.kt | Extracted startDeclarativeLanguageServer for better I/O injection and testability. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeWorkspaceService.kt | Updated to accept DeclarativeModelStore and removed legacy VersionedDocumentStore imports. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeTextDocumentService.kt | Switched to use DeclarativeModelStore.ifAvailable, added settings-file didSave reload logic. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeModelStore.kt | New class managing DeclarativeResourcesModel retrieval, caching, and safe reloads. |
| lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeLanguageServer.kt | Refactored to wire up DeclarativeModelStore, defined MutationRegistry entries, and added isInitialized. |
| lsp/src/integTest/InvalidGradleProjectTests.kt | Two new end-to-end tests verifying server doesn’t crash on invalid settings and recovers on fix. |
| lsp/src/integTest/AbstractEndToEndTest.kt | New base for integration tests with project-dir init helper. |
| lsp/build.gradle.kts | Added Kotlin serialization plugin, switched many dependencies to api, configured integration tests. |
| gradle/libs.versions.toml | Bumped Kotlin and added kotlinx-serialization-json entries. |
Comments suppressed due to low confidence (1)
lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeLanguageServer.kt:176
- [nitpick] The method
isInitializedreports only model availability, which might be confused with server initialization state. Consider renaming it toisModelAvailableor similar to clarify its intent.
fun isInitialized(): Boolean {
lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeTextDocumentService.kt
Outdated
Show resolved
Hide resolved
| api(libs.lsp4j) | ||
| api(libs.gradle.tooling.api) | ||
| api(libs.gradle.declarative.dsl.api) | ||
| api(libs.gradle.declarative.dsl.core) | ||
| api(libs.gradle.declarative.dsl.evaluator) | ||
| api(libs.gradle.declarative.dsl.tooling.models) | ||
| api(libs.logback.classic) |
There was a problem hiding this comment.
ℹ️ These were moved to the api configuration, as test-suites wouldn't inherit implementation dependencies.
I think it makes sense, as all of these are exposed in our API.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
eskatos
left a comment
There was a problem hiding this comment.
It's good to see some robustness!
I requested a few minor changes and asked some questions.
| parentFile.mkdirs() | ||
| writeText( | ||
| """ | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.14.2-bin.zip |
There was a problem hiding this comment.
💅 The Gradle version used in tests should probably be parameterized, eventually. I understand that this is out of scope for this PR but I think it would be nice to at least extract the version number as a prominent constant in order to make it more obvious.
There was a problem hiding this comment.
WDYT, what should be the parametrization source?
There was a problem hiding this comment.
There are options like:
- a list of tested Gradle versions hardcoded in the base test
- a list of tested Gradle versions passed from the build definition
- a test task per tested Gradle version
But my point was that this can actually happen later, it doesn't have to be part of this PR.
I think it would be nice to at least extract the version number as a prominent constant in order to make it more obvious.
lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeTextDocumentService.kt
Outdated
Show resolved
Hide resolved
lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeTextDocumentService.kt
Outdated
Show resolved
Hide resolved
- merge model store and TAPI handling - improve mocking and add extra tests
| message = "The build files have been changed. Declarative model might be out of sync." | ||
| actions = mutableListOf( | ||
| MessageActionItem("Resync"), | ||
| MessageActionItem("Ignore") |
lsp/src/main/kotlin/org/gradle/declarative/lsp/DeclarativeTextDocumentService.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # gradle/libs.versions.toml
Currently, the LSP breaks when opened up with an invalid project (#14).
This can simply mean a typo in a
settings.gradle[.kts]file.This is quite bad, as it's completely expected that projects could (and will be) in a transitional state.
This PR solves this problem by:
**/settings.gradle[.kts]file)