[
https://issues.apache.org/jira/browse/GROOVY-12448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18129382#comment-18129382
]
ASF GitHub Bot commented on GROOVY-12448:
-----------------------------------------
paulk-asert commented on PR #2976:
URL: https://github.com/apache/groovy/pull/2976#issuecomment-6305052468
Thanks Daniel, there's a lot of good work here, and wiring Console/groovysh
to the engine is the right direction. I went through it in some detail,
including running a couple of probes against the PR head. I don't think it's
ready to merge yet. Here's what I found, most important first.
**Must fix before merge**
1. **`ReflectionParameterNames` (core) pins class loaders.** `DEBUG_NAMES`
is a static `ConcurrentHashMap` keyed strongly by `Class<?>`, and
`Java8.fillParameterNames` now routes through it. So every reflection-backed
ClassNode the compiler builds keeps its class, and that class's loader,
reachable forever. A probe that defines a class in a throwaway loader and calls
`names(...)` on one of its methods shows the loader is never collected; without
the call it is. This is also a behaviour change to the core compiler for all
users, and it makes parameter names differ between the reflection path (real
names) and the ASM decompiler path (`param0`). Could we move it out of this PR?
Either keep it inside groovy-lsp, or give it its own JIRA with a cache that
doesn't hold classes strongly.
2. **The completion repair compile ignores the configured
`CompilerSettings`.** `CompletionService.repairedModule` builds a fresh
`CompilerConfiguration` and compiles to SEMANTIC_ANALYSIS without the disabled
global transforms or `withProcessFlags`. With this buffer:
```groovy
import groovy.transform.ASTTest
@ASTTest(value={ System.setProperty('probe.ran','yes') })
def x = 'abc'
x.
```
`engine.complete(uri, 3, 2)` runs the `@ASTTest` closure, whereas the
normal diagnostics compile of the same buffer (with the trailing `x.` completed
to `x.size()`) does not. `@Grab` would be processed the same way. The repair
compile should go through the same settings path as the main compile.
3. **Console highlighting cost per keystroke.**
`SmartDocumentFilter.parseDocument()` runs synchronously in the DocumentFilter
(so on the EDT). On every insert/remove it re-lexes the whole buffer and resets
the whole document to `defaultStyle`, and `offsetOf()` rescans from offset 0
for each token, so the cost is roughly tokens × length. The old filter diffed
token lists and restyled only the changed tokens. Tracking a running offset
would make it linear, and restricting the reset to the dirty range would help
too. Related: those whole-document `setCharacterAttributes` calls are undoable
edits, so the undo manager grows by a full-document attribute edit per
keystroke.
**Please add an opt-out**
Some users will prefer raw typing speed over compiler assistance, and at the
moment they can only partly turn it off:
- **Console:** View → Smart Highlighter falls back to `GroovyFilter`, but
`installLanguageHost()` attaches `ConsoleLanguageHost` unconditionally. The
background diagnostic compile after each typing pause, and the compile on
File→Open, always happen.
- **groovysh:** tail-tips are off by default (Alt-S), but `Main` always
installs `GroovyshLanguageSession` as a completer, so every Tab compiles the
remembered buffer. `noSyntaxCheck` is no longer read on the new SYNTAX path
either.
Could we have a Console preference/menu item that skips attaching the
language host, and a groovysh option (reviving `noSyntaxCheck`, or a new one)
that skips the language session? In both cases it should fall back to the
previous behaviour (regex highlighting / the existing `scriptCompleter` and
Inspector).
**Should fix**
- Each `SmartDocumentFilter` opens its own `GroovyLanguageClient`/server
(with its own compile executor thread), and nothing closes it: not
`Console.exit()`, and not toggling the smart highlighter, which builds a new
filter. Embedders opening several Console windows leak one per window/toggle.
- `ConsoleLanguageHost.refresh()` compiles on the calling thread, so
File→Open now blocks the EDT for a full compile.
- `GroovyLanguageClient`: `version`, `sent` and `diagnostics` are
unsynchronized but used from both the EDT and the debounce thread. A load
overlapping a pending compile can leave the client and server disagreeing about
the current text.
- groovysh: completion and tail-tips compile synchronously on the JLine
thread with no timeout or cancellation. With tail-tips on, METHOD fires on
every keystroke inside `(`, and each one is a full compile of the remembered
buffer.
- `withProcessFlags` flips JVM-wide
`groovy.asttest.enable`/`groovy.grape.enable`. That was tolerable in a
standalone server, but in-process it can race the host's own compiles (e.g. a
Console run while a background compile is in flight).
- Completion shortcut (matrei's comment): `isCompletionChord` requires
`VK_SLASH` and rejects Shift, so on Swedish/German layouts there's no way to
trigger it. A configurable binding would solve both that and the Ctrl+Space
question.
- Host config forwarding is partial: only classpath, plain/star imports and
script base class. Static/aliased imports and AST customizers aren't forwarded,
but the adoc says "imports and AST customizers". Reading `ImportCustomizer`
private fields by reflection is also fragile; it would be better to add an
accessor in core if we need it.
- API surface/compat:
- `SmartDocumentFilter` is public since 3.0, and this removes
`HIGHLIGHTED_TOKEN_TYPE_LIST` and changes `getLatestTokenList()`'s return type.
- `GroovyLanguageClient` is public in a non-internal package.
- `GroovyLanguageEngine` gains a lot of public methods the hosts don't use.
Can we trim/narrow these?
- Packaging: groovysh no longer has groovy-console on its compile classpath,
so a Maven user depending on groovy-groovysh alone loses `/console`
(gracefully, but silently). groovysh and groovy-console now always pull
groovy-lsp + lsp4j + gson, so groovy-lsp is optional in name only. Worth a line
in the release notes at least.
**Merge conflict**
There's one conflict in `Console.groovy` with GROOVY-12436 (key bindings
preserved across theme switch). It's mechanical, but note the PR removes
`import java.awt.Container`, which master's
`updateUIPreservingKeyBindings`/`saveKeyBindings` still use, so resolving just
the marked hunk won't compile. (Alt+/ itself survives a theme switch since it's
a KeyListener, so it doesn't need the 12436 mechanism.)
The Copilot points on `positionOf`/`offsetOf` line handling look fixed,
thanks. Happy to look again once the first three are addressed; splitting the
core `ReflectionParameterNames` change out would also make this easier to
review.
> 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
(v8.20.10#820010)
