diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java index 001612d718f840..b512f9ce15b583 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java +++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java @@ -561,7 +561,7 @@ private static class RuleClassInfoFormatter { private final ExtractorContext extractorContext = ExtractorContext.builder() .labelRenderer(LabelRenderer.DEFAULT) - .extractNonStarlarkAttrs(true) + .extractNativelyDefinedAttrs(true) .build(); /** diff --git a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/AttributeInfoExtractor.java b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/AttributeInfoExtractor.java index 7efcfee6e150bf..d23a95a5975d3f 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/AttributeInfoExtractor.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/AttributeInfoExtractor.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.packages.Types; import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeInfo; import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeType; +import java.util.Map; import java.util.Optional; import java.util.function.Consumer; import net.starlark.java.eval.Starlark.InvalidStarlarkValueException; @@ -30,28 +31,6 @@ /** Starlark API documentation extractor for a rule, macro, or aspect attribute. */ @VisibleForTesting public final class AttributeInfoExtractor { - - @VisibleForTesting - public static final AttributeInfo IMPLICIT_NAME_ATTRIBUTE_INFO = - AttributeInfo.newBuilder() - .setName("name") - .setType(AttributeType.NAME) - .setMandatory(true) - .setDocString("A unique name for this target.") - .build(); - - @VisibleForTesting - public static final AttributeInfo IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO = - AttributeInfo.newBuilder() - .setName("name") - .setType(AttributeType.NAME) - .setMandatory(true) - .setDocString( - "A unique name for this macro instance. Normally, this is also the name for the" - + " macro's main or only target. The names of any other targets that this macro" - + " might create will be this name with a string suffix.") - .build(); - @VisibleForTesting public static final String UNREPRESENTABLE_VALUE = ""; static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attribute) { @@ -64,6 +43,9 @@ static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attr if (!attribute.isConfigurable()) { builder.setNonconfigurable(true); } + if (!attribute.starlarkDefined()) { + builder.setNativelyDefined(true); + } for (ImmutableSet providerGroup : attribute.getRequiredProviders().getStarlarkProviders()) { // TODO(b/290788853): it is meaningless to require a provider on an attribute of a @@ -83,14 +65,24 @@ static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attr return builder.build(); } + /** + * Adds {@code implicitAttributeInfos}, followed by documentable attributes from {@code + * attributes}. + */ static void addDocumentableAttributes( - ExtractorContext context, Iterable attributes, Consumer builder) { + ExtractorContext context, + Map implicitAttributeInfos, + Iterable attributes, + Consumer builder) { + // Inject implicit attributes first. + for (AttributeInfo implicitAttributeInfo : implicitAttributeInfos.values()) { + builder.accept(implicitAttributeInfo); + } for (Attribute attribute : attributes) { - if (attribute.getName().equals(IMPLICIT_NAME_ATTRIBUTE_INFO.getName())) { - // We inject our own IMPLICIT_NAME_ATTRIBUTE_INFO with better documentation. + if (implicitAttributeInfos.containsKey(attribute.getName())) { continue; } - if ((attribute.starlarkDefined() || context.extractNonStarlarkAttrs()) + if ((attribute.starlarkDefined() || context.extractNativelyDefinedAttrs()) && attribute.isDocumented() && ExtractorContext.isPublicName(attribute.getPublicName())) { builder.accept(buildAttributeInfo(context, attribute)); diff --git a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ExtractorContext.java b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ExtractorContext.java index 6be1cf33c0528b..50c33243f55ac2 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ExtractorContext.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ExtractorContext.java @@ -28,7 +28,7 @@ public record ExtractorContext( LabelRenderer labelRenderer, ImmutableMap providerQualifiedNames, - boolean extractNonStarlarkAttrs) { + boolean extractNativelyDefinedAttrs) { public ExtractorContext { checkNotNull(labelRenderer, "labelRenderer cannot be null."); @@ -39,7 +39,14 @@ public record ExtractorContext( public static Builder builder() { return new AutoBuilder_ExtractorContext_Builder() .providerQualifiedNames(ImmutableMap.of()) - .extractNonStarlarkAttrs(false); + .extractNativelyDefinedAttrs(false); + } + + public Builder toBuilder() { + return builder() + .labelRenderer(labelRenderer) + .providerQualifiedNames(providerQualifiedNames) + .extractNativelyDefinedAttrs(extractNativelyDefinedAttrs); } /** Builder for {@link ExtractorContext}. */ @@ -50,7 +57,7 @@ public interface Builder { Builder providerQualifiedNames( ImmutableMap providerQualifiedNames); - Builder extractNonStarlarkAttrs(boolean extractNonStarlarkAttrs); + Builder extractNativelyDefinedAttrs(boolean extractNativelyDefinedAttrs); ExtractorContext build(); } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractor.java b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractor.java index 186f89f1afa654..79e403784edea2 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractor.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractor.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.starlarkdocextract; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.MacroFunction; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.StarlarkRuleFunction; @@ -55,14 +54,43 @@ public final class ModuleInfoExtractor { private final LabelRenderer labelRenderer; @VisibleForTesting - public static final ImmutableList IMPLICIT_REPOSITORY_RULE_ATTRIBUTES = - ImmutableList.of( + public static final ImmutableMap IMPLICIT_MACRO_ATTRIBUTES = + ImmutableMap.of( + "name", + AttributeInfo.newBuilder() + .setName("name") + .setType(AttributeType.NAME) + .setMandatory(true) + .setDocString( + "A unique name for this macro instance. Normally, this is also the name for the" + + " macro's main or only target. The names of any other targets that this" + + " macro might create will be this name with a string suffix.") + .build(), + "visibility", + AttributeInfo.newBuilder() + .setName("visibility") + .setType(AttributeType.LABEL_LIST) + .setMandatory(false) + .setNonconfigurable(true) + .setNativelyDefined(true) + .setDocString( + "The visibility to be passed to this macro's exported targets. It always" + + " implicitly includes the location where this macro is instantiated, so" + + " this attribute only needs to be explicitly set if you want the macro's" + + " targets to be additionally visible somewhere else.") + .build()); + + @VisibleForTesting + public static final ImmutableMap IMPLICIT_REPOSITORY_RULE_ATTRIBUTES = + ImmutableMap.of( + "name", AttributeInfo.newBuilder() .setName("name") .setType(AttributeType.NAME) .setMandatory(true) .setDocString("A unique name for this repository.") .build(), + "repo_mapping", AttributeInfo.newBuilder() .setName("repo_mapping") .setType(AttributeType.STRING_DICT) @@ -346,10 +374,19 @@ protected void visitMacroFunction(String qualifiedName, MacroFunction macroFunct macroFunction.getDocumentation().ifPresent(macroInfoBuilder::setDocString); MacroClass macroClass = macroFunction.getMacroClass(); - // inject the name attribute; addDocumentableAttributes skips non-Starlark-defined attributes. - macroInfoBuilder.addAttribute(AttributeInfoExtractor.IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO); + if (macroClass.isFinalizer()) { + macroInfoBuilder.setFinalizer(true); + } + // For symbolic macros, always extract non-Starlark attributes (to support inherit_attrs). + ExtractorContext contextForImplicitMacroAttributes = + context.extractNativelyDefinedAttrs() + ? context + : context.toBuilder().extractNativelyDefinedAttrs(true).build(); AttributeInfoExtractor.addDocumentableAttributes( - context, macroClass.getAttributes().values(), macroInfoBuilder::addAttribute); + contextForImplicitMacroAttributes, + IMPLICIT_MACRO_ATTRIBUTES, + macroClass.getAttributes().values(), + macroInfoBuilder::addAttribute); moduleInfoBuilder.addMacroInfo(macroInfoBuilder); } @@ -419,10 +456,8 @@ protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect) aspectInfoBuilder.addAspectAttribute(aspectAttribute); } } - aspectInfoBuilder.addAttribute( - AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first AttributeInfoExtractor.addDocumentableAttributes( - context, aspect.getAttributes(), aspectInfoBuilder::addAttribute); + context, ImmutableMap.of(), aspect.getAttributes(), aspectInfoBuilder::addAttribute); moduleInfoBuilder.addAspectInfo(aspectInfoBuilder); } @@ -446,7 +481,10 @@ protected void visitModuleExtension(String qualifiedName, ModuleExtension module tagClassInfoBuilder.setTagName(entry.getKey()); entry.getValue().doc().ifPresent(tagClassInfoBuilder::setDocString); AttributeInfoExtractor.addDocumentableAttributes( - context, entry.getValue().attributes(), tagClassInfoBuilder::addAttribute); + context, + ImmutableMap.of(), + entry.getValue().attributes(), + tagClassInfoBuilder::addAttribute); moduleExtensionInfoBuilder.addTagClass(tagClassInfoBuilder); } moduleInfoBuilder.addModuleExtensionInfo(moduleExtensionInfoBuilder); @@ -460,15 +498,15 @@ protected void visitRepositoryRule( repositoryRuleInfoBuilder.setRuleName(qualifiedName); repositoryRuleFunction.getDocumentation().ifPresent(repositoryRuleInfoBuilder::setDocString); RuleClass ruleClass = repositoryRuleFunction.getRuleClass(); - repositoryRuleInfoBuilder - .setOriginKey( - OriginKey.newBuilder() - .setName(ruleClass.getName()) - .setFile( - context.labelRenderer().render(repositoryRuleFunction.getExtensionLabel()))) - .addAllAttribute(IMPLICIT_REPOSITORY_RULE_ATTRIBUTES); + repositoryRuleInfoBuilder.setOriginKey( + OriginKey.newBuilder() + .setName(ruleClass.getName()) + .setFile(context.labelRenderer().render(repositoryRuleFunction.getExtensionLabel()))); AttributeInfoExtractor.addDocumentableAttributes( - context, ruleClass.getAttributes(), repositoryRuleInfoBuilder::addAttribute); + context, + IMPLICIT_REPOSITORY_RULE_ATTRIBUTES, + ruleClass.getAttributes(), + repositoryRuleInfoBuilder::addAttribute); if (ruleClass.hasAttr("$environ", Types.STRING_LIST)) { repositoryRuleInfoBuilder.addAllEnviron( Types.STRING_LIST.cast(ruleClass.getAttributeByName("$environ").getDefaultValue(null))); diff --git a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractor.java b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractor.java index 6802fd8912d9ba..aa4c4b18a8d9ba 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractor.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractor.java @@ -14,18 +14,33 @@ package com.google.devtools.build.lib.starlarkdocextract; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier; import com.google.devtools.build.lib.packages.Type; +import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeInfo; +import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeType; import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.OriginKey; import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.RuleInfo; /** API documentation extractor for a rule. */ public final class RuleInfoExtractor { + @VisibleForTesting + public static final ImmutableMap IMPLICIT_RULE_ATTRIBUTES = + ImmutableMap.of( + "name", + AttributeInfo.newBuilder() + .setName("name") + .setType(AttributeType.NAME) + .setMandatory(true) + .setDocString("A unique name for this target.") + .build()); + /** * Extracts API documentation for a rule in the form of a {@link RuleInfo} proto. * @@ -72,10 +87,11 @@ public static RuleInfo buildRuleInfo( ruleInfoBuilder.setExecutable(true); } - ruleInfoBuilder.addAttribute( - AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first AttributeInfoExtractor.addDocumentableAttributes( - context, ruleClass.getAttributes(), ruleInfoBuilder::addAttribute); + context, + IMPLICIT_RULE_ATTRIBUTES, + ruleClass.getAttributes(), + ruleInfoBuilder::addAttribute); ImmutableSet advertisedProviders = ruleClass.getAdvertisedProviders().getStarlarkProviders(); if (!advertisedProviders.isEmpty()) { diff --git a/src/main/protobuf/stardoc_output.proto b/src/main/protobuf/stardoc_output.proto index 1827a2a97b28c1..06041e5f21058a 100644 --- a/src/main/protobuf/stardoc_output.proto +++ b/src/main/protobuf/stardoc_output.proto @@ -126,6 +126,9 @@ message MacroInfo { // The module where and the name under which the macro was originally // declared. OriginKey origin_key = 4; + + // True if this macro is a rule finalizer. + bool finalizer = 5; } // Representation of a Starlark rule, repository rule, or module extension tag @@ -161,6 +164,9 @@ message AttributeInfo { // If true, the attribute is non-configurable. bool nonconfigurable = 7; + + // If true, the attribute is defined in Bazel's native code, not in Starlark. + bool natively_defined = 8; } // Representation of a set of providers. 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 21bcee1b7c837d..d8e47730c128a2 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 @@ -31,6 +31,7 @@ java_library( "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util", "//src/test/java/com/google/devtools/build/lib/testutil", + "//third_party:guava", "//third_party:junit4", "//third_party:truth", "@com_google_protobuf//:protobuf_java", diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java index f4a7a2fb66b9dc..0aa15b02db14f7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java @@ -16,10 +16,11 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; -import static com.google.devtools.build.lib.starlarkdocextract.AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO; +import static com.google.devtools.build.lib.starlarkdocextract.RuleInfoExtractor.IMPLICIT_RULE_ATTRIBUTES; import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_ORDINARY; import static org.junit.Assert.assertThrows; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -355,20 +356,27 @@ def my_macro(): OriginKey.newBuilder().setName("my_rule").setFile("//:origin.bzl").build()); assertThat(moduleInfo.getRuleInfo(0).getAttributeList()) - .containsExactly( - IMPLICIT_NAME_ATTRIBUTE_INFO, - AttributeInfo.newBuilder() - .setName("a") - .setType(AttributeType.LABEL) - .setDefaultValue("None") - .addProviderNameGroup( - ProviderNameGroup.newBuilder() - .addProviderName("namespace.RenamedInfo") - .addProviderName("other_namespace.RenamedOtherInfo") - .addOriginKey( - OriginKey.newBuilder().setName("MyInfo").setFile("//:origin.bzl")) - .addOriginKey( - OriginKey.newBuilder().setName("MyOtherInfo").setFile("//:origin.bzl"))) + .isEqualTo( + ImmutableList.builder() + .addAll(IMPLICIT_RULE_ATTRIBUTES.values()) + .add( + AttributeInfo.newBuilder() + .setName("a") + .setType(AttributeType.LABEL) + .setDefaultValue("None") + .addProviderNameGroup( + ProviderNameGroup.newBuilder() + .addProviderName("namespace.RenamedInfo") + .addProviderName("other_namespace.RenamedOtherInfo") + .addOriginKey( + OriginKey.newBuilder() + .setName("MyInfo") + .setFile("//:origin.bzl")) + .addOriginKey( + OriginKey.newBuilder() + .setName("MyOtherInfo") + .setFile("//:origin.bzl"))) + .build()) .build()); assertThat(moduleInfo.getRuleInfo(0).getAdvertisedProviders()) .isEqualTo( @@ -955,7 +963,7 @@ def _impl(repository_ctx): .setOriginKey( OriginKey.newBuilder().setName("my_repo_rule").setFile("//:dep.bzl").build()) .setDocString("My repository rule\n\nWith details") - .addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES) + .addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES.values()) .addAttribute( AttributeInfo.newBuilder() .setName("a") diff --git a/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java b/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java index ad98bcd9b8b2ab..4dd2bfad9b6d48 100644 --- a/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java @@ -17,8 +17,8 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static com.google.devtools.build.lib.skyframe.BzlLoadValue.keyForBuild; -import static com.google.devtools.build.lib.starlarkdocextract.AttributeInfoExtractor.IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO; -import static com.google.devtools.build.lib.starlarkdocextract.AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO; +import static com.google.devtools.build.lib.starlarkdocextract.ModuleInfoExtractor.IMPLICIT_MACRO_ATTRIBUTES; +import static com.google.devtools.build.lib.starlarkdocextract.RuleInfoExtractor.IMPLICIT_RULE_ATTRIBUTES; import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_KWARGS; import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_ORDINARY; import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_VARARGS; @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.ProviderNameGroup; import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.RuleInfo; import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.StarlarkFunctionInfo; +import java.util.List; import java.util.Optional; import java.util.function.Predicate; import net.starlark.java.eval.Module; @@ -712,55 +713,68 @@ def _my_impl(ctx): """); ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getRuleInfoList().get(0).getAttributeList()) - .containsExactly( - IMPLICIT_NAME_ATTRIBUTE_INFO, - AttributeInfo.newBuilder() - .setName("a") - .setType(AttributeType.STRING) - .setDocString("My doc") - .setDefaultValue("\"foo\"") - .build(), - AttributeInfo.newBuilder() - .setName("b") - .setType(AttributeType.STRING) - .setMandatory(true) - .build(), - AttributeInfo.newBuilder() - .setName("c") - .setType(AttributeType.LABEL) - .setDefaultValue("None") - .addProviderNameGroup( - ProviderNameGroup.newBuilder() - .addProviderName("MyInfo1") - .addProviderName("MyInfo2") - .addOriginKey( - OriginKey.newBuilder().setName("MyInfo1").setFile(fakeLabelString)) - .addOriginKey( - OriginKey.newBuilder().setName("MyInfo2").setFile(fakeLabelString))) - .build(), - AttributeInfo.newBuilder() - .setName("d") - .setType(AttributeType.LABEL) - .setDefaultValue("None") - .addProviderNameGroup( - ProviderNameGroup.newBuilder() - .addProviderName("MyInfo1") - .addProviderName("MyInfo2") - .addOriginKey( - OriginKey.newBuilder().setName("MyInfo1").setFile(fakeLabelString)) - .addOriginKey( - OriginKey.newBuilder().setName("MyInfo2").setFile(fakeLabelString))) - .addProviderNameGroup( - ProviderNameGroup.newBuilder() - .addProviderName("MyInfo3") - .addOriginKey( - OriginKey.newBuilder().setName("MyInfo3").setFile(fakeLabelString))) - .build(), - AttributeInfo.newBuilder() - .setName("deprecated_license") - .setType(AttributeType.STRING_LIST) - .setDefaultValue("[\"none\"]") - .setNonconfigurable(true) + .containsExactlyElementsIn( + ImmutableList.builder() + .addAll(IMPLICIT_RULE_ATTRIBUTES.values()) + .add( + AttributeInfo.newBuilder() + .setName("a") + .setType(AttributeType.STRING) + .setDocString("My doc") + .setDefaultValue("\"foo\"") + .build(), + AttributeInfo.newBuilder() + .setName("b") + .setType(AttributeType.STRING) + .setMandatory(true) + .build(), + AttributeInfo.newBuilder() + .setName("c") + .setType(AttributeType.LABEL) + .setDefaultValue("None") + .addProviderNameGroup( + ProviderNameGroup.newBuilder() + .addProviderName("MyInfo1") + .addProviderName("MyInfo2") + .addOriginKey( + OriginKey.newBuilder() + .setName("MyInfo1") + .setFile(fakeLabelString)) + .addOriginKey( + OriginKey.newBuilder() + .setName("MyInfo2") + .setFile(fakeLabelString))) + .build(), + AttributeInfo.newBuilder() + .setName("d") + .setType(AttributeType.LABEL) + .setDefaultValue("None") + .addProviderNameGroup( + ProviderNameGroup.newBuilder() + .addProviderName("MyInfo1") + .addProviderName("MyInfo2") + .addOriginKey( + OriginKey.newBuilder() + .setName("MyInfo1") + .setFile(fakeLabelString)) + .addOriginKey( + OriginKey.newBuilder() + .setName("MyInfo2") + .setFile(fakeLabelString))) + .addProviderNameGroup( + ProviderNameGroup.newBuilder() + .addProviderName("MyInfo3") + .addOriginKey( + OriginKey.newBuilder() + .setName("MyInfo3") + .setFile(fakeLabelString))) + .build(), + AttributeInfo.newBuilder() + .setName("deprecated_license") + .setType(AttributeType.STRING_LIST) + .setDefaultValue("[\"none\"]") + .setNonconfigurable(true) + .build()) .build()); } @@ -817,69 +831,72 @@ def _my_impl(ctx): """); ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getRuleInfoList().get(0).getAttributeList()) - .containsExactly( - IMPLICIT_NAME_ATTRIBUTE_INFO, - AttributeInfo.newBuilder() - .setName("a") - .setType(AttributeType.INT) - .setDefaultValue("0") - .build(), - AttributeInfo.newBuilder() - .setName("b") - .setType(AttributeType.LABEL) - .setDefaultValue("None") - .build(), - AttributeInfo.newBuilder() - .setName("c") - .setType(AttributeType.STRING) - .setDefaultValue("\"\"") - .build(), - AttributeInfo.newBuilder() - .setName("d") - .setType(AttributeType.STRING_LIST) - .setDefaultValue("[]") - .build(), - AttributeInfo.newBuilder() - .setName("e") - .setType(AttributeType.INT_LIST) - .setDefaultValue("[]") - .build(), - AttributeInfo.newBuilder() - .setName("f") - .setType(AttributeType.LABEL_LIST) - .setDefaultValue("[]") - .build(), - AttributeInfo.newBuilder() - .setName("g") - .setType(AttributeType.BOOLEAN) - .setDefaultValue("False") - .build(), - AttributeInfo.newBuilder() - .setName("h") - .setType(AttributeType.LABEL_STRING_DICT) - .setDefaultValue("{}") - .build(), - AttributeInfo.newBuilder() - .setName("i") - .setType(AttributeType.STRING_DICT) - .setDefaultValue("{}") - .build(), - AttributeInfo.newBuilder() - .setName("j") - .setType(AttributeType.STRING_LIST_DICT) - .setDefaultValue("{}") - .build(), - AttributeInfo.newBuilder() - .setName("k") - .setType(AttributeType.OUTPUT) - .setDefaultValue("None") - .setNonconfigurable(true) - .build(), - AttributeInfo.newBuilder() - .setName("l") - .setType(AttributeType.OUTPUT_LIST) - .setDefaultValue("[]") - .setNonconfigurable(true) + .containsExactlyElementsIn( + ImmutableList.builder() + .addAll(IMPLICIT_RULE_ATTRIBUTES.values()) + .add( + AttributeInfo.newBuilder() + .setName("a") + .setType(AttributeType.INT) + .setDefaultValue("0") + .build(), + AttributeInfo.newBuilder() + .setName("b") + .setType(AttributeType.LABEL) + .setDefaultValue("None") + .build(), + AttributeInfo.newBuilder() + .setName("c") + .setType(AttributeType.STRING) + .setDefaultValue("\"\"") + .build(), + AttributeInfo.newBuilder() + .setName("d") + .setType(AttributeType.STRING_LIST) + .setDefaultValue("[]") + .build(), + AttributeInfo.newBuilder() + .setName("e") + .setType(AttributeType.INT_LIST) + .setDefaultValue("[]") + .build(), + AttributeInfo.newBuilder() + .setName("f") + .setType(AttributeType.LABEL_LIST) + .setDefaultValue("[]") + .build(), + AttributeInfo.newBuilder() + .setName("g") + .setType(AttributeType.BOOLEAN) + .setDefaultValue("False") + .build(), + AttributeInfo.newBuilder() + .setName("h") + .setType(AttributeType.LABEL_STRING_DICT) + .setDefaultValue("{}") + .build(), + AttributeInfo.newBuilder() + .setName("i") + .setType(AttributeType.STRING_DICT) + .setDefaultValue("{}") + .build(), + AttributeInfo.newBuilder() + .setName("j") + .setType(AttributeType.STRING_LIST_DICT) + .setDefaultValue("{}") + .build(), + AttributeInfo.newBuilder() + .setName("k") + .setType(AttributeType.OUTPUT) + .setDefaultValue("None") + .setNonconfigurable(true) + .build(), + AttributeInfo.newBuilder() + .setName("l") + .setType(AttributeType.OUTPUT_LIST) + .setDefaultValue("[]") + .setNonconfigurable(true) + .build()) .build()); } @@ -926,13 +943,40 @@ def _my_impl(name, visibility): .setDocString("My doc") .setOriginKey( OriginKey.newBuilder().setName("documented_macro").setFile(fakeLabelString)) - .addAttribute(IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO) + .addAllAttribute(IMPLICIT_MACRO_ATTRIBUTES.values()) .build(), MacroInfo.newBuilder() .setMacroName("undocumented_macro") .setOriginKey( OriginKey.newBuilder().setName("undocumented_macro").setFile(fakeLabelString)) - .addAttribute(IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO) + .addAllAttribute(IMPLICIT_MACRO_ATTRIBUTES.values()) + .build()); + } + + @Test + public void macroFinalizer() throws Exception { + Module module = + exec( + """ + def _my_impl(name, visibility): + pass + + my_finalizer = macro( + doc = "My finalizer", + implementation = _my_impl, + finalizer = True, + ) + """); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); + assertThat(moduleInfo.getMacroInfoList()) + .containsExactly( + MacroInfo.newBuilder() + .setMacroName("my_finalizer") + .setDocString("My finalizer") + .setOriginKey( + OriginKey.newBuilder().setName("my_finalizer").setFile(fakeLabelString)) + .addAllAttribute(IMPLICIT_MACRO_ATTRIBUTES.values()) + .setFinalizer(true) .build()); } @@ -955,21 +999,22 @@ def _my_impl(name): """); ModuleInfo moduleInfo = getExtractor().extractFrom(module); assertThat(moduleInfo.getMacroInfoList().get(0).getAttributeList()) - .containsExactly( - IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO, // name comes first - AttributeInfo.newBuilder() - .setName("some_attr") - .setType(AttributeType.LABEL) - .setMandatory(true) - .build(), - AttributeInfo.newBuilder() - .setName("another_attr") - .setType(AttributeType.INT) - .setDocString("An integer") - .setDefaultValue("42") - .build() - // note that implicit attributes don't get documented - ); + .containsExactlyElementsIn( + ImmutableList.builder() + .addAll(IMPLICIT_MACRO_ATTRIBUTES.values()) + .add( + AttributeInfo.newBuilder() + .setName("some_attr") + .setType(AttributeType.LABEL) + .setMandatory(true) + .build(), + AttributeInfo.newBuilder() + .setName("another_attr") + .setType(AttributeType.INT) + .setDocString("An integer") + .setDefaultValue("42") + .build()) + .build()); } @Test @@ -996,20 +1041,29 @@ def _my_macro_impl(name, visibility, srcs, **kwargs): ) """); ModuleInfo moduleInfo = getExtractor().extractFrom(module); - assertThat(moduleInfo.getMacroInfoList().get(0).getAttributeList()) - .containsExactly( - IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO, // name comes first - // TODO(arostovtsev): for macros, we ought to also document the visibility attr + List attributes = moduleInfo.getMacroInfoList().get(0).getAttributeList(); + assertThat(attributes.get(0)).isEqualTo(IMPLICIT_MACRO_ATTRIBUTES.get("name")); + assertThat(attributes.get(1)).isEqualTo(IMPLICIT_MACRO_ATTRIBUTES.get("visibility")); + // Starlark-defined inherited attribute + assertThat(attributes) + .contains( AttributeInfo.newBuilder() .setName("srcs") .setType(AttributeType.LABEL_LIST) .setDocString("My rule sources") .setDefaultValue("None") // Default value of inherited attributes is always None - .build() - // TODO(arostovtsev): currently, non-Starlark-defined attributes don't get documented. - // This is a reasonable behavior for rules, but we probably ought to document them in - // macros with inherited attributes. - ); + .build()); + // Native inherited attributes may not be documented, so ignore doc string for them. + assertThat(attributes) + .ignoringFields(AttributeInfo.DOC_STRING_FIELD_NUMBER) + .contains( + AttributeInfo.newBuilder() + .setName("tags") + .setType(AttributeType.STRING_LIST) + .setDefaultValue("None") // Default value of inherited attributes is always None + .setNonconfigurable(true) + .setNativelyDefined(true) + .build()); } @Test @@ -1098,7 +1152,7 @@ def _my_impl(ctx): ModuleInfo moduleInfo = getExtractor(repositoryMapping, "my_repo").extractFrom(module); assertThat( moduleInfo.getRuleInfoList().get(0).getAttributeList().stream() - .filter(attr -> !attr.equals(IMPLICIT_NAME_ATTRIBUTE_INFO)) + .filter(attr -> !IMPLICIT_RULE_ATTRIBUTES.containsKey(attr.getName())) .map(AttributeInfo::getDefaultValue)) .containsExactly( "\"@my_repo//test:foo\"", @@ -1160,7 +1214,6 @@ def _my_impl(target, ctx): .setOriginKey(OriginKey.newBuilder().setName("my_aspect").setFile(fakeLabelString)) .addAspectAttribute("deps") .addAspectAttribute("srcs") - .addAttribute(IMPLICIT_NAME_ATTRIBUTE_INFO) .addAttribute( AttributeInfo.newBuilder() .setName("a") diff --git a/src/test/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractorTest.java b/src/test/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractorTest.java index 63f503791714e4..ff5ab630e0ded7 100644 --- a/src/test/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractorTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractorTest.java @@ -52,7 +52,7 @@ public void basicFunctionality() throws Exception { ExtractorContext extractorContext = ExtractorContext.builder() .labelRenderer(LabelRenderer.DEFAULT) - .extractNonStarlarkAttrs(true) + .extractNativelyDefinedAttrs(true) .build(); RuleInfo ruleInfo = RuleInfoExtractor.buildRuleInfo(extractorContext, "namespace.test_rule", ruleClass); @@ -61,13 +61,15 @@ public void basicFunctionality() throws Exception { RuleInfo.newBuilder() .setRuleName("namespace.test_rule") .setOriginKey(OriginKey.newBuilder().setName("test_rule").setFile("")) - .addAttribute(AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO) + .addAllAttribute(RuleInfoExtractor.IMPLICIT_RULE_ATTRIBUTES.values()) .addAttribute( AttributeInfo.newBuilder() .setName("tags") .setType(AttributeType.STRING_LIST) .setDefaultValue("[]") - .setMandatory(false)) + .setMandatory(false) + .setNativelyDefined(true) + .build()) .build()); } @@ -87,7 +89,7 @@ public void allNativeRulesAreSupported() throws Exception { ExtractorContext extractorContext = ExtractorContext.builder() .labelRenderer(LabelRenderer.DEFAULT) - .extractNonStarlarkAttrs(true) + .extractNativelyDefinedAttrs(true) .build(); for (RuleClass ruleClass : ruleClassProvider.getRuleClassMap().values()) { RuleInfo ruleInfo = @@ -99,7 +101,7 @@ public void allNativeRulesAreSupported() throws Exception { .isEqualTo(""); assertWithMessage("rule '%s'", ruleClass.getName()) .that(ruleInfo.getAttributeList().getFirst()) - .isEqualTo(AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); + .isEqualTo(RuleInfoExtractor.IMPLICIT_RULE_ATTRIBUTES.get("name")); assertWithMessage("rule '%s'", ruleClass.getName()) .that(ruleInfo.getAttributeList().stream().map(AttributeInfo::getName)) .containsNoDuplicates(); diff --git a/tools/ctexplain/analyses/summary.py b/tools/ctexplain/analyses/summary.py index 6669e6c5216445..28c98de915f1d8 100644 --- a/tools/ctexplain/analyses/summary.py +++ b/tools/ctexplain/analyses/summary.py @@ -18,7 +18,7 @@ from dataclasses import dataclass from tools.ctexplain.types import ConfiguredTarget -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.util as util +import tools.ctexplain.util as util @dataclass(frozen=True) diff --git a/tools/ctexplain/analyses/summary_test.py b/tools/ctexplain/analyses/summary_test.py index 399b1fd1eec938..ce78442fbe9d02 100644 --- a/tools/ctexplain/analyses/summary_test.py +++ b/tools/ctexplain/analyses/summary_test.py @@ -17,7 +17,7 @@ # Do not edit this line. Copybara replaces it with PY2 migration helper. from frozendict import frozendict -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.analyses.summary as summary +import tools.ctexplain.analyses.summary as summary from tools.ctexplain.types import Configuration from tools.ctexplain.types import ConfiguredTarget from tools.ctexplain.types import NullConfiguration diff --git a/tools/ctexplain/ctexplain.py b/tools/ctexplain/ctexplain.py index 65814799a35cb1..ea2d2433d04ea8 100644 --- a/tools/ctexplain/ctexplain.py +++ b/tools/ctexplain/ctexplain.py @@ -42,11 +42,11 @@ from absl import flags from dataclasses import dataclass -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.analyses.summary as summary +import tools.ctexplain.analyses.summary as summary from tools.ctexplain.bazel_api import BazelApi -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.lib as lib +import tools.ctexplain.lib as lib from tools.ctexplain.types import ConfiguredTarget -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.util as util +import tools.ctexplain.util as util FLAGS = flags.FLAGS diff --git a/tools/ctexplain/lib.py b/tools/ctexplain/lib.py index 172509d1c75f35..9025a6e00231bb 100644 --- a/tools/ctexplain/lib.py +++ b/tools/ctexplain/lib.py @@ -14,7 +14,7 @@ """General-purpose business logic.""" from typing import Tuple -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.bazel_api as bazel_api +import tools.ctexplain.bazel_api as bazel_api from tools.ctexplain.types import ConfiguredTarget diff --git a/tools/ctexplain/lib_test.py b/tools/ctexplain/lib_test.py index e48f8e36874a40..b8edff1e1c897e 100644 --- a/tools/ctexplain/lib_test.py +++ b/tools/ctexplain/lib_test.py @@ -14,8 +14,8 @@ """Tests for lib.py.""" import unittest from src.test.py.bazel import test_base -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.bazel_api as bazel_api -# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.lib as lib +import tools.ctexplain.bazel_api as bazel_api +import tools.ctexplain.lib as lib from tools.ctexplain.types import Configuration from tools.ctexplain.types import HostConfiguration from tools.ctexplain.types import NullConfiguration