From b77e8c2a99ab37b8ef58f08a29f1e703ec15a246 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 11 Apr 2023 14:24:51 -0700 Subject: [PATCH] starlark_doc_extract: stringify label-valued default attribute values For human-readable docs, want to stringify labels in a way that (1) elides the name part when unambiguous, matching buildifier behavior; and (2) respects repo mapping, so we don't print full bzlmod canonical repo names. Unfortunately, this means introducing yet another stringification method to Label, which combines the behavior of getDisplayForm() and toShorthandString() but cannot be conveniently composed from them. Alas. PiperOrigin-RevId: 523504039 Change-Id: Ia09ebb77cff09aaeb270f43ed7e319c90a44aa52 --- .../devtools/build/lib/cmdline/Label.java | 32 ++++++- .../build/lib/cmdline/PackageIdentifier.java | 22 +---- .../build/lib/cmdline/RepositoryName.java | 39 ++++++++ .../build/lib/packages/Attribute.java | 1 - .../build/lib/rules/starlarkdocextract/BUILD | 1 + .../ModuleInfoExtractor.java | 94 +++++++++++++++---- .../StarlarkDocExtract.java | 31 +++++- .../devtools/build/lib/cmdline/LabelTest.java | 71 ++++++++++++++ .../build/lib/cmdline/RepositoryNameTest.java | 14 +++ .../build/lib/rules/starlarkdocextract/BUILD | 1 + .../ModuleInfoExtractorTest.java | 85 ++++++++++++----- 11 files changed, 329 insertions(+), 62 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index 58dde63a9247f4..a5951fc8d28d75 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java @@ -390,7 +390,7 @@ public String getUnambiguousCanonicalForm() { } /** - * Returns a label string that is suitable for display, i.e., it resolves to this label when + * Returns a full label string that is suitable for display, i.e., it resolves to this label when * parsed in the context of the main repository and has a repository part that is as simple as * possible. * @@ -401,6 +401,34 @@ public String getDisplayForm(RepositoryMapping mainRepositoryMapping) { return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name; } + /** + * Returns a shorthand label string that is suitable for display, i.e. in addition to simplifying + * the repository part, labels of the form {@code [@repo]//foo/bar:bar} are simplified to the + * shorthand form {@code [@repo]//foo/bar}, and labels of the form {@code @repo//:repo} and + * {@code @@repo//:repo} are simplified to {@code @repo}. The returned shorthand string resolves + * back to this label only when parsed in the context of the main repository whose repository + * mapping is provided. + * + *

