[
https://issues.apache.org/jira/browse/GROOVY-12448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131066#comment-18131066
]
ASF GitHub Bot commented on GROOVY-12448:
-----------------------------------------
daniellansun commented on PR #2976:
URL: https://github.com/apache/groovy/pull/2976#issuecomment-6370869432
@paulk-asert Thank you for the careful read, and for running the probes. The
three items you marked as required are addressed, and so are the opt-out and
the concrete defects under “Should fix”. Notes on each point follow, in the
same order as the comment.
## Must fix before merge
### 1. `ReflectionParameterNames` pinned class loaders
`Java8.fillParameterNames` again uses `Parameter.getName()` only. The core
compiler no longer reads `LocalVariableTable`, so parameter names on the
reflection path and the ASM path are unchanged from `master`.
The name lookup now lives in
`org.apache.groovy.lsp.internal.util.ReflectionParameterNames` and is used only
by completion. The cache is a `WeakHashMap` keyed by `Class`. The value does
not retain the class. `ParameterNameLeakProbe` defines a class in a throwaway
loader, calls `names`, drops the strong references, and the test asserts that
both the class and the loader are collected.
### 2. The completion repair compile ignored `CompilerSettings`
`CompletionService.repairedModule` now compiles through
`GroovyCompiler.compileFragment`. That uses `CompilerSettings.toConfiguration`,
so `@Grab` is listed in `disabledGlobalASTTransformations`, and the same
tolerance and host customizers as a normal tooling compile.
`@ASTTest` is a local transform, so the disabled-global list does not stop
it. `GroovyCompiler.withProcessFlags` no longer writes `groovy.asttest.enable`
or `groovy.grape.enable`. It sets `ASTTestTransformation`’s per-thread override
for the duration of the compile and restores the previous override. Another
thread still sees the JVM property. `CompletionServiceTest` compiles
```groovy
import groovy.transform.ASTTest
@ASTTest(value={ System.setProperty('probe.ran','yes') })
def x = 'abc'
x.
```
With default settings `probe.ran` stays unset. With `astTestEnabled` the
closure runs, so the assertion is not vacuous.
### 3. Console highlighting cost per keystroke
`SmartDocumentFilter` walks semantic-token deltas with a running line start,
so locating tokens is linear in the buffer. A full paint compares the new spans
with the previous ones and calls `setCharacterAttributes` only where the style
changed. Those calls are made with the document’s undo listeners detached, then
the listeners are restored.
`SmartDocumentFilterTest.testAttributeEditsAreNotUndoableAndCloseIsIdempotent`
inserts twice and expects one undoable edit per insert. A partial render range
(the tab actions) still restyles only that range.
## Opt-out
**Console.** `View -> Compiler Assistance` (preference `compilerAssistance`,
default on) skips `installLanguageHost` and closes a host that is already
attached. Diagnostics, completion, and the background compile then stay off.
`View -> Smart Highlighter` is unchanged and still chooses semantic tokens or
`GroovyFilter`.
**groovysh.** `GroovyEngine.isNoSyntaxCheck()` is honored in
`Main.installLanguageSession`. When `noSyntaxCheck` is set, the language
session is not installed as a completer. Tab keeps `scriptCompleter`. Tail-tips
are put back on `GroovyEngine.scriptDescription` (the Inspector path). Leaving
that callback unset would drop tail-tips rather than restore them.
## Should fix
* **Highlighter leak.** `SmartDocumentFilter.close()` shuts its language
client. `ConsoleTextEditor.enableHighLighter` closes the previous filter before
installing another. `Console.exit` closes the highlighter as well as the
language host. A second `close()` is a no-op. `closeHighlighter` does nothing
when the editor was never built.
* **`refresh()` on File → Open.** The document text is read on the EDT. The
compile runs on the caller (the file-load worker, not the EDT). The paint is
applied on the EDT only if this refresh is still the newest one (`generation`),
so a keystroke that finished a newer highlight is not overwritten by the load.
`invokeAndWait` is used only for the read and the paint.
* **`GroovyLanguageClient` races.** `version` and the last sent text are
updated under one lock, and `publishDiagnostics` records under that same lock.
The lock is not held across `recompile()`. Holding it there deadlocks: the
caller waits for the compile lock while `groovy-lsp-compile` holds that lock
and waits to publish diagnostics. Queries (`compile`, completion, hover,
tokens, CST) sync the text first, then call the server.
* **groovysh compile budget.** Tab completion and syntax/method tail-tips go
through one daemon thread. If a compile is already running, the keystroke is
skipped. A compile that exceeds two seconds returns an empty result, and later
keystrokes keep skipping until that compile finishes. The worker is not
interrupted: `cancel(true)` would mark the future done while the compile was
still running, and the interrupt would stick on the pooled thread.
`completeLine` / `diagnosticsFor`, which the tests call, still run directly.
* **`withProcessFlags` and the host.** Covered in item 2. The JVM properties
are left alone. The existing exclusivity test now asserts the thread-local
override and that a second thread does not see it.
* **Completion shortcut.** Default remains Alt+/, so Ctrl+Space stays free
for the input method. `View -> Completion Shortcut...` stores a `KeyStroke`
string (`alt SLASH`, `ctrl SPACE`, `alt shift SLASH`, …) in the
`completionShortcut` preference. Shift is accepted when the stored stroke
includes it. `ConsoleLanguageHostTest.completionChordFollowsThePreference`
covers the default, a shifted slash, and `ctrl SPACE`.
* **Host configuration.** `ImportCustomizer` now exposes `getImports`,
`getStarImports`, `getStaticImports`, `getStaticStarImports`, and `getAliases`.
`getImports()` omits an import whose alias is not the simple class name, so
`import java.util.List as JL` is not also bound as `List`. The client no longer
reflects into private fields. Console and groovysh send the
`CompilerConfiguration` itself with `groovy.hostConfiguration` and do not
repeat those imports as strings. A second string-built customizer would copy an
alias and also bind the simple name. String settings remain for a client that
has no configuration object, with separate keys for static imports and static
stars.
* **API surface.** `SmartDocumentFilter.getLatestTokenList()` returning
delimiter spans, and the removal of `HIGHLIGHTED_TOKEN_TYPE_LIST`, are recorded
under “Groovy 6 — Groovy Console smart highlighter and the language host” in
`COMPATIBILITY.md`. `GroovyLanguageClient` remains `@Internal`.
`GroovyLanguageEngine` stays in an internal package; the hosts do not call it.
I did not delete its query methods: the in-process tests and the engine API use
them, and narrowing them in this pass would be a second, unrelated break.
* **Packaging.** `COMPATIBILITY.md` and `groovysh.adoc` now say that
`groovy-console` and `groovy-groovysh` depend on `groovy-lsp` (and therefore
LSP4J and Gson), and that `/console` from groovysh needs `groovy-console` on
the classpath because groovysh no longer compiles against it. The website
release notes live in another repository; I have not edited that. The in-repo
note is the one above.
## Merge conflict with GROOVY-12436
The rebase onto current `master` is done. `updateUIPreservingKeyBindings`
and `saveKeyBindings` are kept, including `import java.awt.Container`. CST and
token viewers still go through the language host. Alt+/ is a key listener, so
it does not need the JDK 27 key-binding restore.
Happy to adjust anything that still looks wrong.
> 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)
