Fix docgen handling of multiple modules with the same name If there are multiple modules with the same name: - If only one is marked 'documented', that one takes precedence. - If one is a subclass of the others, the subclass takes precedence. - Otherwise, an exception is thrown. RELNOTES: None. PiperOrigin-RevId: 202359443
diff --git a/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationCollector.java b/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationCollector.java index 884fe29..0704c38 100644 --- a/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationCollector.java +++ b/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationCollector.java
@@ -89,7 +89,7 @@ private static SkylarkModuleDoc getTopLevelModuleDoc(Map<String, SkylarkModuleDoc> modules) { SkylarkModule annotation = getTopLevelModule(); modules.computeIfAbsent( - annotation.name(), (String k) -> new SkylarkModuleDoc(annotation, Object.class)); + annotation.name(), (String k) -> new SkylarkModuleDoc(annotation, TopLevelModule.class)); return modules.get(annotation.name()); } @@ -101,8 +101,29 @@ SkylarkModule annotation = Preconditions.checkNotNull( Runtime.getSkylarkNamespace(moduleClass).getAnnotation(SkylarkModule.class)); - modules.computeIfAbsent( - annotation.name(), (String k) -> new SkylarkModuleDoc(annotation, moduleClass)); + SkylarkModuleDoc previousModuleDoc = modules.get(annotation.name()); + if (previousModuleDoc == null || !previousModuleDoc.getAnnotation().documented()) { + // There is no registered module doc by that name, or the current candidate is "undocumented". + modules.put(annotation.name(), new SkylarkModuleDoc(annotation, moduleClass)); + } else if (previousModuleDoc.getClassObject() != moduleClass && annotation.documented()) { + // Both modules generate documentation for the same name. This is fine if one is a + // subclass of the other, in which case the subclass documentation takes precedence. + // (This is useful if one module is a "common" stable module, and its subclass is + // an experimental module that also supports all stable methods.) + if (previousModuleDoc.getClassObject().isAssignableFrom(moduleClass)) { + modules.put(annotation.name(), new SkylarkModuleDoc(annotation, moduleClass)); + } else if (moduleClass.isAssignableFrom(previousModuleDoc.getClassObject())) { + // This case means the subclass was processed first, so discard the superclass. + } else { + throw new IllegalStateException( + String.format( + "%s and %s are both modules with the same documentation for '%s'", + moduleClass, + previousModuleDoc.getClassObject(), + previousModuleDoc.getAnnotation().name())); + } + } + return modules.get(annotation.name()); } @@ -225,14 +246,14 @@ if (!constructorAnnotation.receiverNameForDoc().isEmpty()) { fullyQualifiedName = constructorAnnotation.receiverNameForDoc(); } else { - fullyQualifiedName = getFullyQualifiedName(originatingModuleName, method, callable); + fullyQualifiedName = getFullyQualifiedName(originatingModuleName, callable); } module.setConstructor(new SkylarkConstructorMethodDoc(fullyQualifiedName, method, callable)); } private static String getFullyQualifiedName( - String objectName, Method method, SkylarkCallable callable) { + String objectName, SkylarkCallable callable) { String objectDotExpressionPrefix = objectName.isEmpty() ? "" : objectName + "."; String methodName = callable.name(); return objectDotExpressionPrefix + methodName;
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidNativeLibsInfoApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidNativeLibsInfoApi.java index cf69fee..0dbc673 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidNativeLibsInfoApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidNativeLibsInfoApi.java
@@ -19,6 +19,7 @@ import com.google.devtools.build.lib.skylarkbuildapi.StructApi; import com.google.devtools.build.lib.skylarkinterface.Param; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; +import com.google.devtools.build.lib.skylarkinterface.SkylarkConstructor; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.SkylarkNestedSet; @@ -58,6 +59,7 @@ ), }, selfCall = true) + @SkylarkConstructor(objectType = AndroidNativeLibsInfoApi.class) public AndroidNativeLibsInfoApi<?> createInfo(SkylarkNestedSet nativeLibs) throws EvalException; } }
diff --git a/src/test/java/com/google/devtools/build/docgen/BUILD b/src/test/java/com/google/devtools/build/docgen/BUILD index 2d3ffdf..026e6c9 100644 --- a/src/test/java/com/google/devtools/build/docgen/BUILD +++ b/src/test/java/com/google/devtools/build/docgen/BUILD
@@ -10,9 +10,19 @@ visibility = ["//src:__pkg__"], ) -test_suite( - name = "all_tests", +java_test( + name = "DocumentationFailuresTests", + size = "medium", + shard_count = 1, tags = ["docgen"], + test_class = "com.google.devtools.build.lib.AllTests", + visibility = ["//devtools/blaze/main:__pkg__"], + runtime_deps = [ + ":documentation-failures-tests", + "//src/test/java/com/google/devtools/build/lib:test_runner", + "//src/test/java/com/google/devtools/build/lib:testutil", + "//third_party:junit4", + ], ) java_test( @@ -29,6 +39,26 @@ ) java_library( + name = "documentation-failures-tests", + testonly = 1, + srcs = [ + "SkylarkDocumentationFailuresTest.java", + ], + deps = [ + "//src/main/java/com/google/devtools/build/docgen:docgen_javalib", + "//src/main/java/com/google/devtools/build/lib:build-base", + "//src/main/java/com/google/devtools/build/lib:skylarkinterface", + "//src/main/java/com/google/devtools/build/lib:syntax", + "//src/test/java/com/google/devtools/build/lib:syntax_testutil", + "//src/test/java/com/google/devtools/build/lib:testutil", + "//src/test/java/com/google/devtools/build/lib/skylark:testutil", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) + +java_library( name = "documentation-tests", testonly = 1, srcs = [ @@ -45,7 +75,6 @@ "//src/main/java/com/google/devtools/build/lib:packages-internal", "//src/main/java/com/google/devtools/build/lib:skylarkinterface", "//src/main/java/com/google/devtools/build/lib:syntax", - "//src/main/java/com/google/devtools/build/lib/collect", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/test/java/com/google/devtools/build/lib:syntax_testutil", @@ -69,6 +98,11 @@ ) test_suite( + name = "all_tests", + tags = ["docgen"], +) + +test_suite( name = "windows_tests", tags = [ "-no_windows",
diff --git a/src/test/java/com/google/devtools/build/docgen/DocumentationTests.java b/src/test/java/com/google/devtools/build/docgen/DocumentationTests.java index c61090e..fd67e70 100644 --- a/src/test/java/com/google/devtools/build/docgen/DocumentationTests.java +++ b/src/test/java/com/google/devtools/build/docgen/DocumentationTests.java
@@ -14,11 +14,10 @@ package com.google.devtools.build.docgen; import com.google.devtools.build.lib.testutil.ClasspathSuite; - import org.junit.runner.RunWith; /** - * Test suite for options parsing framework. + * Test suite for skylark documentation generation. */ @RunWith(ClasspathSuite.class) public class DocumentationTests {
diff --git a/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationFailuresTest.java b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationFailuresTest.java new file mode 100644 index 0000000..b35302f --- /dev/null +++ b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationFailuresTest.java
@@ -0,0 +1,105 @@ +// Copyright 2015 The Bazel Authors. All rights reserved. +// +// Licensed 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 com.google.devtools.build.docgen; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; + +import com.google.devtools.build.docgen.skylark.SkylarkModuleDoc; +import com.google.devtools.build.lib.skylark.util.SkylarkTestCase; +import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; +import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; +import com.google.devtools.build.lib.syntax.SkylarkSemantics; +import com.google.devtools.build.lib.syntax.util.EvaluationTestCase; +import java.util.Map; +import java.util.TreeMap; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Tests for various failure modes of Skylark documentation generation. These are separate from + * other documentation generation tests because the generator does not tolerate broken modules + * anywhere in the classpath. + */ +@RunWith(JUnit4.class) +public class SkylarkDocumentationFailuresTest extends SkylarkTestCase { + + @Before + public final void createBuildFile() throws Exception { + scratch.file("foo/BUILD", + "genrule(name = 'foo',", + " cmd = 'dummy_cmd',", + " srcs = ['a.txt', 'b.img'],", + " tools = ['t.exe'],", + " outs = ['c.txt'])"); + } + + @Override + protected EvaluationTestCase createEvaluationTestCase(SkylarkSemantics semantics) { + return new EvaluationTestCase(); + } + + /** MockClassCommonNameOne */ + @SkylarkModule(name = "MockClassCommonName", + doc = "MockClassCommonName") + private static class MockClassCommonNameOne { + + @SkylarkCallable(name = "one", doc = "one") + public Integer one() { + return 1; + } + } + + /** MockClassCommonNameTwo */ + @SkylarkModule(name = "MockClassCommonName", + doc = "MockClassCommonName") + private static class MockClassCommonNameTwo { + + @SkylarkCallable(name = "two", doc = "two") + public Integer two() { + return 1; + } + } + + /** PointsToCommonName */ + @SkylarkModule(name = "PointsToCommonName", + doc = "PointsToCommonName") + private static class PointsToCommonName { + @SkylarkCallable(name = "one", doc = "one") + public MockClassCommonNameOne getOne() { + return null; + } + + @SkylarkCallable(name = "two", doc = "two") + public MockClassCommonNameTwo getTwo() { + return null; + } + } + + @Test + public void testModuleNameConflict() { + IllegalStateException expected = + assertThrows(IllegalStateException.class, () -> collect(PointsToCommonName.class)); + assertThat(expected.getMessage()).contains("are both modules with the same documentation"); + } + + private Map<String, SkylarkModuleDoc> collect(Class<?> classObject) { + Map<String, SkylarkModuleDoc> modules = new TreeMap<>(); + SkylarkDocumentationCollector.collectJavaObjects( + classObject.getAnnotation(SkylarkModule.class), classObject, modules); + return modules; + } +}
diff --git a/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java index 452b39b..4bca09c 100644 --- a/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java +++ b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java
@@ -279,6 +279,71 @@ } } + /** MockClassCommonNameOne */ + @SkylarkModule(name = "MockClassCommonName", + doc = "MockClassCommonName") + private static class MockClassCommonNameOne { + + @SkylarkCallable(name = "one", doc = "one") + public Integer one() { + return 1; + } + } + + /** SubclassOfMockClassCommonNameOne */ + @SkylarkModule(name = "MockClassCommonName", + doc = "MockClassCommonName") + private static class SubclassOfMockClassCommonNameOne extends MockClassCommonNameOne { + + @SkylarkCallable(name = "two", doc = "two") + public Integer two() { + return 1; + } + } + + /** PointsToCommonNameOneWithSubclass */ + @SkylarkModule(name = "PointsToCommonNameOneWithSubclass", + doc = "PointsToCommonNameOneWithSubclass") + private static class PointsToCommonNameOneWithSubclass { + @SkylarkCallable(name = "one", doc = "one") + public MockClassCommonNameOne getOne() { + return null; + } + + @SkylarkCallable(name = "one_subclass", doc = "one_subclass") + public SubclassOfMockClassCommonNameOne getOneSubclass() { + return null; + } + } + + /** MockClassCommonNameOneUndocumented */ + @SkylarkModule(name = "MockClassCommonName", + documented = false, + doc = "") + private static class MockClassCommonNameUndocumented { + + @SkylarkCallable(name = "two", doc = "two") + public Integer two() { + return 1; + } + } + + /** PointsToCommonNameAndUndocumentedModule */ + @SkylarkModule(name = "PointsToCommonNameAndUndocumentedModule", + doc = "PointsToCommonNameAndUndocumentedModule") + private static class PointsToCommonNameAndUndocumentedModule { + @SkylarkCallable(name = "one", doc = "one") + public MockClassCommonNameOne getOne() { + return null; + } + + @SkylarkCallable(name = "undocumented_module", doc = "undocumented_module") + public MockClassCommonNameUndocumented getUndocumented() { + return null; + } + } + + @Test public void testSkylarkJavaInterfaceExplorerOnSimpleClass() throws Exception { Map<String, SkylarkModuleDoc> objects = collect(MockClassA.class); @@ -439,6 +504,26 @@ + "MockClassWithContainerReturnValues.skylark()"); } + @Test + public void testDocumentedModuleTakesPrecedence() throws Exception { + Map<String, SkylarkModuleDoc> objects = collect(PointsToCommonNameAndUndocumentedModule.class); + Collection<SkylarkMethodDoc> methods = + objects.get("MockClassCommonName").getMethods(); + List<String> methodNames = + methods.stream().map(m -> m.getName()).collect(Collectors.toList()); + assertThat(methodNames).containsExactly("one"); + } + + @Test + public void testDocumentModuleSubclass() { + Map<String, SkylarkModuleDoc> objects = collect(PointsToCommonNameOneWithSubclass.class); + Collection<SkylarkMethodDoc> methods = + objects.get("MockClassCommonName").getMethods(); + List<String> methodNames = + methods.stream().map(m -> m.getName()).collect(Collectors.toList()); + assertThat(methodNames).containsExactly("one", "two"); + } + private Iterable<Method> extractMethods(Collection<SkylarkMethodDoc> methods) { return methods.stream() .filter(methodDoc -> methodDoc instanceof SkylarkJavaMethodDoc)
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index 33748a6..683170e 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -153,6 +153,7 @@ name = "test_runner", testonly = 1, srcs = ["AllTests.java"], + visibility = ["//src/test/java/com/google/devtools/build:__subpackages__"], deps = [ ":testutil", "//third_party:junit4",