Unlike {@link #getDisplayForm}, this method elides the name part of the label if possible. + * + *

Unlike {@link #toShorthandString}, this method respects {@link RepositoryMapping}. + * + * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository + */ + public String getShorthandDisplayForm(RepositoryMapping mainRepositoryMapping) { + if (getPackageFragment().getBaseName().equals(name)) { + return packageIdentifier.getDisplayForm(mainRepositoryMapping); + } else if (getPackageFragment().getBaseName().isEmpty()) { + String repositoryDisplayForm = + getPackageIdentifier().getRepository().getDisplayForm(mainRepositoryMapping); + // Simplify @foo//:foo or @@foo//:foo to @foo; note that `name` cannot start with '@' + if (repositoryDisplayForm.equals("@" + name) || repositoryDisplayForm.equals("@@" + name)) { + return repositoryDisplayForm; + } + } + return getDisplayForm(mainRepositoryMapping); + } + /** Return the name of the repository label refers to without the leading `at` symbol. */ @StarlarkMethod( name = "workspace_name", @@ -419,6 +447,8 @@ public String getWorkspaceName() throws EvalException { * *

Labels with canonical form {@code //foo/bar:bar} have the shorthand form {@code //foo/bar}. * All other labels have identical shorthand and canonical forms. + * + *

Unlike {@link #getShorthandDisplayForm}, this method does not respect repository mapping. */ public String toShorthandString() { if (!getPackageFragment().getBaseName().equals(name)) { diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java index b945c6198381b0..4388d38154e4b0 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java @@ -189,7 +189,7 @@ public String getUnambiguousCanonicalForm() { /** * Returns a label representation for this package that is suitable for display. The returned * string is as simple as possible while referencing the current package when parsed in the - * context of the main repository. + * context of the main repository whose repository mapping is provided. * * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository * @return @@ -204,24 +204,8 @@ public String getUnambiguousCanonicalForm() { * from the main module */ public String getDisplayForm(RepositoryMapping mainRepositoryMapping) { - Preconditions.checkArgument( - mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain()); - if (repository.isMain()) { - // Packages in the main repository can always use repo-relative form. - return "//" + getPackageFragment(); - } - if (!mainRepositoryMapping.usesStrictDeps()) { - // If the main repository mapping is not using strict visibility, then Bzlmod is certainly - // disabled, which means that canonical and apparent names can be used interchangeably from - // the context of the main repository. - return repository.getNameWithAt() + "//" + getPackageFragment(); - } - // If possible, represent the repository with a non-canonical label using the apparent name the - // main repository has for it, otherwise fall back to a canonical label. - return mainRepositoryMapping - .getInverse(repository) - .map(apparentName -> "@" + apparentName + "//" + getPackageFragment()) - .orElseGet(this::getUnambiguousCanonicalForm); + return String.format( + "%s//%s", getRepository().getDisplayForm(mainRepositoryMapping), getPackageFragment()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java index 2f9f6a0ac8b025..5b564ee2faaa91 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java @@ -230,6 +230,45 @@ public String getCanonicalForm() { return isMain() ? "" : getNameWithAt(); } + /** + * Returns the repository part of a {@link Label}'s string representation suitable for display. + * The returned string is as simple as possible in the context of the main repo whose repository + * mapping is provided: an empty string for the main repo, or a string prefixed with a leading + * "{@literal @}" or "{@literal @@}" otherwise. + * + * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository + * @return + *

+ *
the empty string + *
if this is the main repository + *
@protobuf + *
if this repository is a WORKSPACE dependency and its name is "protobuf", + * or if this repository is a Bzlmod dependency of the main module and its apparent name + * is "protobuf" + *
@@protobuf~3.19.2 + *
only with Bzlmod, if this a repository that is not visible from the main module + */ + public String getDisplayForm(RepositoryMapping mainRepositoryMapping) { + Preconditions.checkArgument( + mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain()); + if (isMain()) { + // Packages in the main repository can always use repo-relative form. + return ""; + } + if (!mainRepositoryMapping.usesStrictDeps()) { + // If the main repository mapping is not using strict visibility, then Bzlmod is certainly + // disabled, which means that canonical and apparent names can be used interchangeably from + // the context of the main repository. + return getNameWithAt(); + } + // If possible, represent the repository with a non-canonical label using the apparent name the + // main repository has for it, otherwise fall back to a canonical label. + return mainRepositoryMapping + .getInverse(this) + .map(apparentName -> "@" + apparentName) + .orElse("@" + getNameWithAt()); + } + /** * Returns the runfiles/execRoot path for this repository. If we don't know the name of this repo * (i.e., it is in the main repository), return an empty path fragment. diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index cc32c2da2a54d2..8be96d9681bb60 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -2084,7 +2084,6 @@ public Object getDefaultValue() { * Returns the default value of this attribute, even if it is a computed default, or a late-bound * default. */ - @VisibleForTesting public Object getDefaultValueUnchecked() { return defaultValue; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD index 8d46e2657c761b..4bd1154d9ee574 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD @@ -29,6 +29,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", "//src/main/java/com/google/devtools/build/lib/util:filetype", "//src/main/java/com/google/devtools/build/skydoc/rendering:function_util", 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 9847ef2fae29f5..8d0ee2691cab2e 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 @@ -15,9 +15,12 @@ package com.google.devtools.build.lib.rules.starlarkdocextract; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.StarlarkRuleFunction; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.RuleClass; @@ -35,17 +38,23 @@ import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderInfo; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderNameGroup; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.RuleInfo; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.Predicate; +import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Module; import net.starlark.java.eval.Printer; import net.starlark.java.eval.StarlarkFunction; +import net.starlark.java.eval.StarlarkList; import net.starlark.java.eval.Structure; /** API documentation extractor for a compiled, loaded Starlark module. */ final class ModuleInfoExtractor { + private final Predicate isWantedName; + private final RepositoryMapping repositoryMapping; + @VisibleForTesting static final AttributeInfo IMPLICIT_NAME_ATTRIBUTE_INFO = AttributeInfo.newBuilder() @@ -59,14 +68,21 @@ final class ModuleInfoExtractor { // FakeRepositoryModule currently does? /** - * Extracts structured documentation for the exported symbols (meaning symbols whose first - * character is alphabetic) of a given module. + * Constructs an instance of {@code ModuleInfoExtractor}. * - * @param isWantedName a filter applied to symbols; only those symbols for which the filter - * returns true will be documented + * @param isWantedName a filter applied to symbols names; only those symbols which both are + * exportable (meaning the first character is alphabetic) and for which the filter returns + * true will be documented + * @param repositoryMapping the repository mapping for the repo in which we want to render labels + * as strings */ - public static ModuleInfo extractFrom(Module module, Predicate isWantedName) - throws ExtractionException { + public ModuleInfoExtractor(Predicate isWantedName, RepositoryMapping repositoryMapping) { + this.isWantedName = isWantedName; + this.repositoryMapping = repositoryMapping; + } + + /** Extracts structured documentation for the exported symbols of a given module. */ + public ModuleInfo extractFrom(Module module) throws ExtractionException { ModuleInfo.Builder builder = ModuleInfo.newBuilder(); Optional.ofNullable(module.getDocumentation()).ifPresent(builder::setModuleDocstring); for (var entry : module.getGlobals().entrySet()) { @@ -103,9 +119,8 @@ public ExtractionException(String message, Throwable cause) { * for field bar of exported struct foo * @param value documentable Starlark value */ - private static void addInfo(ModuleInfo.Builder builder, String name, Object value) + private void addInfo(ModuleInfo.Builder builder, String name, Object value) throws ExtractionException { - // Note that may be exported under a different name than its getName() value. if (value instanceof StarlarkRuleFunction) { addRuleInfo(builder, name, (StarlarkRuleFunction) value); } else if (value instanceof StarlarkProvider) { @@ -126,7 +141,7 @@ private static void addInfo(ModuleInfo.Builder builder, String name, Object valu // TODO(b/276733504): should we recurse into dicts to search for documentable values? } - private static void addStructureInfo(ModuleInfo.Builder builder, String name, Structure structure) + private void addStructureInfo(ModuleInfo.Builder builder, String name, Structure structure) throws ExtractionException { for (String fieldName : structure.getFieldNames()) { if (isExportableName(fieldName)) { @@ -182,7 +197,54 @@ private static AttributeType getAttributeType(Attribute attribute, String where) where, attribute.getPublicName(), type.getClass().getSimpleName())); } - private static AttributeInfo buildAttributeInfo(Attribute attribute, String where) + /** + * Recursively transforms labels to strings via {@link Label#getShorthandDisplayForm}. + * + * @return the label's shorthand display string if {@code o} is a label; a container with label + * elements transformed into shorthand display strings recursively if {@code o} is a Starlark + * container; or the original object {@code o} if no label stringification was performed. + */ + private Object stringifyLabels(Object o) { + if (o instanceof Label) { + return ((Label) o).getShorthandDisplayForm(repositoryMapping); + } else if (o instanceof Map) { + return stringifyLabelsOfMap((Map) o); + } else if (o instanceof List) { + return stringifyLabelsOfList((List) o); + } else { + return o; + } + } + + private Object stringifyLabelsOfMap(Map dict) { + boolean neededToStringify = false; + ImmutableMap.Builder builder = ImmutableMap.builder(); + for (Map.Entry entry : dict.entrySet()) { + Object keyWithStringifiedLabels = stringifyLabels(entry.getKey()); + Object valueWithStringifiedLabels = stringifyLabels(entry.getValue()); + if (keyWithStringifiedLabels != entry.getKey() + || valueWithStringifiedLabels != entry.getValue() /* as Objects */) { + neededToStringify = true; + } + builder.put(keyWithStringifiedLabels, valueWithStringifiedLabels); + } + return neededToStringify ? Dict.immutableCopyOf(builder.buildOrThrow()) : dict; + } + + private Object stringifyLabelsOfList(List list) { + boolean neededToStringify = false; + ImmutableList.Builder builder = ImmutableList.builder(); + for (Object element : list) { + Object elementWithStringifiedLabels = stringifyLabels(element); + if (elementWithStringifiedLabels != element /* as Objects */) { + neededToStringify = true; + } + builder.add(elementWithStringifiedLabels); + } + return neededToStringify ? StarlarkList.immutableCopyOf(builder.build()) : list; + } + + private AttributeInfo buildAttributeInfo(Attribute attribute, String where) throws ExtractionException { AttributeInfo.Builder builder = AttributeInfo.newBuilder(); builder.setName(attribute.getPublicName()); @@ -201,15 +263,13 @@ private static AttributeInfo buildAttributeInfo(Attribute attribute, String wher } if (!attribute.isMandatory()) { - // TODO(b/276733504): deeply canonicalize label objects to strings - Object defaultValue = attribute.getDefaultValueUnchecked(); - builder.setDefaultValue( - new Printer().repr(Attribute.valueToStarlark(defaultValue)).toString()); + Object defaultValue = Attribute.valueToStarlark(attribute.getDefaultValueUnchecked()); + builder.setDefaultValue(new Printer().repr(stringifyLabels(defaultValue)).toString()); } return builder.build(); } - private static void addRuleInfo( + private void addRuleInfo( ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkRuleFunction ruleFunction) throws ExtractionException { RuleInfo.Builder ruleInfoBuilder = RuleInfo.newBuilder(); @@ -249,7 +309,7 @@ private static void addProviderInfo( moduleInfoBuilder.addProviderInfo(providerInfoBuilder); } - private static void addAspectInfo( + private void addAspectInfo( ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkDefinedAspect aspect) throws ExtractionException { AspectInfo.Builder aspectInfoBuilder = AspectInfo.newBuilder(); @@ -265,6 +325,4 @@ private static void addAspectInfo( } moduleInfoBuilder.addAspectInfo(aspectInfoBuilder); } - - private ModuleInfoExtractor() {} } diff --git a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java index e583b1c8011a50..0eec7165471546 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java +++ b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.starlarkdocextract; import static com.google.common.base.Verify.verify; +import static com.google.common.base.Verify.verifyNotNull; import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.fromTemplates; import static com.google.devtools.build.lib.packages.Type.STRING_LIST; @@ -44,6 +45,8 @@ import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException; import com.google.devtools.build.lib.skyframe.BzlLoadValue; +import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; +import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ModuleInfo; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.common.options.Option; @@ -162,10 +165,34 @@ private static Module loadModule(RuleContext ruleContext) } private static ModuleInfo getModuleInfo(RuleContext ruleContext, Module module) - throws RuleErrorException { + throws RuleErrorException, InterruptedException { + RepositoryMappingValue repositoryMappingValue; + try { + // We get the starlark_doc_extract target's repository's repo mapping to ensure label + // stringification does not change regardless of whether we are the main repo or a dependency. + // However, this does mean that label stringifactions we produce could be invalid in the main + // repo. + repositoryMappingValue = + (RepositoryMappingValue) + ruleContext + .getAnalysisEnvironment() + .getSkyframeEnv() + .getValueOrThrow( + RepositoryMappingValue.key(ruleContext.getRepository()), + RepositoryMappingResolutionException.class); + } catch (RepositoryMappingResolutionException e) { + ruleContext.ruleError(e.getMessage()); + throw new RuleErrorException(e); + } + verifyNotNull(repositoryMappingValue); + ModuleInfo moduleInfo; try { - moduleInfo = ModuleInfoExtractor.extractFrom(module, getWantedSymbolPredicate(ruleContext)); + moduleInfo = + new ModuleInfoExtractor( + getWantedSymbolPredicate(ruleContext), + repositoryMappingValue.getRepositoryMapping()) + .extractFrom(module); } catch (ModuleInfoExtractor.ExtractionException e) { ruleContext.ruleError(e.getMessage()); throw new RuleErrorException(e); diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java index 2d3587c3647199..592656337f9008 100644 --- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java +++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java @@ -396,6 +396,77 @@ public void testUnambiguousCanonicalForm() throws Exception { .isEqualTo("@@[unknown repo 'foo' requested from @bar]//baz:quux"); } + private static String displayFormFor(String rawLabel, RepositoryMapping repositoryMapping) + throws Exception { + return Label.parseCanonical(rawLabel).getDisplayForm(repositoryMapping); + } + + @Test + public void testDisplayForm() throws Exception { + RepositoryName canonicalName = RepositoryName.create("canonical"); + RepositoryMapping repositoryMapping = + RepositoryMapping.create(ImmutableMap.of("local", canonicalName), RepositoryName.MAIN); + + assertThat(displayFormFor("//foo/bar:bar", repositoryMapping)).isEqualTo("//foo/bar:bar"); + assertThat(displayFormFor("//foo/bar:baz", repositoryMapping)).isEqualTo("//foo/bar:baz"); + + assertThat(displayFormFor("@canonical//bar:bar", repositoryMapping)) + .isEqualTo("@local//bar:bar"); + assertThat(displayFormFor("@canonical//bar:baz", repositoryMapping)) + .isEqualTo("@local//bar:baz"); + assertThat(displayFormFor("@canonical//:canonical", repositoryMapping)) + .isEqualTo("@local//:canonical"); + assertThat(displayFormFor("@canonical//:local", repositoryMapping)).isEqualTo("@local//:local"); + + assertThat(displayFormFor("@other//bar:bar", repositoryMapping)).isEqualTo("@@other//bar:bar"); + assertThat(displayFormFor("@other//bar:baz", repositoryMapping)).isEqualTo("@@other//bar:baz"); + assertThat(displayFormFor("@other//:other", repositoryMapping)).isEqualTo("@@other//:other"); + assertThat(displayFormFor("@@other", repositoryMapping)).isEqualTo("@@other//:other"); + + assertThat(displayFormFor("@unremapped//:unremapped", RepositoryMapping.ALWAYS_FALLBACK)) + .isEqualTo("@unremapped//:unremapped"); + assertThat(displayFormFor("@unremapped", RepositoryMapping.ALWAYS_FALLBACK)) + .isEqualTo("@unremapped//:unremapped"); + } + + private static String shorthandDisplayFormFor( + String rawLabel, RepositoryMapping repositoryMapping) throws Exception { + return Label.parseCanonical(rawLabel).getShorthandDisplayForm(repositoryMapping); + } + + @Test + public void testShorthandDisplayForm() throws Exception { + RepositoryName canonicalName = RepositoryName.create("canonical"); + RepositoryMapping repositoryMapping = + RepositoryMapping.create(ImmutableMap.of("local", canonicalName), RepositoryName.MAIN); + + assertThat(shorthandDisplayFormFor("//foo/bar:bar", repositoryMapping)).isEqualTo("//foo/bar"); + assertThat(shorthandDisplayFormFor("//foo/bar:baz", repositoryMapping)) + .isEqualTo("//foo/bar:baz"); + + assertThat(shorthandDisplayFormFor("@canonical//bar:bar", repositoryMapping)) + .isEqualTo("@local//bar"); + assertThat(shorthandDisplayFormFor("@canonical//bar:baz", repositoryMapping)) + .isEqualTo("@local//bar:baz"); + assertThat(shorthandDisplayFormFor("@canonical//:canonical", repositoryMapping)) + .isEqualTo("@local//:canonical"); + assertThat(shorthandDisplayFormFor("@canonical//:local", repositoryMapping)) + .isEqualTo("@local"); + + assertThat(shorthandDisplayFormFor("@other//bar:bar", repositoryMapping)) + .isEqualTo("@@other//bar"); + assertThat(shorthandDisplayFormFor("@other//bar:baz", repositoryMapping)) + .isEqualTo("@@other//bar:baz"); + assertThat(shorthandDisplayFormFor("@other//:other", repositoryMapping)).isEqualTo("@@other"); + assertThat(shorthandDisplayFormFor("@@other", repositoryMapping)).isEqualTo("@@other"); + + assertThat( + shorthandDisplayFormFor("@unremapped//:unremapped", RepositoryMapping.ALWAYS_FALLBACK)) + .isEqualTo("@unremapped"); + assertThat(shorthandDisplayFormFor("@unremapped", RepositoryMapping.ALWAYS_FALLBACK)) + .isEqualTo("@unremapped"); + } + @Test public void starlarkStrAndRepr() throws Exception { Label label = Label.parseCanonical("//x"); diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java index 56e44fe4886c81..c55abcb141876e 100644 --- a/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java +++ b/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.vfs.PathFragment; import net.starlark.java.eval.EvalException; import org.junit.Test; @@ -80,4 +81,17 @@ public void testGetDefaultCanonicalForm() throws Exception { assertThat(RepositoryName.create("").getCanonicalForm()).isEqualTo(""); assertThat(RepositoryName.create("foo").getCanonicalForm()).isEqualTo("@foo"); } + + @Test + public void testGetDisplayForm() throws Exception { + RepositoryMapping repositoryMapping = + RepositoryMapping.create( + ImmutableMap.of("local", RepositoryName.create("canonical")), RepositoryName.MAIN); + + assertThat(RepositoryName.create("").getDisplayForm(repositoryMapping)).isEmpty(); + assertThat(RepositoryName.create("canonical").getDisplayForm(repositoryMapping)) + .isEqualTo("@local"); + assertThat(RepositoryName.create("other").getDisplayForm(repositoryMapping)) + .isEqualTo("@@other"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD index fc5de21a461185..d68f8a2f0c7b40 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD @@ -29,6 +29,7 @@ java_library( "//src/main/java/net/starlark/java/syntax", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/starlark/util", + "//third_party:guava", "//third_party:junit4", "//third_party:truth", "//third_party/protobuf:protobuf_java", 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 24aec1997ff731..4eb09e7164ccc6 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,7 +18,10 @@ import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.BzlLoadFunction; import com.google.devtools.build.lib.starlark.util.BazelEvaluationTestCase; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.AspectInfo; @@ -33,6 +36,7 @@ import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderNameGroup; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.RuleInfo; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.StarlarkFunctionInfo; +import java.util.function.Predicate; import net.starlark.java.eval.Module; import net.starlark.java.syntax.FileOptions; import net.starlark.java.syntax.ParserInput; @@ -57,18 +61,26 @@ private Module exec(String... lines) throws Exception { return ev.getModule(); } + private static ModuleInfoExtractor getExtractor() { + return new ModuleInfoExtractor(name -> true, RepositoryMapping.ALWAYS_FALLBACK); + } + + private static ModuleInfoExtractor getExtractor(Predicate isWantedName) { + return new ModuleInfoExtractor(isWantedName, RepositoryMapping.ALWAYS_FALLBACK); + } + + private static ModuleInfoExtractor getExtractor(RepositoryMapping repositoryMapping) { + return new ModuleInfoExtractor(name -> true, repositoryMapping); + } + @Test public void moduleDocstring() throws Exception { Module moduleWithDocstring = exec("'''This is my docstring'''", "foo = 1"); - assertThat( - ModuleInfoExtractor.extractFrom(moduleWithDocstring, name -> true).getModuleDocstring()) + assertThat(getExtractor().extractFrom(moduleWithDocstring).getModuleDocstring()) .isEqualTo("This is my docstring"); Module moduleWithoutDocstring = exec("foo = 1"); - assertThat( - ModuleInfoExtractor.extractFrom(moduleWithoutDocstring, name -> true) - .getModuleDocstring()) - .isEmpty(); + assertThat(getExtractor().extractFrom(moduleWithoutDocstring).getModuleDocstring()).isEmpty(); } @Test @@ -84,8 +96,7 @@ public void extractOnlyWantedExportableNames() throws Exception { "def _nonexported_matches_wanted_predicate():", " pass"); - ModuleInfo moduleInfo = - ModuleInfoExtractor.extractFrom(module, name -> name.contains("_wanted")); + ModuleInfo moduleInfo = getExtractor(name -> name.contains("_wanted")).extractFrom(module); assertThat(moduleInfo.getFuncInfoList().stream().map(StarlarkFunctionInfo::getFunctionName)) .containsExactly("exported_wanted"); } @@ -107,7 +118,7 @@ public void namespaces() throws Exception { " MyInfo = _MyInfo,", " ),", ")"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getFuncInfoList().stream().map(StarlarkFunctionInfo::getFunctionName)) .containsExactly("name.spaced.my_func"); assertThat(moduleInfo.getRuleInfoList().stream().map(RuleInfo::getRuleName)) @@ -127,7 +138,7 @@ public void functionDocstring() throws Exception { " pass", "def without_docstring():", " pass"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getFuncInfoList()) // TODO(arostovtsev): remove `ignoringFieldAbsence` below once we stop outputting empty // returns/deprecated blocks as empty strings. @@ -154,7 +165,7 @@ public void functionParams() throws Exception { " documented: Documented param", " '''", " pass"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getFuncInfoList().get(0).getParameterList()) .containsExactly( FunctionParamInfo.newBuilder() @@ -185,7 +196,7 @@ public void functionReturn() throws Exception { "def without_return():", " '''My doc'''", " pass"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getFuncInfoList()) .ignoringFieldAbsence() // TODO(arostovtsev): do not output empty returns/deprecated blocks .containsExactly( @@ -214,7 +225,7 @@ public void functionDeprecated() throws Exception { "def without_deprecated():", " '''My doc'''", " pass"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getFuncInfoList()) .ignoringFieldAbsence() // TODO(arostovtsev): do not output empty returns/deprecated blocks .containsExactly( @@ -236,7 +247,7 @@ public void providerDocstring() throws Exception { exec( "DocumentedInfo = provider(doc = 'My doc')", // "UndocumentedInfo = provider()"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getProviderInfoList()) .containsExactly( ProviderInfo.newBuilder() @@ -252,7 +263,7 @@ public void providerFields() throws Exception { exec( "DocumentedInfo = provider(fields = {'a': 'A', 'b': 'B', '_hidden': 'Hidden'})", "UndocumentedInfo = provider(fields = ['a', 'b', '_hidden'])"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getProviderInfoList()) .containsExactly( ProviderInfo.newBuilder() @@ -275,7 +286,7 @@ public void ruleDocstring() throws Exception { " pass", "documented_lib = rule(doc = 'My doc', implementation = _my_impl)", "undocumented_lib = rule(implementation = _my_impl)"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getRuleInfoList()) .ignoringFields(RuleInfo.ATTRIBUTE_FIELD_NUMBER) // ignore implicit attributes .containsExactly( @@ -302,7 +313,7 @@ public void ruleAttributes() throws Exception { " '_e': attr.string(doc = 'Hidden attribute'),", " }", ")"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getRuleInfoList().get(0).getAttributeList()) .containsExactly( ModuleInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO, @@ -352,7 +363,7 @@ public void attributeOrder() throws Exception { " 'baz': attr.int(),", " }", ")"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat( moduleInfo.getRuleInfoList().get(0).getAttributeList().stream() .map(AttributeInfo::getName)) @@ -383,7 +394,7 @@ public void attributeTypes() throws Exception { " 'l': attr.output_list(),", " }", ")"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getRuleInfoList().get(0).getAttributeList()) .containsExactly( ModuleInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO, @@ -449,6 +460,38 @@ public void attributeTypes() throws Exception { .build()); } + @Test + public void labelStringification() throws Exception { + Module module = + exec( + "def _my_impl(ctx):", + " pass", + "my_lib = rule(", + " implementation = _my_impl,", + " attrs = {", + " 'label': attr.label(default = '//test:foo'),", + " 'label_list': attr.label_list(", + " default = ['//x', '@canonical//y', '@canonical//y:z'],", + " ),", + " 'label_keyed_string_dict': attr.label_keyed_string_dict(", + " default = {'//x': 'label_in_main', '@canonical//y': 'label_in_dep'}", + " ),", + " }", + ")"); + RepositoryName canonicalName = RepositoryName.create("canonical"); + RepositoryMapping repositoryMapping = + RepositoryMapping.create(ImmutableMap.of("local", canonicalName), RepositoryName.MAIN); + ModuleInfo moduleInfo = getExtractor(repositoryMapping).extractFrom(module); + assertThat( + moduleInfo.getRuleInfoList().get(0).getAttributeList().stream() + .filter(attr -> !attr.equals(ModuleInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO)) + .map(AttributeInfo::getDefaultValue)) + .containsExactly( + "\"//test:foo\"", + "[\"//x\", \"@local//y\", \"@local//y:z\"]", + "{\"//x\": \"label_in_main\", \"@local//y\": \"label_in_dep\"}"); + } + @Test public void aspectDocstring() throws Exception { Module module = @@ -457,7 +500,7 @@ public void aspectDocstring() throws Exception { " pass", "documented_aspect = aspect(doc = 'My doc', implementation = _my_impl)", "undocumented_aspect = aspect(implementation = _my_impl)"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getAspectInfoList()) .ignoringFields(AspectInfo.ATTRIBUTE_FIELD_NUMBER) // ignore implicit attributes .containsExactly( @@ -483,7 +526,7 @@ public void aspectAttributes() throws Exception { " '_c': attr.string(doc = 'Hidden attribute'),", " }", ")"); - ModuleInfo moduleInfo = ModuleInfoExtractor.extractFrom(module, name -> true); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getAspectInfoList()) .containsExactly( AspectInfo.newBuilder()