GROOVY-11988: Add support for {@inheritdoc} in external JDK classes (address AI review comments)

Author: paulk-asert

subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/ExternalJavadocSupport.java

@@ -45,6 +45,7 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
@@ -55,6 +56,7 @@
  * source method overrides a method declared outside the documented source set.
  */
 final class ExternalJavadocSupport {
+    private static final System.Logger LOGGER = System.getLogger(ExternalJavadocSupport.class.getName());
     private static final JavaParser JAVA_PARSER = new JavaParser(
             new ParserConfiguration().setLanguageLevel(ParserConfiguration.LanguageLevel.BLEEDING_EDGE)
     );
@@ -63,7 +65,10 @@ final class ExternalJavadocSupport {
     private static final Map<Class<?>, List<ExternalMethodData>> METHOD_CACHE = new ConcurrentHashMap<>();
     private static final Map<Class<?>, GroovyMethodDoc[]> METHOD_DOC_CACHE = new ConcurrentHashMap<>();
     private static final AtomicInteger ACTIVE_CACHE_SESSIONS = new AtomicInteger();
+    private static final AtomicBoolean SRC_ZIP_WARNING_LOGGED = new AtomicBoolean();
+    private static final Object ZIP_LOCK = new Object();
     private static final GroovyMethodDoc[] EMPTY_GROOVYMETHODDOC_ARRAY = new GroovyMethodDoc[0];
+    private static ZipFile sharedZipFile;
 
     private ExternalJavadocSupport() {
     }
@@ -115,14 +120,16 @@ static CacheStats cacheStats() {
     }
 
     /**
-     * Clears all external Javadoc caches. This method is automatically called when
-     * the last active {@link CacheSession} is closed. It can also be called manually
-     * to force a reset of cached data.
+     * Clears all external Javadoc caches and releases the shared JDK source archive
+     * handle. This method is automatically called when the last active
+     * {@link CacheSession} is closed. It can also be called manually to force a reset
+     * of cached data.
      */
     static void clearCaches() {
         RAW_COMMENT_CACHE.clear();
         METHOD_CACHE.clear();
         METHOD_DOC_CACHE.clear();
+        closeSharedZipFile();
     }
 
     private static GroovyMethodDoc[] cachedMethodDocsFor(Class<?> externalClass) {
@@ -277,11 +284,26 @@ private static String resolveEffectiveComment(Class<?> ownerClass, Method method
     }
 
     private static Optional<CompilationUnit> loadCompilationUnit(Class<?> externalClass) {
-        if (JDK_SRC_ZIP == null) return Optional.empty();
+        if (JDK_SRC_ZIP == null) {
+            warnMissingSrcZipOnce();
+            return Optional.empty();
+        }
         String entryName = sourceEntryName(externalClass);
         if (entryName == null) return Optional.empty();
 
+        ZipFile shared = sharedZipFileForActiveSession();
+        if (shared != null) {
+            return parseCompilationUnit(shared, entryName);
+        }
         try (ZipFile zip = new ZipFile(JDK_SRC_ZIP.toFile())) {
+            return parseCompilationUnit(zip, entryName);
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+    }
+
+    private static Optional<CompilationUnit> parseCompilationUnit(ZipFile zip, String entryName) {
+        try {
             ZipEntry entry = zip.getEntry(entryName);
             if (entry == null) {
                 entry = findFallbackEntry(zip, entryName);
@@ -297,7 +319,40 @@ private static Optional<CompilationUnit> loadCompilationUnit(Class<?> externalCl
         }
     }
 
-    private static ZipEntry findFallbackEntry(ZipFile zip, String entryName) {
+    private static ZipFile sharedZipFileForActiveSession() {
+        if (JDK_SRC_ZIP == null || ACTIVE_CACHE_SESSIONS.get() == 0) return null;
+        synchronized (ZIP_LOCK) {
+            if (sharedZipFile != null) return sharedZipFile;
+            if (ACTIVE_CACHE_SESSIONS.get() == 0) return null;
+            try {
+                sharedZipFile = new ZipFile(JDK_SRC_ZIP.toFile());
+            } catch (IOException e) {
+                throw new UncheckedIOException(e);
+            }
+            return sharedZipFile;
+        }
+    }
+
+    private static void closeSharedZipFile() {
+        synchronized (ZIP_LOCK) {
+            if (sharedZipFile == null) return;
+            try {
+                sharedZipFile.close();
+            } catch (IOException ignored) {
+            }
+            sharedZipFile = null;
+        }
+    }
+
+    private static void warnMissingSrcZipOnce() {
+        if (SRC_ZIP_WARNING_LOGGED.compareAndSet(false, true)) {
+            LOGGER.log(System.Logger.Level.WARNING,
+                    "JDK src.zip not found under java.home=" + System.getProperty("java.home")
+                            + "; external {@inheritDoc} resolution from JDK classes is unavailable.");
+        }
+    }
+
+    static ZipEntry findFallbackEntry(ZipFile zip, String entryName) {
         int slash = entryName.indexOf('/');
         String suffix = slash >= 0 ? "/" + entryName.substring(slash + 1) : "/" + entryName;
         return zip.stream()
@@ -395,7 +450,7 @@ private static boolean matchesTypeName(String declaredType, Class<?> reflectedTy
         return false;
     }
 
-    private static String eraseTypeName(String declaredType) {
+    static String eraseTypeName(String declaredType) {
         if (declaredType == null || declaredType.isEmpty()) return "";
         StringBuilder erased = new StringBuilder(declaredType.length());
         int genericDepth = 0;

subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/TagRenderer.java

@@ -1156,6 +1156,12 @@ private static boolean isReferenceType(String typeName) {
     }
 
     private static boolean isTypeVariableName(String typeName) {
+        // Heuristic for recognizing a Java type-variable name when matching an override
+        // against its parent declaration during {@inheritDoc} resolution. Matches the
+        // common single-letter (optionally numbered) convention (T, E, R, K, V, T1, ...).
+        // KEY and VALUE are accepted because Groovy's own org.apache.groovy.json.internal.Cache
+        // declares its type parameters as <KEY, VALUE>, which would otherwise look like
+        // real class names to this matcher.
         return typeName.matches("[A-Z][0-9]?") || "KEY".equals(typeName) || "VALUE".equals(typeName);
     }
 

subprojects/groovy-groovydoc/src/test/groovy/org/codehaus/groovy/tools/groovydoc/GroovyDocToolTest.java

@@ -47,6 +47,9 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+import java.util.zip.ZipOutputStream;
 
 public class GroovyDocToolTest extends GroovyTestCase {
     private static final String MOCK_DIR = "mock/doc";
@@ -824,6 +827,7 @@ public void testInheritDocResolvesForInterfaceMethodsInheritedThroughSuperclasse
     }
 
     public void testInheritDocResolvesFromExternalJdkAbstractClassInHtml() throws Exception {
+        if (skipIfNoJdkSrcZip()) return;
         String base = "org/codehaus/groovy/tools/groovydoc/testfiles";
         htmlTool.add(List.of(base + "/JavaExtendsWriterInheritDoc.java"));
 
@@ -836,15 +840,16 @@ public void testInheritDocResolvesFromExternalJdkAbstractClassInHtml() throws Ex
         assertNotNull("Expected JavaExtendsWriterInheritDoc.html in output", doc);
         assertNotNull("Expected close() section in:\n" + doc, closeSection);
         assertNotNull("Expected flush() section in:\n" + doc, flushSection);
-        assertTrue("Expected inherited close() text from java.io.Writer in:\n" + doc,
-                normalizeWhitespace(closeSection).contains("Closes the stream"));
-        assertTrue("Expected inherited flush() text from java.io.Writer in:\n" + doc,
-                normalizeWhitespace(flushSection).contains("Flushes the stream"));
+        assertMatches("Expected inherited close() text from java.io.Writer in:\n" + doc,
+                "(?i)close[sd]?\\b[^\\n]{0,40}\\bstream", normalizeWhitespace(closeSection));
+        assertMatches("Expected inherited flush() text from java.io.Writer in:\n" + doc,
+                "(?i)flush(es|ed|ing)?\\b[^\\n]{0,40}\\bstream", normalizeWhitespace(flushSection));
         assertFalse("External JDK inheritDoc should not remain literal in:\n" + doc,
                 doc.contains("{@inheritDoc}"));
     }
 
     public void testInheritDocResolvesFromExternalObjectMethodInHtml() throws Exception {
+        if (skipIfNoJdkSrcZip()) return;
         String base = "org/codehaus/groovy/tools/groovydoc/testfiles";
         htmlTool.add(List.of(base + "/JavaObjectCloneInheritDocChild.java"));
 
@@ -855,8 +860,8 @@ public void testInheritDocResolvesFromExternalObjectMethodInHtml() throws Except
         String cloneSection = findMethodSection(doc, "clone", "");
         assertNotNull("Expected JavaObjectCloneInheritDocChild.html in output", doc);
         assertNotNull("Expected clone() section in:\n" + doc, cloneSection);
-        assertTrue("Expected inherited clone() text from java.lang.Object in:\n" + doc,
-                normalizeWhitespace(cloneSection).contains("Creates and returns a copy of this object"));
+        assertMatches("Expected inherited clone() text from java.lang.Object in:\n" + doc,
+                "(?i)copy\\s+of\\s+this\\s+object", normalizeWhitespace(cloneSection));
         assertFalse("External Object inheritDoc should not remain literal in:\n" + doc,
                 doc.contains("{@inheritDoc}"));
     }
@@ -903,6 +908,7 @@ public void testExternalJavadocSupportClearsCachesWhenSessionCloses() throws Exc
     }
 
     public void testInheritDocResolvesFromExternalMapAndObjectMethodsInHtml() throws Exception {
+        if (skipIfNoJdkSrcZip()) return;
         String base = "org/codehaus/groovy/tools/groovydoc/testfiles";
         htmlTool.add(List.of(base + "/JavaImplementsMapInheritDoc.java"));
 
@@ -919,16 +925,16 @@ public void testInheritDocResolvesFromExternalMapAndObjectMethodsInHtml() throws
         assertNotNull("Expected containsValue(Object) section in:\n" + doc, containsValueSection);
         assertNotNull("Expected equals(Object) section in:\n" + doc, equalsSection);
         assertNotNull("Expected hashCode() section in:\n" + doc, hashCodeSection);
-        assertTrue("Expected inherited clear() text from java.util.Map in:\n" + doc,
-                normalizeWhitespace(clearSection).contains("Removes all of the mappings from this map"));
-        assertTrue("Expected inherited containsValue(Object) text from java.util.Map in:\n" + doc,
-                containsValueSection.contains("Returns <CODE>true</CODE> if this map maps one or more keys to the"));
-        assertTrue("Expected inherited equals(Object) text from java.lang.Object in:\n" + doc,
-                equalsSection.contains("Indicates whether some other object is \"equal to\" this one"));
-        assertTrue("Expected normalized inherited hashCode() text from java.lang.Object in:\n" + doc,
-                normalizeWhitespace(hashCodeSection).contains("a hash code value for this object"));
-        assertFalse("Inherited external docs should not retain raw Javadoc comment markers in:\n" + doc,
-                normalizeWhitespace(doc).contains("* Removes all of the mappings"));
+        assertMatches("Expected inherited clear() text from java.util.Map in:\n" + doc,
+                "(?i)remov(es|ing)?\\b[^\\n]{0,30}\\bmapping", normalizeWhitespace(clearSection));
+        assertMatches("Expected inherited containsValue(Object) text from java.util.Map in:\n" + doc,
+                "(?i)map\\s+maps?\\s+one\\s+or\\s+more\\s+key", normalizeWhitespace(containsValueSection));
+        assertMatches("Expected inherited equals(Object) text from java.lang.Object in:\n" + doc,
+                "(?i)\\bother\\s+object\\b[^\\n]{0,40}\\bequal", normalizeWhitespace(equalsSection));
+        assertMatches("Expected normalized inherited hashCode() text from java.lang.Object in:\n" + doc,
+                "(?i)hash\\s*code\\b[^\\n]{0,30}\\bobject", normalizeWhitespace(hashCodeSection));
+        assertFalse("Inherited external docs should not retain raw Javadoc comment markers (a leading '* ') in:\n" + doc,
+                Pattern.compile("\\*\\s+(?i:Removes|Returns|Indicates|Creates|Closes|Flushes)\\b").matcher(doc).find());
         assertFalse("Inherited external docs should not leave raw link/index inline tags in:\n" + doc,
                 doc.contains("{@linkplain") || doc.contains("{@index"));
         assertFalse("External Map/Object inheritDoc should not remain literal in:\n" + doc,
@@ -1051,6 +1057,31 @@ private static String normalizeWhitespace(String text) {
         return text == null ? null : text.replaceAll("\\s+", " ").trim();
     }
 
+    private static void assertMatches(String message, String regex, String content) {
+        if (content == null || !Pattern.compile(regex).matcher(content).find()) {
+            fail(message + "\n  pattern: " + regex + "\n  content: " + content);
+        }
+    }
+
+    private boolean skipIfNoJdkSrcZip() {
+        if (jdkSrcZipPath() != null) return false;
+        System.out.println("[skip] " + getName() + ": JDK src.zip not found under java.home="
+                + System.getProperty("java.home"));
+        return true;
+    }
+
+    private static Path jdkSrcZipPath() {
+        String javaHome = System.getProperty("java.home");
+        if (javaHome == null || javaHome.isEmpty()) return null;
+        Path home = Paths.get(javaHome);
+        Path direct = home.resolve("lib/src.zip");
+        if (Files.isRegularFile(direct)) return direct;
+        Path parent = home.getParent();
+        if (parent == null) return null;
+        Path sibling = parent.resolve("lib/src.zip");
+        return Files.isRegularFile(sibling) ? sibling : null;
+    }
+
     private static GroovyMethodDoc findMethod(GroovyClassDoc classDoc, String name, int parameterCount) {
         for (GroovyMethodDoc method : classDoc.methods()) {
             if (name.equals(method.name()) && method.parameters().length == parameterCount) {
@@ -3448,4 +3479,88 @@ private GroovyClassDoc getGroovyClassDocByName(GroovyRootDoc root, String name)
 
         return null;
     }
+
+    public void testEraseTypeNameStripsGenericsAndWildcards() {
+        assertEquals("List", ExternalJavadocSupport.eraseTypeName("List<String>"));
+        assertEquals("Map", ExternalJavadocSupport.eraseTypeName("Map<String, Integer>"));
+        assertEquals("Map", ExternalJavadocSupport.eraseTypeName("Map<String, Map<String, Integer>>"));
+        assertEquals("Number", ExternalJavadocSupport.eraseTypeName("? extends Number"));
+        assertEquals("Integer", ExternalJavadocSupport.eraseTypeName("? super Integer"));
+        assertEquals("Object", ExternalJavadocSupport.eraseTypeName("?"));
+        assertEquals("List[]", ExternalJavadocSupport.eraseTypeName("List<? extends T>[]"));
+        assertEquals("", ExternalJavadocSupport.eraseTypeName(""));
+        assertEquals("", ExternalJavadocSupport.eraseTypeName(null));
+    }
+
+    public void testFindFallbackEntryFindsByTrailingPathSuffix() throws Exception {
+        Path zipPath = Files.createTempFile("ext-javadoc-fallback", ".zip");
+        try {
+            try (ZipOutputStream out = new ZipOutputStream(Files.newOutputStream(zipPath))) {
+                out.putNextEntry(new ZipEntry("noise/elsewhere/Other.java"));
+                out.write("// other".getBytes());
+                out.closeEntry();
+                out.putNextEntry(new ZipEntry("some.module/com/example/Foo.java"));
+                out.write("// foo".getBytes());
+                out.closeEntry();
+            }
+            try (ZipFile zip = new ZipFile(zipPath.toFile())) {
+                ZipEntry hit = ExternalJavadocSupport.findFallbackEntry(zip, "java.base/com/example/Foo.java");
+                assertNotNull("Expected fallback to find Foo.java by trailing path", hit);
+                assertEquals("some.module/com/example/Foo.java", hit.getName());
+
+                ZipEntry miss = ExternalJavadocSupport.findFallbackEntry(zip, "java.base/com/example/Missing.java");
+                assertNull("No entry should match Missing.java", miss);
+            }
+        } finally {
+            Files.deleteIfExists(zipPath);
+        }
+    }
+
+    public void testFindFallbackEntryHandlesEntryNameWithoutSlash() throws Exception {
+        Path zipPath = Files.createTempFile("ext-javadoc-fallback-noslash", ".zip");
+        try {
+            try (ZipOutputStream out = new ZipOutputStream(Files.newOutputStream(zipPath))) {
+                out.putNextEntry(new ZipEntry("any/where/Bare.java"));
+                out.write("// bare".getBytes());
+                out.closeEntry();
+            }
+            try (ZipFile zip = new ZipFile(zipPath.toFile())) {
+                ZipEntry hit = ExternalJavadocSupport.findFallbackEntry(zip, "Bare.java");
+                assertNotNull("Expected fallback to find entry by '/Bare.java' suffix", hit);
+                assertEquals("any/where/Bare.java", hit.getName());
+            }
+        } finally {
+            Files.deleteIfExists(zipPath);
+        }
+    }
+
+    // Exercises the matchesTypeName same-package / java.lang / array-aware branches: the JDK
+    // source for java.lang.Throwable declares getMessage() returning String, setStackTrace
+    // taking StackTraceElement[] (bare same-package types). Override-matching has to fall
+    // through past canonical/typeName checks before resolving these to the reflected
+    // java.lang.* qualified types and producing a non-empty raw comment.
+    public void testMatchesTypeNameResolvesUnqualifiedJavaLangTypes() {
+        if (skipIfNoJdkSrcZip()) return;
+        try (AutoCloseable ignored = ExternalJavadocSupport.openCacheSession()) {
+            GroovyMethodDoc[] docs = ExternalJavadocSupport.methodsFor(new ExternalGroovyClassDoc(Throwable.class));
+            assertTrue("Expected reflective methods for java.lang.Throwable", docs.length > 0);
+
+            boolean foundGetMessage = false;
+            boolean foundSetStackTrace = false;
+            for (GroovyMethodDoc doc : docs) {
+                String raw = ((SimpleGroovyMethodDoc) doc).getRawCommentText();
+                if (raw == null || raw.isEmpty()) continue;
+                if ("getMessage".equals(doc.name()) && doc.parameters().length == 0) {
+                    foundGetMessage = true;
+                } else if ("setStackTrace".equals(doc.name()) && doc.parameters().length == 1) {
+                    foundSetStackTrace = true;
+                }
+            }
+            assertTrue("Expected resolved Javadoc for Throwable#getMessage()", foundGetMessage);
+            assertTrue("Expected resolved Javadoc for Throwable#setStackTrace(StackTraceElement[])"
+                    + " (bare same-package array parameter)", foundSetStackTrace);
+        } catch (Exception e) {
+            fail("session close threw: " + e);
+        }
+    }
 }