Copilot commented on code in PR #2976: URL: https://github.com/apache/groovy/pull/2976#discussion_r4242259619
########## subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/language/GroovyshLanguageSession.groovy: ########## @@ -0,0 +1,308 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.groovysh.language + +import groovy.transform.CompileStatic +import groovy.transform.Internal +import groovy.transform.PackageScope +import org.apache.groovy.groovysh.jline.GroovyEngine +import org.apache.groovy.lsp.internal.engine.GroovyLanguageEngine +import org.jline.console.CmdDesc +import org.jline.console.CmdLine +import org.jline.reader.Candidate +import org.jline.reader.Completer +import org.jline.reader.LineReader +import org.jline.reader.ParsedLine +import org.jline.utils.AttributedString + +import java.lang.reflect.Modifier +import java.util.regex.Pattern + +/** + * Compiler-backed session for groovysh. Does not import LSP4J. The adapter + * composes the REPL buffer, binding type declarations, and the current line; + * the engine compiles that synthetic script through semantic analysis. + * Implementation detail of groovysh, not an embedding API. + */ +@CompileStatic +@Internal +final class GroovyshLanguageSession implements AutoCloseable, Completer { + + /** This session's unsaved buffer. The engine does not assign the name. */ + @PackageScope + static final URI BUFFER_URI = URI.create('groovy-buffer:repl') + + private static final Pattern SYSTEM_VAR = Pattern.compile('[A-Z]+[A-Z_]*') + private static final Pattern IDENTIFIER = Pattern.compile('[\\p{L}_$][\\p{L}\\p{N}_$]*') + private static final Set<String> SKIP = ['GROOVYSH_OPTIONS', 'GROOVY_OPTIONS', 'CONSOLE_OPTIONS', + 'ROOT', 'PWD', 'NANORC', '_args'] as Set + + private final GroovyEngine groovy + private final GroovyLanguageEngine engine + private String namesPreamble + private Set<String> namesCache + + GroovyshLanguageSession(GroovyEngine groovy) { + this.groovy = groovy + this.engine = new GroovyLanguageEngine() + engine.setParentLoader(groovy.classLoader) Review Comment: This only forwards the engine class loader, so the language session drops the `CompilerConfiguration` that `GroovyEngine` was constructed with. Embedders are explicitly supported in supplying import/AST customizers (`GroovyEngine.java:216-256`), but code made valid by those customizers will be reported as unresolved by completions and tail-tip diagnostics even though groovysh executes it successfully. The language engine needs to inherit or safely translate the shell's semantic compiler configuration, not just its loader. This issue also appears on line 194 of the same file. ########## subprojects/groovy-lsp/src/main/java/org/apache/groovy/lsp/internal/engine/GroovyLanguageEngine.java: ########## @@ -493,6 +543,114 @@ private static URI requireUri(final URI uri) { return Uris.normalize(uri); } + private static boolean scriptLevelMethod(final MethodNode method) { + if (method == null || method.isSynthetic()) { + return false; + } + String name = method.getName(); + if (name == null || name.isEmpty() || "<init>".equals(name) || "<clinit>".equals(name)) { + return false; + } + if ("run".equals(name) && method.getParameters().length == 0) { + return false; + } + return !"main".equals(name); + } + + private static void addScriptAssignments(final ClassNode script, final Set<String> names) { + MethodNode run = script.getMethod("run", Parameter.EMPTY_ARRAY); + if (run == null || run.getCode() == null) { + return; + } + collectAssignments(run.getCode(), names); + } + + private static void collectAssignments(final Statement statement, final Set<String> names) { + if (statement instanceof BlockStatement block) { + for (Statement nested : block.getStatements()) { + collectAssignments(nested, names); + } + return; + } + if (!(statement instanceof ExpressionStatement expressionStatement)) { + return; + } + Expression expression = expressionStatement.getExpression(); + if (expression instanceof DeclarationExpression declaration) { + addLeftNames(declaration.getLeftExpression(), names); + return; + } + if (expression instanceof BinaryExpression binary + && binary.getOperation() != null + && binary.getOperation().getType() == Types.ASSIGN) { + addLeftNames(binary.getLeftExpression(), names); + } + } + + private static void addLeftNames(final Expression left, final Set<String> names) { + if (left instanceof VariableExpression variable) { + String name = variable.getName(); + if (name != null && !name.isEmpty()) { + names.add(name); + } + } else if (left instanceof TupleExpression tuple) { + for (Expression element : tuple.getExpressions()) { + addLeftNames(element, names); + } + } + } + + /** + * Maps a UTF-16 document offset to a 0-based line and character. + * + * @param text document text + * @param offset UTF-16 offset + * @return {@code {line, character}} + */ + public static int[] positionOf(final String text, final int offset) { + if (text == null || text.isEmpty() || offset <= 0) { + return new int[] {0, 0}; + } + int capped = Math.min(offset, text.length()); + int line = 0; + int character = 0; + for (int i = 0; i < capped; i++) { + if (text.charAt(i) == '\n') { + line++; + character = 0; + } else { + character++; + } + } Review Comment: The inverse coordinate helper recognizes only `\n`; Groovy source using standalone `\r` line endings is therefore reported entirely on one line, unlike the existing `TextDocument` coordinate conversion which supports `\r`, `\n`, and `\r\n`. Handle CR as a line break and consume the following LF as the same break. ########## subprojects/groovy-console/src/main/groovy/groovy/console/ui/language/ConsoleLanguageHost.groovy: ########## @@ -0,0 +1,385 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.console.ui.language + +import groovy.transform.CompileStatic +import groovy.transform.Internal +import groovy.transform.PackageScope +import org.apache.groovy.lsp.internal.engine.GroovyLanguageEngine + +import javax.swing.JMenuItem +import javax.swing.JPopupMenu +import javax.swing.JTextPane +import javax.swing.SwingUtilities +import javax.swing.event.DocumentEvent +import javax.swing.event.DocumentListener +import javax.swing.text.BadLocationException +import javax.swing.text.DefaultHighlighter +import java.awt.Color +import java.awt.Point +import java.awt.event.InputEvent +import java.awt.event.KeyAdapter +import java.awt.event.KeyEvent +import java.util.concurrent.Executors +import java.util.concurrent.RejectedExecutionException +import java.util.concurrent.ScheduledExecutorService +import java.util.concurrent.ScheduledFuture +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger + +/** + * In-process language host for Groovy Console. Compiles the unsaved editor + * buffer through {@link GroovyLanguageEngine} (no class generation) and + * paints diagnostic highlights; Ctrl+Space offers completions. + * Implementation detail of Groovy Console, not an embedding API. + */ +@CompileStatic +@Internal +final class ConsoleLanguageHost implements AutoCloseable { + + /** This editor's unsaved buffer. The engine does not assign the name. */ + @PackageScope + static final URI BUFFER_URI = URI.create('groovy-buffer:console') + + private static final int DEBOUNCE_MS = 150 + private static final int MAX_COMPLETIONS = 20 + + private final GroovyLanguageEngine engine + private final JTextPane input + private final DocumentListener documentListener + private final KeyAdapter completionKeys + private final DefaultHighlighter.DefaultHighlightPainter errorPainter = + new DefaultHighlighter.DefaultHighlightPainter(new Color(255, 180, 180)) + private final List<Object> highlights = [] + private final AtomicInteger generation = new AtomicInteger() + private final ScheduledExecutorService debounce = Executors.newSingleThreadScheduledExecutor { Runnable r -> + Thread thread = new Thread(r, 'groovy-console-language') + thread.daemon = true + thread + } + private volatile boolean closed + private volatile ClassLoader parent + private ScheduledFuture<?> pending + + private ConsoleLanguageHost(JTextPane input, ClassLoader loader) { + this.input = input + this.parent = loader + this.engine = new GroovyLanguageEngine() + engine.setParentLoader(loader) + engine.openBuffer(BUFFER_URI, 'groovy', textOf(input)) + this.documentListener = new DocumentListener() { + @Override + void insertUpdate(DocumentEvent e) { schedule() } + + @Override + void removeUpdate(DocumentEvent e) { schedule() } + + @Override + void changedUpdate(DocumentEvent e) { } + } + this.completionKeys = new KeyAdapter() { + @Override + void keyPressed(KeyEvent e) { + if (e.keyCode == KeyEvent.VK_SPACE && (e.modifiersEx & InputEvent.CTRL_DOWN_MASK) != 0) { + showCompletions() + e.consume() + } + } + } + if (input?.document != null) { + input.document.addDocumentListener(documentListener) + input.addKeyListener(completionKeys) + } + } + + /** + * @param input editor pane + * @param loader GroovyShell class loader + * @return a host bound to {@code input} + */ + static ConsoleLanguageHost attach(JTextPane input, ClassLoader loader) { + new ConsoleLanguageHost(input, loader) + } + + /** + * Compiles {@code text} immediately and returns diagnostics. + * + * @param text buffer text + * @return hits in 0-based UTF-16 coordinates + */ + @PackageScope + List<GroovyLanguageEngine.Hit> compileAndDiagnostics(String text) { + engine.setParentLoader(parent) + engine.updateBuffer(BUFFER_URI, text) + engine.diagnostics(BUFFER_URI) + } + + /** + * Completions at a 0-based UTF-16 position. + * + * @param text buffer text + * @param line 0-based line + * @param character 0-based character + * @return candidates + */ + @PackageScope + List<GroovyLanguageEngine.Candidate> complete(String text, int line, int character) { + engine.setParentLoader(parent) + engine.updateBuffer(BUFFER_URI, text) + engine.complete(BUFFER_URI, line, character) + } + + /** + * Hover markdown at a 0-based UTF-16 position. + * + * @param text buffer text + * @param line 0-based line + * @param character 0-based character + * @return markdown + */ + @PackageScope + String describe(String text, int line, int character) { + engine.setParentLoader(parent) + engine.updateBuffer(BUFFER_URI, text) + engine.describe(BUFFER_URI, line, character) + } + + /** + * Compiles the current editor text and paints diagnostic highlights + * on the calling thread. Production callers run this on the EDT + * after a file load; typing uses {@link #schedule()} instead. + */ + void refresh() { + if (closed) { + return + } + generation.incrementAndGet() + pending?.cancel(false) + String text = textOf(input) + paint(text, compileAndDiagnostics(text)) + } + + /** + * Offers completions at the caret. Used by Ctrl+Space and tests. + */ + @PackageScope + void completeAtCaret() { + showCompletions() + } + + /** + * Records the GroovyShell class loader after classpath or shell changes + * and schedules a background recompile so highlights catch up without a + * keystroke. + * + * @param loader GroovyShell class loader + */ + void setParentLoader(ClassLoader loader) { + if (closed) { + return + } + this.parent = loader + engine.setParentLoader(loader) + schedule() + } + + @Override + void close() { + if (closed) { + return + } + closed = true + generation.incrementAndGet() + pending?.cancel(false) + if (input != null) { + if (input.document != null) { + input.document.removeDocumentListener(documentListener) + } + input.removeKeyListener(completionKeys) + } + debounce.shutdownNow() + try { + debounce.awaitTermination(1, TimeUnit.SECONDS) + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt() + } + engine.close() + } + + private void schedule() { + if (closed || debounce.isShutdown()) { + return + } + int gen = generation.incrementAndGet() + String text = textOf(input) + pending?.cancel(false) + try { + pending = debounce.schedule({ + try { + if (closed || gen != generation.get()) { + return + } + def hits = compileAndDiagnostics(text) + if (closed || gen != generation.get()) { + return + } + SwingUtilities.invokeLater { + if (!closed && gen == generation.get()) { + paint(text, hits) + } + } + } catch (RuntimeException ignored) { + } + }, DEBOUNCE_MS, TimeUnit.MILLISECONDS) + } catch (RejectedExecutionException ignored) { + } + } + + private void paint(String text, List<GroovyLanguageEngine.Hit> hits) { + def highlighter = input?.highlighter + if (highlighter == null) { + return + } + highlights.each { highlighter.removeHighlight(it) } + highlights.clear() + hits?.each { hit -> + int[] span = highlightSpan(text, hit.startLine(), hit.startCharacter(), + hit.endLine(), hit.endCharacter()) + try { + highlights << highlighter.addHighlight(span[0], span[1], errorPainter) + } catch (BadLocationException ignored) { + } + } + if (hits) { + input.toolTipText = hits.take(3).collect { it.message() }.join('\n') + } else { + input.toolTipText = null + } + } + + private void showCompletions() { + if (closed || input == null || debounce.isShutdown()) { + return + } + String text = textOf(input) + int caret = Math.min(Math.max(input.caretPosition, 0), text.length()) + int[] position = GroovyLanguageEngine.positionOf(text, caret) + int gen = generation.get() + pending?.cancel(false) + try { + pending = debounce.schedule({ + try { + if (closed || gen != generation.get()) { + return + } + def items = complete(text, position[0], position[1]) + SwingUtilities.invokeLater { + if (!closed && gen == generation.get()) { + showMenu(items) + } + } + } catch (RuntimeException ignored) { + } + }, 0, TimeUnit.MILLISECONDS) + } catch (RejectedExecutionException ignored) { + } + } + + private void showMenu(List<GroovyLanguageEngine.Candidate> items) { + if (input == null || !items) { + return + } + try { + def menu = new JPopupMenu() + items.take(MAX_COMPLETIONS).each { item -> + def entry = new JMenuItem(item.label() ?: item.insertText()) + entry.addActionListener { + insertAtCaret(input, plainInsert(item.insertText() ?: item.label())) + } + menu.add(entry) + } + Point point = input.caret.magicCaretPosition ?: new Point(0, 0) + menu.show(input, (int) point.x, (int) point.y + 16) + } catch (RuntimeException ignored) { + } + } + + @PackageScope + static void insertAtCaret(JTextPane pane, String insert) { + if (pane?.document == null) { + return + } + String text = textOf(pane) + String piece = insert ?: '' + if (piece.isEmpty()) { + return + } + int caret = Math.min(Math.max(pane.caretPosition, 0), text.length()) + int start = caret + while (start > 0 && Character.isJavaIdentifierPart(text.charAt(start - 1))) { + start-- + } + int end = caret + while (end < text.length() && Character.isJavaIdentifierPart(text.charAt(end))) { + end++ + } + String prefix = text.substring(start, caret) + try { + if (prefix && piece.startsWith(prefix)) { + if (end > caret) { + pane.document.remove(caret, end - caret) + } + pane.document.insertString(caret, piece.substring(prefix.length()), null) + } else if (start < end) { + pane.document.remove(start, end - start) + pane.document.insertString(start, piece, null) + } else { + pane.document.insertString(caret, piece, null) + } + } catch (BadLocationException ignored) { + } + } + + @PackageScope + static String plainInsert(String insert) { + if (insert == null || insert.isEmpty()) { + return '' + } + insert.replaceAll(/\$\{\d+:([^}]*)\}/, '$1').replaceAll(/\$\d+/, '') + } + + private static String textOf(JTextPane pane) { + pane == null ? '' : (pane.text ?: '') + } + + /** + * Maps a 0-based UTF-16 diagnostic range onto document offsets. + * A zero-width or inverted range becomes one character so the + * highlighter has something to paint. + */ + @PackageScope + static int[] highlightSpan(String text, int startLine, int startCharacter, int endLine, int endCharacter) { + int start = GroovyLanguageEngine.offsetOf(text, startLine, startCharacter) + int end = GroovyLanguageEngine.offsetOf(text, endLine, endCharacter) + if (end <= start) { + end = Math.min(start + 1, text == null ? 0 : text.length()) + } Review Comment: A zero-width diagnostic at EOF remains zero-width: when `start == text.length()`, clamping `start + 1` back to the text length leaves `[start, start]`, so Swing paints nothing. EOF diagnostics are common for incomplete input (for example an unclosed construct); move the start back one character when the non-empty document has no character after the reported position. ########## subprojects/groovy-lsp/src/main/java/org/apache/groovy/lsp/internal/engine/GroovyLanguageEngine.java: ########## @@ -493,6 +543,114 @@ private static URI requireUri(final URI uri) { return Uris.normalize(uri); } + private static boolean scriptLevelMethod(final MethodNode method) { + if (method == null || method.isSynthetic()) { + return false; + } + String name = method.getName(); + if (name == null || name.isEmpty() || "<init>".equals(name) || "<clinit>".equals(name)) { + return false; + } + if ("run".equals(name) && method.getParameters().length == 0) { + return false; + } + return !"main".equals(name); + } + + private static void addScriptAssignments(final ClassNode script, final Set<String> names) { + MethodNode run = script.getMethod("run", Parameter.EMPTY_ARRAY); + if (run == null || run.getCode() == null) { + return; + } + collectAssignments(run.getCode(), names); + } + + private static void collectAssignments(final Statement statement, final Set<String> names) { + if (statement instanceof BlockStatement block) { + for (Statement nested : block.getStatements()) { + collectAssignments(nested, names); + } + return; + } + if (!(statement instanceof ExpressionStatement expressionStatement)) { + return; + } + Expression expression = expressionStatement.getExpression(); + if (expression instanceof DeclarationExpression declaration) { + addLeftNames(declaration.getLeftExpression(), names); + return; + } + if (expression instanceof BinaryExpression binary + && binary.getOperation() != null + && binary.getOperation().getType() == Types.ASSIGN) { + addLeftNames(binary.getLeftExpression(), names); + } + } + + private static void addLeftNames(final Expression left, final Set<String> names) { + if (left instanceof VariableExpression variable) { + String name = variable.getName(); + if (name != null && !name.isEmpty()) { + names.add(name); + } + } else if (left instanceof TupleExpression tuple) { + for (Expression element : tuple.getExpressions()) { + addLeftNames(element, names); + } + } + } + + /** + * Maps a UTF-16 document offset to a 0-based line and character. + * + * @param text document text + * @param offset UTF-16 offset + * @return {@code {line, character}} + */ + public static int[] positionOf(final String text, final int offset) { + if (text == null || text.isEmpty() || offset <= 0) { + return new int[] {0, 0}; + } + int capped = Math.min(offset, text.length()); + int line = 0; + int character = 0; + for (int i = 0; i < capped; i++) { + if (text.charAt(i) == '\n') { + line++; + character = 0; + } else { + character++; + } + } + return new int[] {line, character}; + } + + /** + * Maps a 0-based line and character to a UTF-16 document offset. + * + * @param text document text + * @param line 0-based line + * @param character 0-based character + * @return offset in {@code text} + */ + public static int offsetOf(final String text, final int line, final int character) { + if (text == null || text.isEmpty()) { + return 0; + } + int pos = 0; + int current = 0; + int row = Math.max(line, 0); + while (current < row) { + int nl = text.indexOf('\n', pos); + if (nl < 0) { + return text.length(); + } + pos = nl + 1; + current++; + } + return Math.min(pos + Math.max(character, 0), text.length()); + } Review Comment: This clamps the character against the whole document rather than the requested line, so an oversized character on line 0 can map into a later line (the new test even expects line 0/99 in `ab\ncd` to become offset 5). That violates the line/character contract and can paint a diagnostic on unrelated text. Reuse the existing UTF-16-aware `TextDocument.offsetOf`, which also handles CR and CRLF consistently. ########## subprojects/groovy-console/src/main/groovy/groovy/console/ui/Console.groovy: ########## @@ -579,9 +583,19 @@ class Console implements CaretListener, HyperlinkListener, ComponentListener, Fo } installInterceptor() updateTitle() + installLanguageHost() swing.doLater inputArea.&requestFocus } + /** + * Attaches the in-process language host to the editor. groovy-console + * depends on groovy-lsp, so the host is always constructed. + */ + @CompileStatic + private void installLanguageHost() { + languageHost = ConsoleLanguageHost.attach(inputArea, shell.classLoader) + } Review Comment: `Console` can be launched with a customized `baseConfig` (including config-script import/custom AST customizers), but this host receives only `shell.classLoader`. Because `GroovyLanguageEngine` creates a fresh default `CompilerConfiguration`, the editor can highlight valid console code as erroneous and omit completions for imports supplied by the console configuration. Pass or translate the shell's semantic compiler settings into the host as well. -- 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]
