Copilot commented on code in PR #2512:
URL: https://github.com/apache/groovy/pull/2512#discussion_r3182238291
##########
subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/SimpleGroovyClassDoc.java:
##########
@@ -855,8 +857,18 @@ private GroovyClassDoc
resolveInternalClassDocFromImport(GroovyRootDoc rootDoc,
if (isPrimitiveType(baseName)) return null;
for (String importName : importedClassesAndPackages) {
String targetClassName = null;
+ String nestedBaseName = baseName.replace('.', '$');
if (aliases.containsKey(baseName)) {
targetClassName = aliases.get(baseName);
+ } else if (baseName.contains(".")) {
+ int dot = baseName.indexOf('.');
+ String outerName = baseName.substring(0, dot);
+ String nestedSuffix = baseName.substring(dot).replace('.',
'$');
+ if (importName.endsWith("/" + outerName)) {
+ targetClassName = importName + nestedSuffix;
+ } else if (importName.endsWith("/*")) {
+ targetClassName = importName.substring(0,
importName.length() - 1) + nestedBaseName;
+ }
Review Comment:
The nested-type normalization here uses baseName.replace('.', '$') /
nestedSuffix.replace('.', '$'), but this class’s internal nested naming uses
'.' in fullPathName (e.g., fullPathName + "." + innerClassName). Using '$' here
is likely to make classNamedExact(...) miss nested classes. Consider
normalizing nested names to the same '.'-based convention used elsewhere in
this file, or converting importName/baseName into that representation before
lookup.
##########
subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/SimpleGroovyClassDoc.java:
##########
@@ -882,11 +894,23 @@ private GroovyClassDoc
resolveInternalClassDocFromImport(GroovyRootDoc rootDoc,
private GroovyClassDoc
resolveInternalClassDocFromSamePackage(GroovyRootDoc rootDoc, String baseName) {
if (isPrimitiveType(baseName)) return null;
- if (baseName.contains(".")) return null;
int lastSlash = fullPathName.lastIndexOf('/');
if (lastSlash < 0) return null;
String pkg = fullPathName.substring(0, lastSlash + 1);
- return ((SimpleGroovyRootDoc)rootDoc).classNamedExact(pkg + baseName);
+ String candidate = baseName.contains(".") ? baseName.replace('.', '$')
: baseName;
+ return ((SimpleGroovyRootDoc)rootDoc).classNamedExact(pkg + candidate);
+ }
+
+ private GroovyClassDoc
resolveNestedClassDocFromEnclosingTypes(GroovyRootDoc rootDoc, String baseName)
{
+ if (rootDoc == null || fullPathName == null ||
!fullPathName.contains("$")) return null;
+ String nestedSuffix = baseName.replace('.', '$');
+ String current = fullPathName;
+ while (current.contains("$")) {
+ current = current.substring(0, current.lastIndexOf('$'));
+ GroovyClassDoc doc = ((SimpleGroovyRootDoc)
rootDoc).classNamedExact(current + "$" + nestedSuffix);
Review Comment:
resolveNestedClassDocFromEnclosingTypes checks for '$' in fullPathName and
walks enclosing types by lastIndexOf('$'), but this class builds nested class
fullPathName values using '.' separators (e.g., outer + "." + inner). As-is,
this method will never run for normal nested classes and the lookup key
(current + "$" + nestedSuffix) won’t match stored keys. Consider switching this
logic to the '.' nested separator used by fullPathName/classNamedExact.
##########
subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/SimpleGroovyClassDoc.java:
##########
@@ -882,11 +894,23 @@ private GroovyClassDoc
resolveInternalClassDocFromImport(GroovyRootDoc rootDoc,
private GroovyClassDoc
resolveInternalClassDocFromSamePackage(GroovyRootDoc rootDoc, String baseName) {
if (isPrimitiveType(baseName)) return null;
- if (baseName.contains(".")) return null;
int lastSlash = fullPathName.lastIndexOf('/');
if (lastSlash < 0) return null;
String pkg = fullPathName.substring(0, lastSlash + 1);
- return ((SimpleGroovyRootDoc)rootDoc).classNamedExact(pkg + baseName);
+ String candidate = baseName.contains(".") ? baseName.replace('.', '$')
: baseName;
+ return ((SimpleGroovyRootDoc)rootDoc).classNamedExact(pkg + candidate);
Review Comment:
resolveInternalClassDocFromSamePackage now maps dotted names to '$'
(baseName.replace('.', '$')), but nested classes appear to be keyed with '.'
separators in fullPathName elsewhere in this class. This lookup likely won’t
find same-package nested classes unless the same convention is used
consistently.
##########
subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/TagRenderer.java:
##########
@@ -961,20 +967,22 @@ private static String resolveInheritDoc(GroovyMemberDoc
memberDoc,
Set<GroovyMethodDoc> visited,
InheritDocContext
inheritDocContext) {
if (!(memberDoc instanceof GroovyMethodDoc thisMethod)) return null;
- if (classDoc == null) return null;
+ if (classDoc == null) return "";
if (visited == null) visited = new HashSet<>();
if (!visited.add(thisMethod)) return ""; // cycle: suppress literal
{@inheritDoc}
GroovyMethodDoc parent = findInheritedMethod(thisMethod, classDoc, new
HashSet<>());
- if (parent == null) return null;
- if (parent instanceof SimpleGroovyMemberDoc parentMember
- && parentMember.belongsToClass instanceof SimpleGroovyClassDoc
parentClassDoc) {
+ if (parent == null) return "";
+ if (parent instanceof SimpleGroovyMemberDoc parentMember) {
Review Comment:
resolveInheritDoc’s Javadoc says it returns null when no parent method is
found or doc is unavailable, but the implementation now returns "" for several
of those cases (e.g., classDoc==null, parent==null, missing inheritedTag).
Since the caller treats non-null as “resolved”, this changes behavior and makes
the method contract misleading. Either restore null for “unresolved” or update
the contract/caller logic to match the new semantics.
--
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]
