paulk-asert commented on PR #2976: URL: https://github.com/apache/groovy/pull/2976#issuecomment-6378591688
Thanks Daniel — this is a big step forward. I went through the head (`6624bbe2d7`) against each point from my last comment, plus a fresh read of the core and groovy-lsp changes. Two of the three must-fixes are done, and most of the should-fixes too. What's left is a short list; three items are regressions for users who never turn the new features on, so I don't think it's ready to merge quite yet. Details below, most important first. **Verified fixed** - `ReflectionParameterNames` out of core: the core diff is now only `ImportCustomizer` and `ASTTestTransformation`, `Java8` is untouched, and the `WeakHashMap` copy in groovy-lsp holds nothing that references the class. `ParameterNameLeakProbe` is a nice touch. - Repair compile: `compileFragment` → `settings.toConfiguration(host)` under `withProcessFlags`. Good. - `refresh()` off the EDT, client lifecycle, Compiler Assistance preference, configurable completion chord (incl. the Shift+7 case), `groovy.hostConfiguration` passing the object instead of reflecting into `ImportCustomizer`, the `GroovyLanguageClient` lock, the groovysh compile budget — all confirmed in the code. **Must fix before merge** 1. **Hard runtime dependency with no fallback.** `ConsoleTextEditor()` unconditionally does `doc.setDocumentFilter(new SmartDocumentFilter(doc))` (`ConsoleTextEditor.java:234`), which opens a `GroovyLanguageClient` even with both toggles off; and `Main.installLanguageSession` does an unguarded `new GroovyshLanguageSession` (`Main.groovy:516`) inside the outer `catch (Throwable)`, so the shell prints a stack trace and exits 1. A Maven user who excludes `groovy-lsp` (or gson) loses the console *and* groovysh. Both need a `LinkageError` guard that falls back to `GroovyFilter` / the Inspector path with a one-line warning, plus a test for the absent-jar case (there isn't one yet). 2. **The groovysh opt-out can't be reached from the CLI.** `installLanguageSession` runs at `Main.groovy:384`; `groovysh_init.groovy` — the only place a CLI user can set `GROOVY_OPTIONS.noSyntaxCheck` — runs at `:410`. So the knob documented at `groovysh.adoc:71` is read before it can be set, and the session never re-reads it. 3. **Highlighting cost is only half fixed.** Offsets are linear now and the undo-listener detach is correct, thanks. But the "paint only what changed" diff is keyed on exact `(start, end, style)` with a linear `containsSame` scan (`SmartDocumentFilter.java:271-282`), and `renderRange` is only set by Tab/Shift-Tab, so every ordinary keystroke takes the `full` path. A mid-buffer insert shifts every following span, so each one is retired and repainted, with an O(tokens²) membership scan on the EDT; only typing at EOF is cheap. Shifting `painted` by the edit delta first, or a sorted merge against the previous spans, would fix both. 4. **`ASTTestTransformation` thread-local can turn `@ASTTest` on.** `enabled()` returns the override whenever it is set, so `setThreadEnabled(true)` runs `@ASTTest` closures in a JVM started with `-Dgroovy.asttest.enable=false`, which THREAT_MODEL.md documents as making the annotation a no-op — and a remote LSP client can request that via `astTestEnabled`. The override should only be able to disable: `override != null ? override && property : property`. Also: both new methods are public (no modifier in Groovy), have no `@since`, and are only tested from groovy-lsp. 5. **EDT stall on close.** `ConsoleLanguageHost.close()` runs on the EDT (from `Console.exit` and the assistance toggle) and ends in `LanguageServerContext.close()`, which takes `compileLock` (`LanguageServerContext.java:348`) — the lock `recompile()` holds for the whole compile. `shutdownNow` can't interrupt the compiler, so exit or toggle blocks for the rest of a large compile. **Should fix** - `@since 6.0.0` on the `ImportCustomizer` getters and the "Groovy 6 —" heading in `COMPATIBILITY.md` should be 7.0.0 / Groovy 7. `getStarImports()` returns the internal trailing-dot form, which `CompilerSettings.stripTrailingDot` then undoes; better to return plain package names before it's public API. - EOF diagnostic: the JIRA design says a zero-width diagnostic at the end is painted on the last character; the code returns `[length, length]` and `paint` drops it (`ConsoleLanguageHost.groovy:529-531, 1150-1156`), with a test asserting the drop. Either is fine, but ticket and code should agree. - Inspect → CST / Tokens go through `languageHost?.` (`Console.groovy:1449-1456`) and show nothing when Compiler Assistance is off; they used to always work. - groovysh Tab shows duplicates: `scriptCompleter` stays registered alongside the session, so `a.` lists `a.add(` (Inspector, group *Methods*) and `a.add` (session, group *Method*). From code reading, not reproduced interactively. - Synthetic binding declarations: `IDENTIFIER` admits keywords (an embedder binding named `in` or `class` fails every compile); `SYSTEM_VAR` drops ALL-CAPS user variables like `PI`; a value whose class is a purged `ScriptN` yields an unresolvable declaration. - Tests: `ConsoleLanguageWiringTest` leaves the real user Preferences changed (`threadInterrupt=true`, `currentClasspathDir` pointing at a temp dir); several host/session tests rely on `Thread.sleep` or on a cold-engine compile finishing inside the 2 s budget; `completeAtCaretFlushesMenuPath` and `consoleCommandLoadsTheConsoleByName` assert nothing. - Dead code left from the rewrites: `SmartDocumentFilter.offsetOf` (only a reflective test keeps it alive) and the 3-arg `repairedModule`. Sonar flags both. - Docs: the JIRA says "host source does not import LSP4J" and `groovy-lsp/ARCHITECTURE.md:326` says the adapters don't; both hosts import `org.eclipse.lsp4j.CompletionItem`/`Diagnostic`/`SignatureInformation`, which is exactly why `lsp4j` had to become an `api` dependency. The AGENTS.md wording is the accurate one. - `compilerAssistance` is static, but toggling it only re-runs `installLanguageHost()` for the current window. **Sonar / CI** The quality-gate BLOCKER at `CompletionService.java:905` is a false positive: `displayedNames` always sizes its array to `parameters.length`. The remaining ten are smells, two of them the dead methods above. Tests at head: console 218/0, groovysh 226/0, lsp 342/0; clean merge onto current master. Happy to look again once 1–5 are in. If it helps, 1 and 2 are small; 3 is the one worth a quick timing probe on a few-thousand-line buffer before and after. -- 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]
