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