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.
--
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]
