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()