daniellansun commented on PR #2976: URL: https://github.com/apache/groovy/pull/2976#issuecomment-6380425358
@paulk-asert Thank you for the second pass, and for confirming the earlier fixes (parameter-name cache, repair compile, refresh off the EDT, the completion chord including Swedish Shift+7, host configuration, the client lock, and the groovysh query budget). The five items you marked before merge are in, together with the concrete follow-ups. ## Must-fix 1. **Missing groovy-lsp or Gson.** `ConsoleTextEditor` installs the semantic highlighter inside a `LinkageError` handler and falls back to `GroovyFilter`, with one warning. groovysh does the same around `new GroovyshLanguageSession`: one warning on stderr, Tab stays on the Inspector completer, and the shell does not exit 1. Any other failure during setup still exits 1. Tests cover a thrown `NoClassDefFoundError` for both hosts. 2. **`noSyntaxCheck` and `groovysh_init.groovy`.** The init file runs first. The session is chosen after that, so a CLI user who sets `GROOVY_OPTIONS.noSyntaxCheck` in the init file is honoured. `groovysh.adoc` says so. The embedder callback runs after that choice, so it sees the completer that was actually installed. 3. **Highlighting cost.** Each edit shifts painted spans by `inserted - removed`. Spans that overlap the edit are dropped. The style diff is then one pass over two sorted lists, and clears run before paints. A character typed in the middle of a few-hundred-line buffer restyles that line, not the tail. Attribute edits stay off the undo stack. A test counts `setCharacterAttributes` on a 400-line buffer and bounds the count. 4. **`@ASTTest` thread override.** `enabled()` is `override != null ? override && property : property`. `setThreadEnabled(true)` does not evaluate the closure when `-Dgroovy.asttest.enable=false`. `setThreadEnabled(false)` still turns the annotation off for that thread. Both methods are `@since 7.0.0`. Core tests cover both directions. `THREAT_MODEL.md` states the same rule. 5. **Close on the EDT.** `ConsoleLanguageHost.close()` still detaches listeners on the calling thread. When that thread is the event dispatch thread, `language.close()` (and the wait on `compileLock`) runs on `groovy-console-language-close`. That thread is not a daemon, so the process still releases the client, its class loaders, and its temp output after the window has returned. A test holds the tooling-compile lock and checks that `close()` on the EDT returns within a second. ## Follow-ups from the same comment - `ImportCustomizer` getters are `@since 7.0.0`. `getStarImports()` returns package names without a trailing dot (`java.io`, not `java.io.`). The COMPATIBILITY heading for this work is Groovy 7. - Inspect CST and Inspect Tokens still run when Compiler Assistance is off, through a one-shot client. The View menu toggle updates the host **and** the Compiler Assistance check on every open window. - With the language session installed, Tab registers that session and not the Inspector `scriptCompleter`, so `a.` is not listed twice. `noSyntaxCheck` and the missing-jar path keep the Inspector completer only. - Binding declarations skip keywords (`in`, `class`), keep all-caps user names such as `PI`, and declare a script class — including `ScriptN[][]` — as `Object` with the right number of `[]`. Primitive arrays stay `int[][]`. - `ConsoleLanguageWiringTest` restores `threadInterrupt`, `currentClasspathDir`, and `compilerAssistance`. `SmartDocumentFilter.offsetOf` and the unused 3-argument `repairedModule` are gone. - `groovy-lsp/ARCHITECTURE.md` now matches `AGENTS.md`: the hosts import `CompletionItem`, `Diagnostic`, and `SignatureInformation`, and they do not import `org.apache.groovy.lsp.internal`. ## End-of-buffer diagnostics The console still does not paint a zero-width diagnostic that sits at the end of the buffer. Painting the previous character makes that character look like the error. `groovy-console.adoc` already says this, and the host test still expects `[length, length]`. `GroovyLanguageEngine.spanOf` is a different helper: it paints the last character, and `lsp-userguide.adoc` says so. The two behaviours are each documented with the code that implements them. ## Sonar The quality-gate blocker on `CompletionService.displayedNames` is unchanged. The array length is `parameters.length`; that report is a false positive. The two dead methods called out in the review are removed. ## Tests `:groovy-console:test` (222 passed, 1 skipped) and `:groovy-groovysh:test` (228 passed, 2 skipped) were green, as were the `@ASTTest`, `ImportCustomizer`, and `CompilerCustomizationBuilder` tests and the language-service tests touched here. After the review follow-ups (menu checks, array ranks, `/console` cleanup), those tests and the console and groovysh CodeNarc gates were run again and passed. `lintGate` is clean for the root project, groovy-console, groovy-groovysh, and groovy-lsp. Happy to adjust anything that still looks off. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
