[
https://issues.apache.org/jira/browse/GROOVY-12448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131305#comment-18131305
]
ASF GitHub Bot commented on GROOVY-12448:
-----------------------------------------
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.
> 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)
