From 5ede15c6e1ed9480f578b8e541d92a3ce95bc53e Mon Sep 17 00:00:00 2001
From: Googler <arostovtsev@google.com>
Date: Mon, 26 Jun 2023 09:19:45 -0700
Subject: [PATCH] starlark_doc_extract: match legacy Stardoc behavior for
 private aspect attrs and attr.license()

For private aspect propagation attrs, legacy Stardoc does not emit documentation.

For the deprecated attr.license() attributes, legacy Stardoc pretends the attribute
is a string list, so we do the same. (Note that we have to finally fix
License.repr() to return something sensible that can be parsed by
BuildType.LICENSE.convert() - i.e. by License.parseLicense() - because
starlark_doc_extract, unlike legacy Stardoc, correctly processes attribute default
values.)

PiperOrigin-RevId: 543457875
Change-Id: I5d01312e30ccdc8b1d71322f496ec8a237fa993d
---
 .../devtools/build/lib/packages/License.java  | 28 +++++++++--
 .../ModuleInfoExtractor.java                  | 12 ++++-
 .../build/lib/packages/LicenseTest.java       | 46 +++++++++++++++++++
 .../ModuleInfoExtractorTest.java              | 19 +++++++-
 .../lib/starlark/StarlarkIntegrationTest.java |  2 +-
 5 files changed, 98 insertions(+), 9 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/License.java b/src/main/java/com/google/devtools/build/lib/packages/License.java
index 0f3c3bf06efdcd..4fe246a4224c8c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/License.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/License.java
@@ -14,6 +14,9 @@
 
 package com.google.devtools.build.lib.packages;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.common.base.Ascii;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -27,6 +30,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Set;
+import java.util.stream.Stream;
 import net.starlark.java.eval.Printer;
 
 /** Support for license and distribution checking. */
@@ -35,6 +39,7 @@
 public final class License implements LicenseApi {
   private final ImmutableSet<LicenseType> licenseTypes;
   private final ImmutableSet<Label> exceptions;
+  private static final String EXCEPTION_PREFIX = "exception=";
 
   /**
    * The error that's thrown if a build file contains an invalid license string.
@@ -161,9 +166,9 @@ public static License parseLicense(List<String> licStrings) throws LicenseParsin
     Set<LicenseType> licenseTypes = EnumSet.noneOf(LicenseType.class);
     Set<Label> exceptions = Sets.newTreeSet();
     for (String str : licStrings) {
-      if (str.startsWith("exception=")) {
+      if (str.startsWith(EXCEPTION_PREFIX)) {
         try {
-          Label label = Label.parseCanonical(str.substring("exception=".length()));
+          Label label = Label.parseCanonical(str.substring(EXCEPTION_PREFIX.length()));
           exceptions.add(label);
         } catch (LabelSyntaxException e) {
           throw new LicenseParsingException(e.getMessage());
@@ -210,9 +215,9 @@ public boolean isSpecified() {
   @Override
   public String toString() {
     if (exceptions.isEmpty()) {
-      return licenseTypes.toString().toLowerCase();
+      return Ascii.toLowerCase(licenseTypes.toString());
     } else {
-      return licenseTypes.toString().toLowerCase() + " with exceptions " + exceptions;
+      return Ascii.toLowerCase(licenseTypes.toString()) + " with exceptions " + exceptions;
     }
   }
 
@@ -241,8 +246,21 @@ public boolean isImmutable() {
     return true; // licences are Starlark-hashable
   }
 
+  /**
+   * Represents the License as a canonically ordered list of strings that can be parsed by {@link
+   * License#parseLicense} to get back an equal License.
+   */
   @Override
   public void repr(Printer printer) {
-    printer.append(this.toString());
+    // The order of license types is guaranteed to be canonical by EnumSet, and the order of
+    // exceptions is guaranteed to be lexicographic order by TreeSet.
+    printer.printList(
+        Stream.concat(
+                licenseTypes.stream().map(licenseType -> Ascii.toLowerCase(licenseType.toString())),
+                exceptions.stream().map(label -> EXCEPTION_PREFIX + label.getCanonicalForm()))
+            .collect(toImmutableList()),
+        "[",
+        ", ",
+        "]");
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java
index f71962b1829313..1db8b65e22ef60 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java
@@ -383,7 +383,11 @@ protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect)
               .setFile(
                   aspect.getAspectClass().getExtensionLabel().getDisplayForm(repositoryMapping)));
       aspect.getDocumentation().ifPresent(aspectInfoBuilder::setDocString);
