Copilot commented on code in PR #3965:
URL:
https://github.com/apache/incubator-kie-tools/pull/3965#discussion_r3842594021
##########
packages/drools-lsp/drools-completion/src/main/java/org/drools/completion/DRLCompletionHelper.java:
##########
@@ -208,6 +250,160 @@ private static List<CompletionItem>
getFieldCompletionItems(DRL10Parser.Compilat
return fieldItems(memberIndex.membersOf(fqcn));
}
+ /**
+ * The dot-separated segments preceding a caret that sits immediately
after a
+ * dot ({@code $ref.order.|} → {@code [$ref, order]}), or {@code null}
when the
+ * caret follows something else. A numeric head is a decimal literal being
+ * typed, not a path.
+ */
+ private static String[] dottedChainBeforeCaret(String text, Position
caret) {
+ if (text == null || caret == null) {
+ return null;
+ }
+ String[] lines = text.split("\n", -1);
+ int row = caret.getLine();
+ if (row < 0 || row >= lines.length) {
+ return null;
+ }
Review Comment:
This splits lines using `\"\\n\"`, which will leave trailing `\"\\r\"`
characters in CRLF documents. Since LSP positions are line/character-based and
typically treat line endings as separators, the `\\r` can skew column-based
indexing and cause dotted-chain detection to fail in Windows-style line
endings. Consider switching to the same line split used elsewhere
(`\"\\r?\\n\"`, `split(\"\\\\R\", -1)`, or a shared utility) to keep
caret-to-line logic consistent.
##########
packages/drools-lsp/drools-completion/src/main/java/org/drools/completion/DRLCompletionHelper.java:
##########
@@ -208,6 +250,160 @@ private static List<CompletionItem>
getFieldCompletionItems(DRL10Parser.Compilat
return fieldItems(memberIndex.membersOf(fqcn));
}
+ /**
+ * The dot-separated segments preceding a caret that sits immediately
after a
+ * dot ({@code $ref.order.|} → {@code [$ref, order]}), or {@code null}
when the
+ * caret follows something else. A numeric head is a decimal literal being
+ * typed, not a path.
+ */
+ private static String[] dottedChainBeforeCaret(String text, Position
caret) {
+ if (text == null || caret == null) {
+ return null;
+ }
+ String[] lines = text.split("\n", -1);
+ int row = caret.getLine();
+ if (row < 0 || row >= lines.length) {
+ return null;
+ }
+ String line = lines[row];
+ int col = Math.min(caret.getCharacter(), line.length());
+ if (col == 0 || line.charAt(col - 1) != '.') {
+ return null;
+ }
+ int start = col - 1;
+ while (start > 0 && isChainChar(line.charAt(start - 1))) {
+ start--;
+ }
+ String path = line.substring(start, col - 1);
+ if (path.isEmpty() || path.endsWith(".") ||
Character.isDigit(path.charAt(0))) {
+ return null;
+ }
+ return path.split("\\.", -1);
+ }
+
+ private static boolean isChainChar(char c) {
+ return Character.isLetterOrDigit(c) || c == '_' || c == '$' || c ==
'.';
+ }
+
+ /**
+ * Member items for a dotted path the caret sits at the end of. The head
is a
+ * binding ({@code $p.}), a field of the enclosing pattern's type ({@code
ref.}
+ * inside {@code Fact(...)}), or a type name ({@code Status.}); the
remaining
+ * segments are fields. Every hop goes through the same walker the
bindings and
+ * hover use, so all three agree on what a path resolves to.
+ */
+ /**
+ * Completion items for the members of the type the chain resolves to, or
+ * {@code null} when the chain's head names no type the document knows — a
+ * qualified name rather than a member access. An empty list means the path
+ * did resolve and simply has no members to offer, which is an answer:
after
+ * a dot nothing but a member is legal.
+ */
+ private static List<CompletionItem> memberItemsForChain(String[] chain,
String text, Position caret,
+
DRL10Parser.CompilationUnitContext compilationUnit,
+ int
caretTokenIndex, ClassIndex classIndex,
+ ClassMemberIndex
memberIndex, Path documentPath,
+ Map<Path, String>
openFiles) {
+ if (compilationUnit == null) {
+ return null;
+ }
+ Map<String, DeclaredType> typeIndex = DRLWorkspaceTypeIndex.build(
+
DRLDeclaredTypeParser.extractFromCompilationUnit(compilationUnit),
documentPath, openFiles);
+
+ String head = chain[0];
+ String rootType;
+ int firstFieldSegment = 1;
+ if (head.startsWith("$")) {
+ rootType = LhsBindingResolver.resolveAt(text,
DRLHoverHelper.positionToOffset(text, caret), typeIndex)
+ .get(head.substring(1));
+ } else if (!head.isEmpty() && Character.isUpperCase(head.charAt(0))) {
+ rootType = head;
+ } else {
+ // A bare lower-case head is a field of the pattern the caret is
in.
+ rootType = enclosingPatternTypeFromText(text,
DRLHoverHelper.positionToOffset(text, caret));
+ firstFieldSegment = 0;
+ }
+ if (rootType == null || rootType.isEmpty()) {
+ // The head names no type the document knows, so this dot is not a
+ // member access at all — a qualified name, most likely.
+ return null;
+ }
+
+ String resolved = rootType.substring(rootType.lastIndexOf('.') + 1);
+ if (firstFieldSegment < chain.length) {
+ String path = String.join(".", Arrays.copyOfRange(chain,
firstFieldSegment, chain.length));
+ resolved = LhsBindingResolver.resolvePath(
+ LhsBindingResolver.typeOrClasspath(resolved, typeIndex),
path, typeIndex);
Review Comment:
`resolved` is always reduced to the simple name of `rootType`. If the
enclosing pattern type is written as a fully-qualified name (e.g.
`org.example.Foo(...)`) without an import, this drops the qualification and can
prevent `memberItemsOfType(...)` from resolving the classpath type for member
completion. Consider keeping `rootType` as FQCN when it contains dots (and only
simplifying when needed for declared-type lookup), so member completion works
with fully-qualified pattern heads.
##########
packages/drools-lsp/drools-completion/src/main/java/org/drools/completion/JavaSourceTypeIndex.java:
##########
@@ -0,0 +1,357 @@
+/*
+ * 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.completion;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.logging.Logger;
+import java.util.stream.Stream;
+
+/**
+ * Indexes {@code .java} source under a workspace's source roots via {@link
+ * JavaSourceTypeParser}, so completion/hover/lint can resolve types and
+ * members before a compile exists. An instance is an immutable snapshot;
+ * callers rebuild via {@link #build} on workspace changes rather than
+ * mutating one in place — the same model {@code ClassIndex} uses.
+ */
+public final class JavaSourceTypeIndex implements JavaMemberSource {
+
+ private static final Logger logger =
Logger.getLogger(JavaSourceTypeIndex.class.getName());
+
+ private static final JavaSourceTypeIndex EMPTY =
+ new JavaSourceTypeIndex(Set.of(), Map.of(), Map.of(), Map.of());
+
+ private static final class CachedEntry {
+ final long modMillis;
+ final List<JavaSourceType> types;
+
+ CachedEntry(long modMillis, List<JavaSourceType> types) {
+ this.modMillis = modMillis;
+ this.types = types;
+ }
+ }
+
+ /** Per-file cache keyed by normalized absolute path, valid by mtime.
Shared across builds. */
+ private static final Map<Path, CachedEntry> FILE_CACHE = new
ConcurrentHashMap<>();
+
+ /**
+ * Drops all cached parse results. Entries are already mtime-validated, so
+ * this is about bounding memory in a long-running server rather than
+ * correctness; call it when the workspace source roots are rebuilt or on
+ * shutdown.
+ */
+ public static void clearCache() {
+ FILE_CACHE.clear();
+ }
+
+ private final Set<Path> roots;
+ private final Map<String, List<String>> classNames;
+ private final Map<String, JavaSourceType> typesByFqcn;
+ private final Map<String, Path> fileByFqcn;
+
+ private JavaSourceTypeIndex(Set<Path> roots, Map<String, List<String>>
classNames,
+ Map<String, JavaSourceType> typesByFqcn,
Map<String, Path> fileByFqcn) {
+ this.roots = roots;
+ this.classNames = classNames;
+ this.typesByFqcn = typesByFqcn;
+ this.fileByFqcn = fileByFqcn;
+ }
+
+ /** An index over no source roots; resolves nothing. */
+ public static JavaSourceTypeIndex empty() {
+ return EMPTY;
+ }
+
+ /**
+ * Walks each of {@code sourceRoots} for {@code .java} files, parses each
+ * (via the mtime cache) into its top-level types, and keeps those whose
+ * package passes {@code packageFilters} — a type's package is its FQCN
+ * minus the last segment; it passes when {@code packageFilters} is empty,
+ * when a filter equals the package exactly, or when a filter ends with
+ * {@code *} and the package starts with the prefix before it. A
+ * default-package type passes only when {@code packageFilters} is empty.
+ * On a duplicate FQCN across roots, the first one seen wins (walk order);
+ * later ones are logged at FINE and dropped.
+ */
+ public static JavaSourceTypeIndex build(Set<Path> sourceRoots,
List<String> packageFilters) {
+ List<String> filters = packageFilters == null ? List.of() :
packageFilters;
+ Set<Path> usedRoots = new LinkedHashSet<>();
+ Map<String, JavaSourceType> typesByFqcn = new LinkedHashMap<>();
+ Map<String, Path> fileByFqcn = new LinkedHashMap<>();
+
+ if (sourceRoots != null) {
+ for (Path root : sourceRoots) {
+ if (root == null || !Files.isDirectory(root)) {
+ continue;
+ }
+ usedRoots.add(root);
+ indexRoot(root, filters, typesByFqcn, fileByFqcn);
+ }
+ }
+
+ // The roots are the one fact needed to explain everything else this
+ // index reports — including duplicate-FQCN drops, which are expected
+ // when two roots legitimately see the same file and alarming
otherwise.
+ if (!usedRoots.isEmpty()) {
+ logger.info("Indexed " + typesByFqcn.size() + " Java source
type(s) from "
Review Comment:
Index rebuilds can be triggered frequently (e.g. `.java` watch events).
Logging this at `INFO` on every rebuild may produce noisy logs and degrade
signal-to-noise in server output. Consider lowering this to `FINE`/`FINEST`,
rate-limiting it, or logging only when the indexed root set changes.
##########
packages/drools-lsp/drools-completion/src/test/java/org/drools/completion/JavaSourceTypeParserTest.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.completion;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Optional;
+import org.junit.jupiter.api.Test;
+
+class JavaSourceTypeParserTest {
+
+ private JavaSourceType only(String src) {
+ List<JavaSourceType> ts = JavaSourceTypeParser.parse(src);
+ assertEquals(1, ts.size(), () -> "expected exactly one top-level type,
got " + ts.size());
+ return ts.get(0);
+ }
+
+ private Optional<Field> member(JavaSourceType t, String name) {
+ return t.members.stream().filter(f -> f.name.equals(name)).findFirst();
+ }
+
+ /**
+ * Reflection lists instance members: it filters static fields out and its
+ * property scan rejects static methods. A static offered as a fact
property
+ * before a build and withdrawn after it is worse than never offering it,
+ * because a rule written against it does not compile.
+ */
+ @Test
+ void staticMembersAreNotFactProperties() {
+ JavaSourceType t = only(
+ "package com.example;\n"
+ + "public class Order {\n"
+ + " public static final String VERSION = \"1\";\n"
+ + " public int id;\n"
+ + " public static String getBuild() { return \"b\"; }\n"
+ + " public String getCode() { return \"c\"; }\n"
+ + "}\n");
+
+ assertTrue(member(t, "id").isPresent(), () -> "members=" + t.members);
+ assertTrue(member(t, "code").isPresent(), () -> "members=" +
t.members);
+ assertTrue(member(t, "VERSION").isEmpty(),
+ () -> "a static field is not a fact property: " + t.members);
+ assertTrue(member(t, "build").isEmpty(),
+ () -> "nor is a static getter: " + t.members);
+ }
+
+ /**
+ * Reflection reports public methods and public constructors only, so the
+ * source view must not offer more than the compiled view will: a member
that
+ * appears before a build and vanishes after it is worse than one that
never
+ * appeared, and hover's Constructors section would otherwise name a
+ * constructor the author cannot call.
+ */
+ @Test
+ void nonPublicGettersAndConstructorsAreNotMembers() {
+ JavaSourceType t = only(
+ "package com.example;\n"
+ + "public class Order {\n"
+ + " public int id;\n"
+ + " private String secret;\n"
+ + " private String getSecret() { return secret; }\n"
+ + " public String getCode() { return \"c\"; }\n"
+ + " private Order() { }\n"
+ + " public Order(int id) { }\n"
+ + "}\n");
+
+ assertTrue(member(t, "id").isPresent(), () -> "members=" + t.members);
+ assertTrue(member(t, "code").isPresent(), () -> "members=" +
t.members);
+ assertTrue(member(t, "secret").isEmpty(),
+ () -> "a non-public getter must not be a member: " + t.members);
+ assertEquals(List.of("Order(int)"), t.constructors);
+ }
+
+ @Test
+ void parsesClassFieldsGettersAndFqcn() {
+ JavaSourceType t = only(
+ "package com.example.model;\n"
+ + "public class Patient {\n"
+ + " private String name;\n"
+ + " public int ageYears;\n"
+ + " public String getName() { return name; }\n"
+ + " public boolean isActive() { return true; }\n"
+ + " public Patient(String name, int ageYears) { }\n"
+ + "}\n");
+ assertEquals("com.example.model.Patient", t.fqcn);
+ assertEquals("Patient", t.simpleName);
+ assertEquals(Field.Origin.GETTER, member(t,
"name").orElseThrow().origin);
+ assertEquals("int", member(t, "ageYears").orElseThrow().type);
+ assertEquals(Field.Origin.GETTER, member(t,
"active").orElseThrow().origin);
+ assertTrue(t.constructors.contains("Patient(String, int)"),
+ () -> "constructors=" + t.constructors);
+ }
+
+ @Test
+ void parsesEnumConstantsWithArgs() {
+ JavaSourceType t = only(
+ "package com.example;\n"
+ + "public enum Severity {\n"
+ + " LOW(1), HIGH(3);\n"
+ + " private final int weight;\n"
+ + " Severity(int weight) { this.weight = weight; }\n"
+ + " public int getWeight() { return weight; }\n"
+ + "}\n");
+ assertTrue(t.isEnum);
+ Field low = member(t, "LOW").orElseThrow();
+ assertEquals(Field.Origin.ENUM_CONSTANT, low.origin);
+ assertEquals("1", low.args);
+ assertEquals("Severity", low.type);
+ assertEquals(Field.Origin.GETTER, member(t,
"weight").orElseThrow().origin);
+ }
+
+ @Test
+ void parsesInterfaceAndRecordNames() {
+ assertEquals("com.example.Repo", only(
+ "package com.example;\npublic interface Repo { String id();
}\n").fqcn);
+ JavaSourceType rec = only(
+ "package com.example;\npublic record Point(int x, int y) { }\n");
+ assertEquals("com.example.Point", rec.fqcn);
+ assertEquals("int", member(rec, "x").orElseThrow().type);
+ assertTrue(rec.constructors.contains("Point(int, int)"),
+ () -> "constructors=" + rec.constructors);
+ }
+
+ @Test
+ void capturesExtendsAndPosition() {
+ JavaSourceType t = only(
+ "package com.example;\n"
+ + "public class Child extends com.example.Parent {\n"
+ + "}\n");
+ assertEquals("Parent", t.extendsSimpleName);
+ assertEquals(1, t.declLine); // 0-based; "public class Child" is line
index 1
+ }
+
+ @Test
+ void toleratesGarbageAndDefaultPackage() {
+ assertTrue(JavaSourceTypeParser.parse("").isEmpty());
+ assertTrue(JavaSourceTypeParser.parse("this is not java {{{").isEmpty()
+ || !JavaSourceTypeParser.parse("this is not java {{{").isEmpty());
// never throws
+ JavaSourceType t = only("public class NoPkg { int x; }");
Review Comment:
The assertion on lines 156–157 is tautological and doesn’t validate behavior
beyond 'no exception', but it calls `parse(...)` twice. Consider replacing it
with a single call plus an `assertDoesNotThrow(...)` (or equivalent) to express
intent without redundant parsing.
--
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]
