Copilot commented on code in PR #4005:
URL:
https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4059374663
##########
packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspServer.java:
##########
@@ -441,9 +463,90 @@ 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");
+ int generation = formatterPullGeneration.incrementAndGet();
+ try {
+ return target.configuration(new ConfigurationParams(List.of(item)))
+ .thenAccept(answer -> {
+ if (generation == formatterPullGeneration.get()) {
+ applyPulledFormatterOptions(answer);
Review Comment:
The generation check and option update are not atomic. An older callback can
pass this check, pause while a newer request increments the generation and
applies its answer, then resume and overwrite it with stale settings. Protect
generation increments and this check-and-apply operation with the same lock, or
serialize configuration pulls.
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java:
##########
@@ -0,0 +1,723 @@
+/*
+ * 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) {
+ emitGroup(consumePatternPrefix(), enclosed.lhsExpression());
+ } else if (!prefixKeyword(ctx).isEmpty()) {
+ e.emit(e.indent() + consumePatternPrefix() + prefixKeyword(ctx));
+ e.newline();
+ e.depth++;
+ visitOperands(ctx);
+ e.depth--;
+ } 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());
+ }
+ }
+
+ /**
+ * {@code or} or {@code and} when the element is written in prefix form,
+ * {@code (or A B)}, which is kept rather than rewritten as {@code A or B};
+ * empty otherwise.
+ */
+ private static String prefixKeyword(DRL10Parser.LhsExpressionContext ctx) {
+ int type = ctx.getStart().getType();
+ boolean prefix = (ctx instanceof DRL10Parser.LhsOrContext && type ==
DRL10Lexer.DRL_OR)
+ || (ctx instanceof DRL10Parser.LhsAndContext && type ==
DRL10Lexer.DRL_AND);
+ return prefix ? ctx.getStart().getText() : "";
+ }
+
+ private void visitOperands(DRL10Parser.LhsExpressionContext ctx) {
+ List<DRL10Parser.LhsExpressionContext> operands = ctx instanceof
DRL10Parser.LhsOrContext orCtx
+ ? orCtx.lhsExpression()
+ : ((DRL10Parser.LhsAndContext) ctx).lhsExpression();
+ for (DRL10Parser.LhsExpressionContext operand : operands) {
+ visitLhsExpression(operand);
+ }
+ }
+
+ /**
+ * {@code head(} and {@code )} on their own lines with the element between
+ * them indented; a prefix-form element keeps its keyword on the opening
+ * line — {@code (or}, {@code not(and} — with its operands beneath.
+ */
+ private void emitGroup(String head, DRL10Parser.LhsExpressionContext inner) {
+ String keyword = prefixKeyword(inner);
+ e.emit(e.indent() + head + "(" + keyword);
+ e.newline();
+ e.depth++;
+ if (keyword.isEmpty()) {
+ visitLhsExpression(inner);
+ } else {
+ visitOperands(inner);
+ }
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+
+ private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx.lhsPatternBind() != null) {
+ visitPatternBind(ctx.lhsPatternBind());
+ } else {
+ visitConditionalElement(ctx);
+ }
+ emitTrailingInvocations(ctx);
+ if (ctx.SEMI() != null && isOopath(ctx.lhsPatternBind())) {
+ // Before Drools 10 two OOPath lines without a ";" between them were read
+ // as one path, so this one terminator is the author's to keep.
+ e.appendToLastLine(";");
+ e.newline();
+ }
+ }
+
+ private static boolean isOopath(DRL10Parser.LhsPatternBindContext bind) {
+ return bind != null && bind.lhsPattern().stream().anyMatch(p ->
p.xpathPrimary() != null);
+ }
+
+ private void visitConditionalElement(DRL10Parser.LhsUnaryContext ctx) {
+ // Consume any pending prefix (e.g. "and "): visitPatternBind does this
+ // itself, the other handlers need it here.
+ 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());
Review Comment:
The pending `and ` prefix is consumed before dispatch, but these three
branches discard it because their helpers emit
`forall(`/`accumulate(`/`groupby(` directly. Consequently, a valid condition
such as `A() and accumulate(...)` loses the explicit connective and is then
rejected by the content-preservation gate instead of being formatted. Pass the
prefix into each helper and prepend it to its opening line.
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/RhsFormatter.java:
##########
@@ -0,0 +1,517 @@
+/*
+ * 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.Token;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats consequences (then blocks): token-level, since the parser keeps
them opaque.
+ *
+ * <p>The DRL parser treats {@code then} blocks as opaque text with individual
+ * tokens on a special RHS channel, so formatting works at the token level
+ * rather than the CST level:
+ * <ul>
+ * <li>{@code emitConsequenceBody} collects visible RHS tokens into
+ * per-statement lists, splitting only at newlines where paren depth is 0
+ * (so continuation lines within expanded calls stay together).</li>
+ * <li>{@code emitRhsStatement} tries the compact (single-line) form first.
+ * If it exceeds {@code options.lineLength()}, it delegates to
+ * {@code rhsEmitExpanded}.</li>
+ * <li>{@code rhsEmitExpanded} uses {@code rhsMarkExpanded} to decide which
+ * {@code (} groups need expansion: any group whose compact rendering
+ * (including trailing {@code );}) would exceed the line limit is marked.
+ * Marked groups get one argument per line with the closing {@code )}
+ * on its own line at the same indent as the opening.</li>
+ * </ul>
+ */
+final class RhsFormatter {
+ private final Emitter e;
+
+ // Extra depth added when a case/default label is emitted without an inline
+ // consequence (consequence follows on subsequent lines). Reset to 0 when
+ // the next case/default label or closing '}' is encountered.
+ private int caseLabelDepthBump = 0;
+
+ RhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitRhs(DRL10Parser.RhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "then");
+ e.newline();
+ e.depth++;
+
+ emitConsequenceBody(ctx.consequenceBody());
+
+ for (DRL10Parser.NamedConsequenceContext nc : ctx.namedConsequence()) {
+ e.depth--;
+ // named consequence header: "then[name]"
+ String ncText = nc.RHS_NAMED_CONSEQUENCE_THEN().getText();
+ e.emit(e.indent() + ncText);
+ e.newline();
+ e.depth++;
+ emitConsequenceBody(nc.consequenceBody());
+ }
+
+ e.depth--;
+ }
+
+ private void emitConsequenceBody(DRL10Parser.ConsequenceBodyContext ctx) {
+ if (ctx == null || ctx.getChildCount() == 0) return;
+
+ int startIdx = ctx.getStart().getTokenIndex();
+ int stopIdx = ctx.getStop().getTokenIndex();
+
+ // Collect visible RHS tokens into statements. A statement boundary is
+ // a newline where paren depth is 0 (balanced). Newlines inside parens
+ // are continuation lines and belong to the same statement.
+ List<String> stmtTokens = new ArrayList<>();
+ boolean pendingBlankLine = false;
+ int parenDepth = 0;
+ int lastTokenLine = -1;
+
+ for (int i = startIdx; i <= stopIdx; i++) {
+ Token t = e.tokens.get(i);
+ String text = t.getText();
+
+ if (t.getChannel() == Token.HIDDEN_CHANNEL) {
+ if (text.contains("\n") && parenDepth == 0) {
+ if (!stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+ long nlCount = text.chars().filter(c -> c == '\n').count();
+ if (nlCount > 1) pendingBlankLine = true;
+ }
+ // Newlines inside parens are ignored (tokens collected into same
statement)
+ continue;
+ }
+
+ if (Emitter.isComment(t)) {
+ // A comment sharing its source line with the previous token is a
+ // trailing comment (e.g. "insert( new Foo() ); // audit note") —
+ // flush the statement it trails, then reattach it to that line
+ // instead of dropping it onto a fresh one.
+ boolean trailsPreviousToken = t.getLine() == lastTokenLine &&
parenDepth == 0;
+ if (!stmtTokens.isEmpty() && parenDepth == 0) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+ if (trailsPreviousToken) {
+ e.appendToLastLine(" " + text.trim());
+ e.newline();
+ } else {
+ if (pendingBlankLine) { e.blankLine(); pendingBlankLine = false; }
+ e.emit(e.indent() + text.trim());
+ e.newline();
+ }
+ continue;
+ }
+
+ // Track paren depth for statement boundary detection.
+ // Only count () — curly braces are block delimiters (switch, if, for)
+ // and should not suppress newline-based statement splitting.
+ if (text.equals("(")) parenDepth++;
+ else if (text.equals(")")) parenDepth = Math.max(0, parenDepth - 1);
+
+ // case/default keywords at top level act as statement boundaries so
+ // that multiple cases concatenated on one line are split correctly
+ // (e.g. badly-formatted input: "break; case 2: ...").
+ if (isCaseLabelStart(text) && parenDepth == 0 && !stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+
+ if (pendingBlankLine) { e.blankLine(); pendingBlankLine = false; }
+ stmtTokens.add(text);
+ lastTokenLine = t.getLine();
+ }
+ if (!stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ }
+
+ e.lastEmittedTokenIndex = Math.max(e.lastEmittedTokenIndex, stopIdx);
+ }
+
+ /**
+ * Wrapper around {@link #emitRhsStatement} that manages brace-depth and
+ * case/default label splitting so that switch blocks are formatted as:
+ * <pre>
+ * switch( x )
+ * {
+ * case 1:
+ * value = ...; break;
+ * default:
+ * value = ...;
+ * }
+ * </pre>
+ * Rules:
+ * <ul>
+ * <li>Statement starting with {@code }} → decrement depth before
emit.</li>
+ * <li>Statement ending with {@code {} → increment depth after emit.</li>
+ * <li>Statement starting with {@code case} or {@code default} → split at
+ * the first top-level {@code :}, emit label at current depth, bump
+ * depth by 1, emit consequence, restore depth.</li>
+ * </ul>
+ */
+ private void emitRhsStatementInBlock(List<String> toks) {
+ if (toks.isEmpty()) return;
+
+ String first = toks.get(0);
+ String last = toks.get(toks.size() - 1);
+
+ // Closing brace: undo any case-consequence depth bump, then decrease
brace indent.
+ if (first.equals("}")) {
+ e.depth -= caseLabelDepthBump;
+ caseLabelDepthBump = 0;
+ e.depth--;
Review Comment:
This unwinds the case-label indentation for every statement beginning with
`}`, including a nested `if`, loop, or block inside the current case. After
that inner block closes, the remaining statements in the same case are emitted
one level too shallow; nested switches similarly lose the outer case state.
Track case indentation by the corresponding switch/brace depth and unwind it
only at the next label or the matching switch close.
--
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]
