[
https://issues.apache.org/jira/browse/GROOVY-12448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131259#comment-18131259
]
ASF GitHub Bot commented on GROOVY-12448:
-----------------------------------------
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.
> Wire Groovy Console and groovysh to the in-process language engine
> ------------------------------------------------------------------
>
> Key: GROOVY-12448
> URL: https://issues.apache.org/jira/browse/GROOVY-12448
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Context
> GROOVY-12411 added groovy-lsp and the in-process GroovyLanguageEngine. Groovy
> Console and groovysh were left unconnected. This change attaches both hosts
> to that engine. Diagnostics and completion for an unsaved buffer do not
> generate class files.
> Scripts still run through GroovyShell in the console and through GroovyEngine
> in groovysh.
> h2. User-visible behavior
> * *Groovy Console.* The editor compiles the unsaved buffer in the background
> and highlights compiler diagnostics. {{Alt+/}} offers compiler-backed
> completions. {{Ctrl+Space}} is left free, because that chord switches the
> input method on Chinese desktops and on macOS. {{Ctrl+/}} is still comment
> and uncomment. A multiline string keeps the color of its opening delimiter:
> single quote, double quote, or slashy. The dollar sign that opens a GString
> expression is not colored as part of the string. A jar or directory added
> from the Script menu is picked up on the next background compile. The compile
> uses the console compiler configuration, including imports and AST
> customizers. Turning Allow interruption on or off edits that configuration,
> and the next compile sees the edit.
> * *groovysh.* Tab completion and tail tips consult the compiler for the
> current buffer: imports, types, methods, and binding variables. A syntax tail
> tip reports diagnostics and does not evaluate the line. A method tail tip
> asks for signatures on the full line, so an unclosed opening parenthesis is
> visible, and uses the previous inspector only when the compiler has none.
> {{/grab}} is still how Maven artifacts are added. The next completion or tail
> tip notices the loader change. Customizers on the compiler configuration
> given to the engine at construction apply to completion and tail tips as well
> as to evaluation. An import typed at the prompt is buffer text, so the next
> completion sees it as part of the script.
> h2. Design
> * groovy-console and groovy-groovysh depend on groovy-lsp with an
> {{implementation}} dependency, not an {{api}} dependency. LSP4J is not part
> of either module's published API. groovy-lsp stays an optional module and is
> not listed directly by groovy-all. Consumers of the console or groovysh still
> receive it transitively.
> * Each host keeps its own adapter and talks to the engine with JDK records.
> Host source does not import LSP4J. The console adapter owns the buffer name
> {{groovy-buffer:console}}. The groovysh adapter owns {{groovy-buffer:repl}}.
> The engine does not assign those names. Both adapters are internal
> implementation details, not an embedding API.
> * Positions are 0-based UTF-16. CR, LF, and CRLF are line breaks. A character
> past the end of a line stays on that line. A zero-width diagnostic at the end
> of the text is painted on the last character.
> * A query that is given the buffer text replaces that text first. Identical
> text keeps the document version, so the snapshot can be reused. Publishing
> the host compiler configuration copies the script base class and the
> compilation customizers into the tooling compile, and drops the cached
> compile on every call, including a repeat of the same instance. Hosts do that
> when the configuration is replaced or edited, not before every query.
> * The console publishes its configuration when the host is created and again
> when the host rebinds. Rebinding covers classpath changes and the Allow
> interruption toggle. Later queries publish the parent loader only.
> * groovysh publishes the engine configuration at construction, and again only
> when the configuration instance itself changes. That field is final.
> Interactive imports are buffer snippets, not edits to an import customizer.
> * The compile fingerprint includes the URLs of a URLClassLoader, so an
> in-place addURL is visible on the next compile.
> * groovysh builds one script for each question: the remembered buffer, a
> declaration for each binding name the buffer does not already introduce, then
> the current line. Those names come from the compiled AST. The walk does not
> enter a closure or a lambda. It skips the generated no-arg run method and a
> static main method whose only parameter is an array.
> * Console coloring uses one lenient lexer walk on the editor thread. It does
> not paint the compiled name-kind overlay, because those kinds share the
> identifier style. Brace matching still reads that lexer token list. groovysh
> keeps its existing nanorc highlighter.
> * Snippet placeholders are removed from completion text before it is
> inserted. groovysh keeps only the first line of that text.
> h2. Out of scope
> * Driving the AST browser from a language-engine snapshot.
> * A public embedding API for either adapter.
> * A separate engine jar that does not contain LSP4J. Feature services inside
> groovy-lsp still use those types, so LSP4J remains a runtime dependency of
> both hosts.
> * Noticing an in-place edit of a customizer that groovysh has already
> published. The shell does not edit that object after construction. The
> console does, and it publishes the configuration again after the edit.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
