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

4 files changed, 228 insertions, 5 deletions

diff --git a/src/test/java/com/google/devtools/build/docgen/BUILD b/src/test/java/com/google/devtools/build/docgen/BUILD
index 2d3ffdf44f..026e6c9e81 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 @@ filegroup(
     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_test(
 )
 
 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 @@ java_library(
         "//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 @@ java_library(
 )
 
 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 c61090e210..fd67e70b65 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 0000000000..b35302f1ba
--- /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 452b39bfd6..4bca09cae3 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 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
     }
   }
 
+  /** 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 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
                 + "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)