From 3ac772f04c19ed6ede5855d15d8699b634f2284f Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Thu, 10 Dec 2020 12:58:42 -0500 Subject: [PATCH] [java-source-utils] Better support orphaned Javadocs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/xamarin/java.interop/pull/687#issuecomment-727099128 What happens when there's a "regular" Java comment in between a Javadoc comment and a member? /* partial */ class Object { /** Create and return a copy of this object… */ // BEGIN Android-changed: Use native local helper for clone() // … protected Object clone() throws CloneNotSupportedException {…} } What happens is that the Javadoc becomes *orphaned*. Commit 69e1b80a attempted to handle such orphaned Javadocs via heuristic, using the first orphaned Javadoc comment in the parent scope. This didn't work reliably, as the parent scope could contain multiple "*unrelated*" orphaned Javadoc comments: class Outer { /** Orphaned #1 */ // cause orphaning class Inner {} void m() {} } Because containing types are fully processed before contained types, `Outer.m()` would grab the Javadoc for `Outer.Inner` before `Outer.Inner` would have a chance to grab it. Re-work the logic to associate orphaned Javadocs with their members, by requiring that the Javadoc comment begin *before* the member of interest, and *after* any preceding members. This should prevent incorrect correlation of orphaned Javadoc comment blocks. Additionally, update gradle to use javaparser 3.18.0, from 3.16.1: * https://github.com/javaparser/javaparser/compare/javaparser-parent-3.16.1...javaparser-parent-3.18.0 --- tools/java-source-utils/build.gradle | 4 +- .../android/JniPackagesInfoFactory.java | 92 ++++++++++++++----- .../android/JavadocXmlGeneratorTest.java | 4 + .../com/microsoft/android/Outer.java | 2 +- 4 files changed, 74 insertions(+), 28 deletions(-) diff --git a/tools/java-source-utils/build.gradle b/tools/java-source-utils/build.gradle index 516e39b6b..445f43b08 100644 --- a/tools/java-source-utils/build.gradle +++ b/tools/java-source-utils/build.gradle @@ -30,8 +30,8 @@ repositories { dependencies { // This dependency is used by the application. - implementation 'com.github.javaparser:javaparser-core:3.16.1' - implementation 'com.github.javaparser:javaparser-symbol-solver-core:3.16.1' + implementation 'com.github.javaparser:javaparser-core:3.18.0' + implementation 'com.github.javaparser:javaparser-symbol-solver-core:3.18.0' // Use JUnit test framework testImplementation 'junit:junit:4.12' diff --git a/tools/java-source-utils/src/main/java/com/microsoft/android/JniPackagesInfoFactory.java b/tools/java-source-utils/src/main/java/com/microsoft/android/JniPackagesInfoFactory.java index b63c3162b..b91e6eb37 100644 --- a/tools/java-source-utils/src/main/java/com/microsoft/android/JniPackagesInfoFactory.java +++ b/tools/java-source-utils/src/main/java/com/microsoft/android/JniPackagesInfoFactory.java @@ -1,6 +1,7 @@ package com.microsoft.android; import java.io.File; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Optional; @@ -35,8 +36,6 @@ import com.microsoft.android.ast.*; -import javassist.compiler.ast.FieldDecl; - import static com.microsoft.android.util.Parameter.*; public final class JniPackagesInfoFactory { @@ -90,6 +89,8 @@ private void parse(final JniPackagesInfo packages, final CompilationUnit unit) t : ""; final JniPackageInfo packageInfo = packages.getPackage(packageName); + fixJavadocComments(unit, unit.getTypes()); + for (final TypeDeclaration type : unit.getTypes()) { if (JavaSourceUtilsOptions.verboseOutput && type.getFullyQualifiedName().isPresent()) { System.out.println("Processing: " + type.getFullyQualifiedName().get()); @@ -171,6 +172,7 @@ static ClassOrInterfaceType getTypeParameterBound(TypeParameter typeParameter) { } private final void parseType(final JniPackageInfo packageInfo, final JniTypeInfo typeInfo, TypeDeclaration typeDecl) { + fixJavadocComments(typeDecl, getUndocumentedBodyMembers(typeDecl.getMembers())); for (final BodyDeclaration body : typeDecl.getMembers()) { if (body.isAnnotationDeclaration()) { final AnnotationDeclaration annoDecl = body.asAnnotationDeclaration(); @@ -215,6 +217,68 @@ private final void parseType(final JniPackageInfo packageInfo, final JniTypeInfo } } + private final void fixJavadocComments(final Node decl, final Iterable> bodyMembers) { + final List> members = getUndocumentedBodyMembers(bodyMembers); + final List orphanedComments = getOrphanComments(decl); + + if (members.size() == 0) + return; + + final BodyDeclaration firstMember = members.get(0); + JavadocComment comment = orphanedComments.stream() + .filter(c -> c.getBegin().get().isBefore(firstMember.getBegin().get())) + .reduce((a, b) -> b) + .orElse(null); + if (comment != null) { + ((NodeWithJavadoc) firstMember).setJavadocComment(comment); + } + + for (int i = 1; i < members.size(); ++i) { + BodyDeclaration prevMember = members.get(i-1); + BodyDeclaration member = members.get(i); + + Optional commentOpt = orphanedComments.stream() + .filter(c -> c.getBegin().get().isAfter(prevMember.getEnd().get()) && + c.getEnd().get().isBefore(member.getBegin().get())) + .findFirst(); + if (!commentOpt.isPresent()) + continue; + ((NodeWithJavadoc)member).setJavadocComment(commentOpt.get()); + } + } + + private final List> getUndocumentedBodyMembers(Iterable> bodyMembers) { + final List> members = new ArrayList> (); + for (BodyDeclaration member : bodyMembers) { + if (!(member instanceof NodeWithJavadoc)) { + continue; + } + final NodeWithJavadoc memberJavadoc = (NodeWithJavadoc) member; + if (memberJavadoc.getJavadocComment().isPresent()) + continue; + final Optional memberBeginOpt = member.getBegin(); + if (!memberBeginOpt.isPresent()) + continue; + members.add(member); + } + members.sort((a, b) -> a.getBegin().get().compareTo(b.getBegin().get())); + return members; + } + + private final List getOrphanComments(Node decl) { + final List orphanedComments = new ArrayList(decl.getOrphanComments().size()); + for (Comment c : decl.getOrphanComments()) { + if (!c.isJavadocComment()) + continue; + final Optional commentBeginOpt = c.getBegin(); + if (!commentBeginOpt.isPresent()) + continue; + orphanedComments.add(c.asJavadocComment()); + } + orphanedComments.sort((a, b) -> a.getBegin().get().compareTo(b.getBegin().get())); + return orphanedComments; + } + private final void parseAnnotationMemberDecl(final JniTypeInfo typeInfo, final AnnotationMemberDeclaration memberDecl) { final JniMethodInfo methodInfo = new JniMethodInfo(typeInfo, memberDecl.getNameAsString()); typeInfo.add(methodInfo); @@ -266,30 +330,8 @@ private static final void fillJavadoc(final HasJavadocComment member, NodeWithJa JavadocComment javadoc = null; if (nodeWithJavadoc.getJavadocComment().isPresent()) { javadoc = nodeWithJavadoc.getJavadocComment().get(); - } else { - Node node = (Node) nodeWithJavadoc; - if (!node.getParentNode().isPresent()) - return; - - /* - * Sometimes `JavaParser` won't associate a Javadoc comment block with - * the AST node we expect it to. In such circumstances the Javadoc - * comment will become an "orphan" comment, unassociated with anything. - * - * If `nodeWithJavadoc` has no Javadoc comment, use the *first* - * orphan Javadoc comment in the *parent* scope, then *remove* that - * comment so that it doesn't "stick around" for the next member we - * attempt to grab Javadoc comments for. - */ - Node parent = node.getParentNode().get(); - for (Comment c : parent.getOrphanComments()) { - if (c.isJavadocComment()) { - javadoc = c.asJavadocComment(); - c.remove(); - break; - } - } } + if (javadoc != null) { member.setJavadocComment(javadoc.parse().toText()); } diff --git a/tools/java-source-utils/src/test/java/com/microsoft/android/JavadocXmlGeneratorTest.java b/tools/java-source-utils/src/test/java/com/microsoft/android/JavadocXmlGeneratorTest.java index e16c5d26c..e46d8738c 100644 --- a/tools/java-source-utils/src/test/java/com/microsoft/android/JavadocXmlGeneratorTest.java +++ b/tools/java-source-utils/src/test/java/com/microsoft/android/JavadocXmlGeneratorTest.java @@ -4,6 +4,7 @@ import java.io.File; import java.io.FileNotFoundException; import java.io.PrintStream; +import java.io.FileOutputStream; import java.io.UnsupportedEncodingException; import java.util.Arrays; import javax.xml.parsers.ParserConfigurationException; @@ -120,6 +121,9 @@ private static void testWritePackages(final String resourceJava, final String re final String expected = JniPackagesInfoTest.getResourceContents(resourceXml); generator.writePackages(packagesInfo); + // try (FileOutputStream o = new FileOutputStream(resourceXml + "-jonp.xml")) { + // bytes.writeTo(o); + // } assertEquals(resourceJava + " Javadoc XML", expected, bytes.toString()); } } diff --git a/tools/java-source-utils/src/test/resources/com/microsoft/android/Outer.java b/tools/java-source-utils/src/test/resources/com/microsoft/android/Outer.java index 3087e5d28..c2f9ad9e7 100644 --- a/tools/java-source-utils/src/test/resources/com/microsoft/android/Outer.java +++ b/tools/java-source-utils/src/test/resources/com/microsoft/android/Outer.java @@ -9,7 +9,7 @@ * * JNI sig: Lexample/Outer; */ - +// Cause the above Javadoc block to be orphaned from the below class declaration @Outer.MyAnnotation(keys={"a", "b", "c"}) public class Outer {