-      aspectInfoBuilder.addAllAspectAttribute(aspect.getAttributeAspects());
+      for (String aspectAttribute : aspect.getAttributeAspects()) {
+        if (isPublicName(aspectAttribute)) {
+          aspectInfoBuilder.addAspectAttribute(aspectAttribute);
+        }
+      }
       aspectInfoBuilder.addAttribute(IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first
       for (Attribute attribute : aspect.getAttributes()) {
         if (isPublicName(attribute.getPublicName())) {
@@ -472,6 +476,12 @@ private static AttributeType getAttributeType(Attribute attribute, String where)
         return AttributeType.OUTPUT;
       } else if (type.equals(BuildType.OUTPUT_LIST)) {
         return AttributeType.OUTPUT_LIST;
+      } else if (type.equals(BuildType.LICENSE)) {
+        // TODO(https://github.com/bazelbuild/bazel/issues/6420): deprecated, disabled in Bazel by
+        // default, broken and with almost no remaining users, so we don't have an AttributeType for
+        // it. Until this type is removed, following the example of legacy Stardoc, pretend it's a
+        // list of strings.
+        return AttributeType.STRING_LIST;
       }
 
       throw new ExtractionException(
diff --git a/src/test/java/com/google/devtools/build/lib/packages/LicenseTest.java b/src/test/java/com/google/devtools/build/lib/packages/LicenseTest.java
index 845f0b519cf8de..5db294d21d6875 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/LicenseTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/LicenseTest.java
@@ -15,8 +15,17 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.packages.License.LicenseType;
 import java.util.Arrays;
+import net.starlark.java.eval.Module;
+import net.starlark.java.eval.Mutability;
+import net.starlark.java.eval.Sequence;
+import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkSemantics;
+import net.starlark.java.eval.StarlarkThread;
+import net.starlark.java.syntax.FileOptions;
+import net.starlark.java.syntax.ParserInput;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -35,4 +44,41 @@ public void testLeastRestrictive() {
     assertThat(License.leastRestrictive(Arrays.<LicenseType>asList()))
         .isEqualTo(LicenseType.BY_EXCEPTION_ONLY);
   }
+
+  /** Evaluates a string as a Starlark expression returning a sequence of strings. */
+  private static Sequence<String> evalAsSequence(String string) throws Exception {
+    ParserInput input = ParserInput.fromLines(string);
+    Mutability mutability = Mutability.create("test");
+    Object parsedValue =
+        Starlark.execFile(
+            input,
+            FileOptions.DEFAULT,
+            Module.create(),
+            new StarlarkThread(mutability, StarlarkSemantics.DEFAULT));
+    mutability.freeze();
+    return Sequence.cast(parsedValue, String.class, "evalAsSequence() input");
+  }
+
+  @Test
+  public void repr() throws Exception {
+    assertThat(Starlark.repr(License.NO_LICENSE)).isEqualTo("[\"none\"]");
+    assertThat(License.parseLicense(evalAsSequence(Starlark.repr(License.NO_LICENSE))))
+        .isEqualTo(License.NO_LICENSE);
+
+    License withoutExceptions = License.parseLicense(ImmutableList.of("notice", "restricted"));
+    // License types sorted by LicenseType enum order.
+    assertThat(Starlark.repr(withoutExceptions)).isEqualTo("[\"restricted\", \"notice\"]");
+    assertThat(License.parseLicense(evalAsSequence(Starlark.repr(withoutExceptions))))
+        .isEqualTo(withoutExceptions);
+
+    License withExceptions =
+        License.parseLicense(
+            ImmutableList.of("notice", "restricted", "exception=//foo:bar", "exception=//baz:qux"));
+    // Exceptions sorted alphabetically.
+    assertThat(Starlark.repr(withExceptions))
+        .isEqualTo(
+            "[\"restricted\", \"notice\", \"exception=//baz:qux\", \"exception=//foo:bar\"]");
+    assertThat(License.parseLicense(evalAsSequence(Starlark.repr(withExceptions))))
+        .isEqualTo(withExceptions);
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractorTest.java b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractorTest.java
index 5e2af0916dc475..b72629052eb77c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractorTest.java
@@ -18,6 +18,7 @@
 import static com.google.common.truth.Truth8.assertThat;
 import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.BazelModuleContext;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -54,7 +55,12 @@ public final class ModuleInfoExtractorTest {
   private String fakeLabelString = null; // set by exec()
 
   private Module exec(String... lines) throws Exception {
+    return execWithOptions(ImmutableList.of(), lines);
+  }
+
+  private Module execWithOptions(ImmutableList<String> options, String... lines) throws Exception {
     BazelEvaluationTestCase ev = new BazelEvaluationTestCase();
+    ev.setSemantics(options.toArray(new String[0]));
     Module module = ev.getModule();
     Label fakeLabel = BazelModuleContext.of(module).label();
     fakeLabelString = fakeLabel.getCanonicalForm();
@@ -434,7 +440,10 @@ public void ruleAdvertisedProviders() throws Exception {
   @Test
   public void ruleAttributes() throws Exception {
     Module module =
-        exec(
+        execWithOptions(
+            // TODO(https://github.com/bazelbuild/bazel/issues/6420): attr.license() is deprecated,
+            // and will eventually be removed from Bazel.
+            ImmutableList.of("--noincompatible_no_attr_license"),
             "MyInfo1 = provider()",
             "MyInfo2 = provider()",
             "MyInfo3 = provider()",
@@ -448,6 +457,7 @@ public void ruleAttributes() throws Exception {
             "        'c': attr.label(providers = [MyInfo1, MyInfo2]),",
             "        'd': attr.label(providers = [[MyInfo1, MyInfo2], [MyInfo3]]),",
             "        '_e': attr.string(doc = 'Hidden attribute'),",
+            "        'deprecated_license': attr.license(),",
             "    }",
             ")");
     ModuleInfo moduleInfo = getExtractor().extractFrom(module);
@@ -495,6 +505,11 @@ public void ruleAttributes() throws Exception {
                         .addProviderName("MyInfo3")
                         .addOriginKey(
                             OriginKey.newBuilder().setName("MyInfo3").setFile(fakeLabelString)))
+                .build(),
+            AttributeInfo.newBuilder()
+                .setName("deprecated_license")
+                .setType(AttributeType.STRING_LIST)
+                .setDefaultValue("[\"none\"]")
                 .build());
   }
 
@@ -710,7 +725,7 @@ public void aspectAttributes() throws Exception {
             "    pass",
             "my_aspect = aspect(",
             "    implementation = _my_impl,",
-            "    attr_aspects = ['deps', 'srcs'],",
+            "    attr_aspects = ['deps', 'srcs', '_private'],",
             "    attrs = {",
             "        'a': attr.string(doc = 'My doc', default = 'foo'),",
             "        'b': attr.string(mandatory = True),",
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
index 9f41befe89f28b..b698efac1d597f 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
@@ -3112,7 +3112,7 @@ public void testLicenseType() throws Exception {
 
     getConfiguredTarget("//test:test");
 
-    assertContainsEvent("[none]");
+    assertContainsEvent("[\"none\"]");
   }
 
   @Test