Copilot commented on code in PR #4005: URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4003112534
########## packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/Emitter.java: ########## @@ -0,0 +1,573 @@ +/* + * 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.drools.formatter; + +import java.util.ArrayList; +import java.util.List; + +import org.antlr.v4.runtime.CommonTokenStream; +import org.antlr.v4.runtime.ParserRuleContext; +import org.antlr.v4.runtime.Token; +import org.antlr.v4.runtime.misc.Interval; +import org.drools.drl.parser.antlr4.DRL10Lexer; +import org.drools.drl.parser.antlr4.DRL10Parser; +import org.drools.drl.parser.antlr4.DRL10ParserHelper; + +/** + * Output side of the formatter: the buffer, indentation depth, hidden-token and + * comment emission, {@code @formatter:off} regions, and the two text extractors + * ({@code styledText} normalizes spacing, {@code sourceSlice} is byte-faithful). + * Every area formatter writes through one instance of this. + * + * <p>Comments and blank lines from the hidden channel are emitted by + * {@code emitHiddenTokensBefore} / {@code emitTrailingHiddenTokens}, which + * walk the token stream between the last emitted index and the current node. + * Block comments are re-indented; line comments are preserved. + * + * <p>Emission is STYLE-NORMALIZING and COMMENT-PRESERVING, not byte-preserving: + * token content and comments always survive, while spacing and the sanctioned + * separator/terminator normalizations (statement {@code ;}, accumulate/groupby + * source separator {@code ,} → {@code ;}) come from the formatter, not the + * source. + * <ul> + * <li>{@code styledText} — the standard emission helper: visible tokens + * of a node (or token span) joined per the house spacing rules + * ({@code needsSpaceBetween}), block comments kept inline, hidden + * whitespace discarded.</li> + * <li>{@code sourceSlice} — exact original text of a node (or token span), + * including comments and inter-token whitespace. Reserved for the sites + * where verbatim IS the contract: frozen regions/{@code emitVerbatim}, + * the unparsed-rule fallback, and the multi-line reflow guards that keep + * a line comment's newline.</li> + * <li>Both advance {@code lastEmittedTokenIndex} so hidden-token emission + * never double-prints what a span already contains.</li> + * <li>{@code singleLine} — collapses newline+indent runs to single spaces + * for one-line emission contexts.</li> + * </ul> + */ +final class Emitter { + + final DRL10Parser parser; + final CommonTokenStream tokens; + final StringBuilder out = new StringBuilder(); + final FormatterOptions options; + final TokenSpacing spacing; + int depth = 0; + int lastEmittedTokenIndex = -1; + int consecutiveNewlines = 0; + + // Source-line ranges (1-based, inclusive) frozen by @formatter:off/on + // markers; top-level statements overlapping these are emitted verbatim. + // Populated once the token stream is filled, via computeSkipRegions(). + List<int[]> skipRegions = List.of(); + + Emitter(String text, FormatterOptions options) { + parser = DRL10ParserHelper.createDrlParser(text); + tokens = (CommonTokenStream) parser.getTokenStream(); + this.options = options; + this.spacing = new TokenSpacing(options); + } + + String result() { + return out.toString(); + } + + // ── output helpers ──────────────────────────────────────────────────── + + String indent() { + return options.indentUnit().repeat(Math.max(0, depth)); + } + + String indent(int offset) { + return options.indentUnit().repeat(Math.max(0, depth + offset)); + } + + void emit(String text) { + out.append(text); + if (!text.isEmpty()) { + consecutiveNewlines = 0; + } + } + + void newline() { + out.append('\n'); + consecutiveNewlines++; + } + + void blankLine() { + if (consecutiveNewlines < 2) { + newline(); + } + } + + /** + * Append text to the last line of output (before any trailing newline). + * Used to attach suffixes like "," to a pattern's closing ")". + */ + void appendToLastLine(String suffix) { + // Strip trailing newline, append suffix. Caller must call newline() afterwards. + int len = out.length(); + while (len > 0 && out.charAt(len - 1) == '\n') { + len--; + } + out.setLength(len); + out.append(suffix); + consecutiveNewlines = 0; + } + + /** + * Emits a header line and its metadata items (annotations, attributes) per + * options.headerMetadata(), leaving depth one deeper than on entry — the + * body's level. Comments between items are emitted before the item they + * precede; under INLINE that places them on their own lines after the header. + */ + void emitHeader(String header, List<? extends ParserRuleContext> items) { + FormatterOptions.HeaderMetadata mode = options.headerMetadata(); + if (mode == FormatterOptions.HeaderMetadata.INLINE) { + StringBuilder line = new StringBuilder(header); + for (ParserRuleContext item : items) { + String text = styledText(item); Review Comment: `styledText(item)` advances `lastEmittedTokenIndex` while the inline header is only being buffered. By the time the second loop calls `emitHiddenTokensBefore`, the cursor is already past every metadata item, so comments between the header and an annotation/attribute (or between items) are silently dropped in `INLINE` mode. Collect the item text without advancing the emission cursor, or buffer and emit those hidden tokens before moving it. ########## packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspServer.java: ########## @@ -441,9 +460,85 @@ public CompletableFuture<InitializeResult> initialize(InitializeParams params) { }); } + textService.setFormatterOptions(formatterOptionsOf(params.getInitializationOptions())); + return CompletableFuture.supplyAsync(() -> initializeResult); } + /** + * Pulls {@code drools.lsp.formatter} through {@code workspace/configuration} and + * registers for an empty configuration change, the pattern LSP 3.17 prescribes: + * "If the server still needs to react to configuration changes (since the server + * caches the result of {@code workspace/configuration} requests) the server should + * register for an empty configuration change using the following registration + * pattern" (LSP 3.17, workspace/configuration). + */ + @Override + public void initialized(InitializedParams params) { + pullFormatterOptions(); + LanguageClient target = client; + if (!clientSupportsConfigurationRegistration || target == null) { + return; + } + Registration registration = new Registration("drools.lsp.didChangeConfiguration", + "workspace/didChangeConfiguration"); + try { + target.registerCapability(new RegistrationParams(List.of(registration))) + .exceptionally(e -> { + logger.log(Level.WARNING, "Client refused to register for configuration " + + "changes — formatter settings will need a restart", e); + return null; + }); + } catch (Exception e) { + logger.log(Level.WARNING, "Client does not implement client/registerCapability", e); + } + } + + CompletableFuture<Void> pullFormatterOptions() { + LanguageClient target = client; + if (!clientProvidesConfiguration || target == null) { + return CompletableFuture.completedFuture(null); + } + ConfigurationItem item = new ConfigurationItem(); + item.setSection("drools.lsp.formatter"); + try { + return target.configuration(new ConfigurationParams(List.of(item))) + .thenAccept(this::applyPulledFormatterOptions) Review Comment: Configuration pulls can be outstanding concurrently: startup and successive `didChangeConfiguration` notifications each call this method without chaining. JSON-RPC responses may complete out of order, allowing an older settings response to overwrite the newest live options. Sequence the pulls or attach a monotonically increasing generation and apply only the latest response. ########## packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java: ########## @@ -0,0 +1,671 @@ +/* + * 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.drools.formatter; + +import java.util.List; + +import org.antlr.v4.runtime.Token; +import org.drools.drl.parser.antlr4.DRL10Lexer; +import org.drools.drl.parser.antlr4.DRL10Parser; + +/** + * Formats a rule's conditions (the when block) and a query's body. + * + * <p>This class walks the CST with typed {@code visit*} methods: + * <ul> + * <li>{@code visitLhs} → {@code visitLhsExpression} dispatches on + * expression type (and/or/unary/enclosed).</li> + * <li>{@code visitLhsUnary} handles patterns, not/exists, accumulate, + * forall, eval, and groupby.</li> + * <li>{@code visitPatternBind} renders a pattern on one line if it fits + * within {@code options.lineLength()} and its constraint parens hold no line + * comment (a {@code //} comment would swallow the rest of a one-line + * rendering); otherwise delegates to {@code emitReflowedPattern} which + * puts each constraint on its own line — trailing comments attached — + * with the closing {@code )} aligned to the opening.</li> + * <li>Parentheses are padded — {@code Person( age > 18 )} — whatever the + * source did, an empty pair staying {@code ()}. The rule is enforced in + * three places because parens reach the output three ways: token-by-token + * emission ({@code needsSpaceBetween}), verbatim RHS code + * ({@code rhsNeedsSpace}), and string-built signatures + * ({@code parenthesized}).</li> + * <li>{@code visitAndDef} emits explicit {@code and} keywords between + * patterns in accumulate/groupby source via a {@code nextPatternPrefix} + * mechanism.</li> + * </ul> + */ +final class LhsFormatter { + private final Emitter e; + + // Prefix to prepend to the next pattern (e.g. "and " between patterns) + private String nextPatternPrefix = ""; + + LhsFormatter(Emitter e) { + this.e = e; + } + + void visitLhs(DRL10Parser.LhsContext ctx) { + e.emitHiddenTokensBefore(ctx); + e.emit(e.indent() + "when"); + e.newline(); + e.depth++; + for (DRL10Parser.LhsExpressionContext expr : ctx.lhsExpression()) { + visitLhsExpression(expr); + } + e.depth--; + } + + void visitLhsExpression(DRL10Parser.LhsExpressionContext ctx) { + e.emitHiddenTokensBefore(ctx); + if (ctx instanceof DRL10Parser.LhsExpressionEnclosedContext enclosed) { + e.emit(e.indent() + consumePatternPrefix() + "("); + e.newline(); + e.depth++; + visitLhsExpression(enclosed.lhsExpression()); + e.depth--; + e.emit(e.indent() + ")"); + e.newline(); + } else if (ctx instanceof DRL10Parser.LhsOrContext orCtx) { + List<DRL10Parser.LhsExpressionContext> exprs = orCtx.lhsExpression(); + for (int i = 0; i < exprs.size(); i++) { + if (i > 0) { + e.emit(e.indent() + "or"); + e.newline(); + } + visitLhsExpression(exprs.get(i)); + } + } else if (ctx instanceof DRL10Parser.LhsAndContext andCtx) { + List<DRL10Parser.LhsExpressionContext> exprs = andCtx.lhsExpression(); + for (int i = 0; i < exprs.size(); i++) { + if (i > 0) { + nextPatternPrefix = "and "; + } + visitLhsExpression(exprs.get(i)); + } + } else if (ctx instanceof DRL10Parser.LhsUnarySingleContext unary) { + visitLhsUnary(unary.lhsUnary()); + } + } + + private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) { + e.emitHiddenTokensBefore(ctx); + // Consume any pending prefix (e.g. "and ") for non-pattern nodes. + // visitPatternBind consumes it itself; other handlers need it here. + if (ctx.lhsPatternBind() != null) { + visitPatternBind(ctx.lhsPatternBind()); + emitTrailingInvocations(ctx); + return; + } + String prefix = consumePatternPrefix(); + if (ctx.lhsExists() != null) { + visitLhsExists(ctx.lhsExists(), prefix); + } else if (ctx.lhsNot() != null) { + visitLhsNot(ctx.lhsNot(), prefix); + } else if (ctx.lhsEval() != null) { + String inner = e.styledText(ctx.lhsEval().conditionalOrExpression()); + e.emit(e.indent() + prefix + + (e.options.parenPadding() ? "eval( " + inner + " )" : "eval(" + inner + ")")); + e.newline(); + } else if (ctx.lhsForall() != null) { + visitLhsForall(ctx.lhsForall()); + } else if (ctx.lhsAccumulate() != null) { + visitLhsAccumulate(ctx.lhsAccumulate()); + } else if (ctx.lhsGroupBy() != null) { + visitLhsGroupBy(ctx.lhsGroupBy()); + } else if (ctx.lhsExpression() != null) { + // parenthesized lhsExpression + e.emit(e.indent() + prefix + "("); + e.newline(); + e.depth++; + visitLhsExpression(ctx.lhsExpression()); + e.depth--; + e.emit(e.indent() + ")"); + e.newline(); + } else if (ctx.conditionalBranch() != null) { + e.emit(e.indent() + prefix + e.styledText(ctx.conditionalBranch())); + e.newline(); + } + // exists/not/eval and a parenthesised group can each carry a trailing + // invocation too; the pattern-bind path above appends its own. + emitTrailingInvocations(ctx); + } + + /** + * The trailing consequence invocations of an {@code lhsUnary}, rendered for + * the end of the element's line. + * + * <p>The grammar hangs these off {@code lhsUnary} as siblings of the element + * they qualify — {@code lhsPatternBind consequenceInvocation*}, + * {@code lhsExists namedConsequenceInvocation?} — so visiting only the + * element drops them. Dropping one is silent: {@code $p : Bar( age > 18 ) if + * ( $p.age > 65 ) do[senior]} loses its guard while the {@code then[senior]} + * block survives, leaving a labelled consequence that can never fire, and the + * output still re-parses, so the output-reparse net cannot see it. + */ + private String trailingInvocations(DRL10Parser.LhsUnaryContext ctx) { + StringBuilder sb = new StringBuilder(); + for (DRL10Parser.ConsequenceInvocationContext invocation : ctx.consequenceInvocation()) { + sb.append(' ').append(e.styledText(invocation)); + } + if (ctx.namedConsequenceInvocation() != null) { + sb.append(' ').append(e.styledText(ctx.namedConsequenceInvocation())); + } + return sb.toString(); + } + + /** + * Attach an {@code lhsUnary}'s trailing invocations to the element just + * emitted. The element may have taken several lines (reflowed, or forced + * multi-line by a comment), so this appends to the emitted text rather than + * to a current-line buffer. + */ + private void emitTrailingInvocations(DRL10Parser.LhsUnaryContext ctx) { + String trailing = trailingInvocations(ctx); + if (trailing.isEmpty()) { + return; + } + e.appendToLastLine(trailing); + e.newline(); + } + + private void visitLhsExists(DRL10Parser.LhsExistsContext ctx, String prefix) { + if (ctx.lhsPatternBind() != null) { + if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "exists ")) { + return; + } + String patternText = formatPatternBind(ctx.lhsPatternBind()); + String line = e.indent() + prefix + "exists " + patternText; + if (line.length() <= e.options.lineLength()) { + e.emit(line); + e.newline(); + } else { + emitReflowedPattern(ctx.lhsPatternBind(), prefix + "exists "); + } + } else if (ctx.lhsExpression() != null) { + e.emit(e.indent() + prefix + "exists("); + e.newline(); + e.depth++; + visitLhsExpression(ctx.lhsExpression()); + e.depth--; + e.emit(e.indent() + ")"); + e.newline(); + } + } + + private void visitLhsNot(DRL10Parser.LhsNotContext ctx, String prefix) { + if (ctx.lhsPatternBind() != null) { + if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "not ")) { + return; + } + String patternText = formatPatternBind(ctx.lhsPatternBind()); + String line = e.indent() + prefix + "not " + patternText; + if (line.length() <= e.options.lineLength()) { + e.emit(line); + e.newline(); + } else { + emitReflowedPattern(ctx.lhsPatternBind(), prefix + "not "); + } + } else if (ctx.lhsExpression() != null) { + e.emit(e.indent() + prefix + "not("); + e.newline(); + e.depth++; + visitLhsExpression(ctx.lhsExpression()); + e.depth--; + e.emit(e.indent() + ")"); + e.newline(); + } + } + + private void visitLhsForall(DRL10Parser.LhsForallContext ctx) { + e.emit(e.indent() + "forall("); + e.newline(); + e.depth++; + for (DRL10Parser.LhsPatternBindContext pb : ctx.lhsPatternBind()) { + visitPatternBind(pb); + } + e.depth--; + e.emit(e.indent() + ")"); + e.newline(); + } + + private void visitLhsAccumulate(DRL10Parser.LhsAccumulateContext ctx) { + String keyword = ctx.DRL_ACC() != null ? "acc" : "accumulate"; + e.emit(e.indent() + keyword + "("); + e.newline(); + e.depth++; + // The grammar accepts "," or ";" after the source pattern, but the DRL + // language reference documents inline accumulate as + // accumulate( <source pattern>; <functions> [;<constraints>] ) + // so a source "," is normalized to the documented ";" form. + visitAndDef(ctx.lhsAndDef()); + e.appendToLastLine(e.options.normalizeTerminators() ? ";" : sourceSeparator(ctx.lhsAndDef())); + e.newline(); + // accumulate functions + List<DRL10Parser.AccumulateFunctionContext> funcs = ctx.accumulateFunction(); + for (int i = 0; i < funcs.size(); i++) { + e.emit(e.indent() + e.styledText(funcs.get(i))); Review Comment: Comments between the accumulate source separator and the first function, or between functions, are skipped here: `styledText` starts at the function's visible token and then advances the global cursor past the preceding hidden tokens without emitting them. The same applies before trailing constraints/`)`. Emit the intervening hidden tokens for each child before advancing the cursor so formatting cannot delete source comments. This issue also appears on line 288 of the same file. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
