Emit consistent names for providers in starlark_doc_extract If a module re-exports a provider in a namespace or under a different name, and that provider is also referred to by a rule or rule attribute, we should use the exported provider name consistently throughout our doc output (note that we also output the OriginKey message with the name under which the provider was exported in its original module). This requires processing a module's docs in two phases: first, collect all documentable providers' names and keys; and then process all documentable symbol documentation as before, using the providers' exported names when emitting rule docs. In turn, that means refactoring ModuleInfoExtractor to use a common abstraction (GlobalsVisitor) for recursively walking namespaced Starlark values in both phases. Also remove the rewriteWorkspace call in originKeyFileAndModuleInfoFileLabels which was accidentally added in the previous change. PiperOrigin-RevId: 538939476 Change-Id: I33565966019673b58caa134e676900ba16d09412
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkProviderIdentifier.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkProviderIdentifier.java index 8827225..3b1818a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkProviderIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkProviderIdentifier.java
@@ -88,6 +88,12 @@ } } + /** + * Returns the provider key name for a declared provider, or the legacy ID for a legacy provider. + * + * <p>Used for rendering human-readable descriptions, such as for a rule attribute's set of + * required providers. + */ @Override public String toString() { if (isLegacy()) {
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 2ebf275..6b0226a 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
@@ -41,6 +41,7 @@ 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.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -55,7 +56,7 @@ /** API documentation extractor for a compiled, loaded Starlark module. */ final class ModuleInfoExtractor { - private final Predicate<String> isWantedName; + private final Predicate<String> isWantedGlobal; private final RepositoryMapping repositoryMapping; @VisibleForTesting @@ -73,34 +74,44 @@ /** * Constructs an instance of {@code ModuleInfoExtractor}. * - * @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 + * @param isWantedGlobal a filter applied to the module's globals; only those symbols which both + * are loadable (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 ModuleInfoExtractor(Predicate<String> isWantedName, RepositoryMapping repositoryMapping) { - this.isWantedName = isWantedName; + public ModuleInfoExtractor( + Predicate<String> isWantedGlobal, RepositoryMapping repositoryMapping) { + this.isWantedGlobal = isWantedGlobal; this.repositoryMapping = repositoryMapping; } - /** Extracts structured documentation for the exported symbols of a given module. */ + /** Extracts structured documentation for the loadable symbols of a given module. */ public ModuleInfo extractFrom(Module module) throws ExtractionException { ModuleInfo.Builder builder = ModuleInfo.newBuilder(); Optional.ofNullable(module.getDocumentation()).ifPresent(builder::setModuleDocstring); Optional.ofNullable(BazelModuleContext.of(module)) .map(bazelModuleContext -> bazelModuleContext.label().getDisplayForm(repositoryMapping)) .ifPresent(builder::setFile); - for (var entry : module.getGlobals().entrySet()) { - String topLevelSymbol = entry.getKey(); - if (isExportableName(topLevelSymbol) && isWantedName.test(topLevelSymbol)) { - addInfo(builder, topLevelSymbol, entry.getValue()); - } - } + + // We do two traversals over the module's globals: (1) find qualified names (including any + // nesting structs) for providers loadable from this module; (2) build the documentation + // proto, using the information from traversal 1 for provider names references by rules and + // attributes. + ProviderQualifiedNameCollector providerQualifiedNameCollector = + new ProviderQualifiedNameCollector(); + providerQualifiedNameCollector.traverse(module); + DocumentationExtractor documentationExtractor = + new DocumentationExtractor( + builder, + isWantedGlobal, + repositoryMapping, + providerQualifiedNameCollector.buildQualifiedNames()); + documentationExtractor.traverse(module); return builder.build(); } - private static boolean isExportableName(String name) { + private static boolean isPublicName(String name) { return name.length() > 0 && Character.isAlphabetic(name.charAt(0)); } @@ -120,252 +131,386 @@ } /** - * @param builder proto builder to which to append documentation - * @param name the name under which the value is exported by the module; for example, "foo.bar" - * for field bar of exported struct foo - * @param value documentable Starlark value + * A stateful visitor which traverses a Starlark module's documentable globals, recursing into + * structs. */ - private void addInfo(ModuleInfo.Builder builder, String name, Object value) - throws ExtractionException { - if (value instanceof StarlarkRuleFunction) { - addRuleInfo(builder, name, (StarlarkRuleFunction) value); - } else if (value instanceof StarlarkProvider) { - addProviderInfo(builder, name, (StarlarkProvider) value); - } else if (value instanceof StarlarkFunction) { - try { - builder.addFuncInfo( - FunctionUtil.fromNameAndFunction( - name, (StarlarkFunction) value, /* withOriginKey= */ true, repositoryMapping)); - } catch (DocstringParseException e) { - throw new ExtractionException(e); - } - } else if (value instanceof StarlarkDefinedAspect) { - addAspectInfo(builder, name, (StarlarkDefinedAspect) value); - } else if (value instanceof Structure) { - addStructureInfo(builder, name, (Structure) value); - } - // else the value is a constant (string, list etc.), and we currently don't have a convention - // for associating a doc string with one - so we don't emit documentation for it. - // TODO(b/276733504): should we recurse into dicts to search for documentable values? - } - - private void addStructureInfo(ModuleInfo.Builder builder, String name, Structure structure) - throws ExtractionException { - for (String fieldName : structure.getFieldNames()) { - if (isExportableName(fieldName)) { - try { - Object fieldValue = structure.getValue(fieldName); - if (fieldValue != null) { - addInfo(builder, String.format("%s.%s", name, fieldName), fieldValue); - } - } catch (EvalException e) { - throw new ExtractionException( - String.format("in struct %s field %s: failed to read value", name, fieldName), e); + private abstract static class GlobalsVisitor { + public void traverse(Module module) throws ExtractionException { + for (var entry : module.getGlobals().entrySet()) { + String globalSymbol = entry.getKey(); + if (shouldVisitGlobal(globalSymbol)) { + visit(globalSymbol, entry.getValue()); } } } - } - private static AttributeType getAttributeType(Attribute attribute, String where) - throws ExtractionException { - Type<?> type = attribute.getType(); - if (type.equals(Type.INTEGER)) { - return AttributeType.INT; - } else if (type.equals(BuildType.LABEL)) { - return AttributeType.LABEL; - } else if (type.equals(Type.STRING)) { - if (attribute.getPublicName().equals("name")) { - return AttributeType.NAME; - } else { - return AttributeType.STRING; + /** Returns whether the visitor should visit (and possibly recurse into) the given global. */ + protected abstract boolean shouldVisitGlobal(String globalSymbol); + + /** + * @param qualifiedName the name under which the value may be accessed by a user of the module; + * for example, "foo.bar" for field bar of global struct foo + * @param value the Starlark value + */ + private void visit(String qualifiedName, Object value) throws ExtractionException { + if (value instanceof StarlarkRuleFunction) { + visitRule(qualifiedName, (StarlarkRuleFunction) value); + } else if (value instanceof StarlarkProvider) { + visitProvider(qualifiedName, (StarlarkProvider) value); + } else if (value instanceof StarlarkFunction) { + visitFunction(qualifiedName, (StarlarkFunction) value); + } else if (value instanceof StarlarkDefinedAspect) { + visitAspect(qualifiedName, (StarlarkDefinedAspect) value); + } else if (value instanceof Structure) { + visitStructure(qualifiedName, (Structure) value); } - } else if (type.equals(Type.STRING_LIST)) { - return AttributeType.STRING_LIST; - } else if (type.equals(Type.INTEGER_LIST)) { - return AttributeType.INT_LIST; - } else if (type.equals(BuildType.LABEL_LIST)) { - return AttributeType.LABEL_LIST; - } else if (type.equals(Type.BOOLEAN)) { - return AttributeType.BOOLEAN; - } else if (type.equals(BuildType.LABEL_KEYED_STRING_DICT)) { - return AttributeType.LABEL_STRING_DICT; - } else if (type.equals(Type.STRING_DICT)) { - return AttributeType.STRING_DICT; - } else if (type.equals(Type.STRING_LIST_DICT)) { - return AttributeType.STRING_LIST_DICT; - } else if (type.equals(BuildType.OUTPUT)) { - return AttributeType.OUTPUT; - } else if (type.equals(BuildType.OUTPUT_LIST)) { - return AttributeType.OUTPUT_LIST; + // else the value is a constant (string, list etc.), and we currently don't have a convention + // for associating a doc string with one - so we don't emit documentation for it. + // TODO(b/276733504): should we recurse into dicts to search for documentable values? Note + // that dicts (unlike structs!) can have reference cycles, so we would need to track the set + // of traversed entities. } - throw new ExtractionException( - String.format( - "in %s attribute %s: unsupported type %s", - where, attribute.getPublicName(), type.getClass().getSimpleName())); + protected void visitRule(String qualifiedName, StarlarkRuleFunction value) + throws ExtractionException {} + + protected void visitProvider(String qualifiedName, StarlarkProvider value) {} + + protected void visitFunction(String qualifiedName, StarlarkFunction value) + throws ExtractionException {} + + protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect) + throws ExtractionException {} + + private void visitStructure(String qualifiedName, Structure structure) + throws ExtractionException { + for (String fieldName : structure.getFieldNames()) { + if (isPublicName(fieldName)) { + try { + Object fieldValue = structure.getValue(fieldName); + if (fieldValue != null) { + visit(String.format("%s.%s", qualifiedName, fieldName), fieldValue); + } + } catch (EvalException e) { + throw new ExtractionException( + String.format( + "in struct %s field %s: failed to read value", qualifiedName, fieldName), + e); + } + } + } + } } /** - * 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. + * A {@link GlobalsVisitor} which finds the qualified names (including any nesting structs) for + * providers loadable from this module. */ - 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 static final class ProviderQualifiedNameCollector extends GlobalsVisitor { + private final LinkedHashMap<StarlarkProvider.Key, String> qualifiedNames = + new LinkedHashMap<>(); + + /** + * Builds a map from the keys of the Starlark providers which were walked via {@link #traverse} + * to the qualified names (including any structs) under which those providers may be accessed by + * a user of this module. + * + * <p>If the same provider is accessible under multiple names, the first documentable name wins. + */ + public ImmutableMap<StarlarkProvider.Key, String> buildQualifiedNames() { + return ImmutableMap.copyOf(qualifiedNames); + } + + /** + * Returns true if the symbol is a loadable name (starts with an alphabetic character, not '_'). + * + * <p>{@link ProviderQualifiedNameCollector} traverses all loadable providers, not filtering by + * ModuleInfoExtractor#isWantedName, because a non-wanted provider symbol may still be referred + * to by a wanted rule; we do not want the provider names emitted in rule documentation to vary + * when we change the isWantedName filter. + */ + @Override + protected boolean shouldVisitGlobal(String globalSymbol) { + return isPublicName(globalSymbol); + } + + @Override + protected void visitProvider(String qualifiedName, StarlarkProvider value) { + qualifiedNames.putIfAbsent(value.getKey(), qualifiedName); } } - private Object stringifyLabelsOfMap(Map<?, ?> dict) { - boolean neededToStringify = false; - ImmutableMap.Builder<Object, Object> 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; + /** A {@link GlobalsVisitor} which extracts documentation for symbols in this module. */ + private static final class DocumentationExtractor extends GlobalsVisitor { + private final ModuleInfo.Builder moduleInfoBuilder; + private final Predicate<String> isWantedGlobal; + private final RepositoryMapping repositoryMapping; + private final ImmutableMap<StarlarkProvider.Key, String> providerQualifiedNames; + + /** + * @param moduleInfoBuilder builder to which {@link #traverse} adds extracted documentation + * @param isWantedGlobal a filter applied to global symbols; only those symbols which both are + * loadable (meaning the first character is alphabetic) and for which the filter returns + * true will be documented + * @param repositoryMapping repo mapping to use for stringifying labels + * @param providerQualifiedNames a map from the keys of documentable Starlark providers loadable + * from this module to the qualified names (including structure namespaces) under which + * those providers are accessible to a user of this module + */ + DocumentationExtractor( + ModuleInfo.Builder moduleInfoBuilder, + Predicate<String> isWantedGlobal, + RepositoryMapping repositoryMapping, + ImmutableMap<StarlarkProvider.Key, String> providerQualifiedNames) { + this.moduleInfoBuilder = moduleInfoBuilder; + this.isWantedGlobal = isWantedGlobal; + this.repositoryMapping = repositoryMapping; + this.providerQualifiedNames = providerQualifiedNames; + } + + @Override + protected boolean shouldVisitGlobal(String globalSymbol) { + return isPublicName(globalSymbol) && isWantedGlobal.test(globalSymbol); + } + + @Override + protected void visitFunction(String qualifiedName, StarlarkFunction function) + throws ExtractionException { + try { + moduleInfoBuilder.addFuncInfo( + FunctionUtil.fromNameAndFunction( + qualifiedName, function, /* withOriginKey= */ true, repositoryMapping)); + } catch (DocstringParseException e) { + throw new ExtractionException(e); } - builder.put(keyWithStringifiedLabels, valueWithStringifiedLabels); } - return neededToStringify ? Dict.immutableCopyOf(builder.buildOrThrow()) : dict; - } - private Object stringifyLabelsOfList(List<?> list) { - boolean neededToStringify = false; - ImmutableList.Builder<Object> builder = ImmutableList.builder(); - for (Object element : list) { - Object elementWithStringifiedLabels = stringifyLabels(element); - if (elementWithStringifiedLabels != element /* as Objects */) { - neededToStringify = true; + @Override + protected void visitRule(String qualifiedName, StarlarkRuleFunction ruleFunction) + throws ExtractionException { + RuleInfo.Builder ruleInfoBuilder = RuleInfo.newBuilder(); + // Record the name under which this symbol is made accessible, which may differ from the + // symbol's exported name + ruleInfoBuilder.setRuleName(qualifiedName); + // ... but record the origin rule key for cross references. + ruleInfoBuilder.setOriginKey( + OriginKey.newBuilder() + .setName(ruleFunction.getName()) + .setFile(ruleFunction.getExtensionLabel().getDisplayForm(repositoryMapping))); + ruleFunction.getDocumentation().ifPresent(ruleInfoBuilder::setDocString); + RuleClass ruleClass = ruleFunction.getRuleClass(); + ruleInfoBuilder.addAttribute(IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first + for (Attribute attribute : ruleClass.getAttributes()) { + if (attribute.starlarkDefined() + && attribute.isDocumented() + && isPublicName(attribute.getPublicName())) { + ruleInfoBuilder.addAttribute(buildAttributeInfo(attribute, "rule " + qualifiedName)); + } } - 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()); - Optional.ofNullable(attribute.getDoc()).ifPresent(builder::setDocString); - builder.setType(getAttributeType(attribute, where)); - builder.setMandatory(attribute.isMandatory()); - for (ImmutableSet<StarlarkProviderIdentifier> providerGroup : - attribute.getRequiredProviders().getStarlarkProviders()) { - builder.addProviderNameGroup(buildProviderNameGroup(providerGroup)); + ImmutableSet<StarlarkProviderIdentifier> advertisedProviders = + ruleClass.getAdvertisedProviders().getStarlarkProviders(); + if (!advertisedProviders.isEmpty()) { + ruleInfoBuilder.setAdvertisedProviders(buildProviderNameGroup(advertisedProviders)); + } + moduleInfoBuilder.addRuleInfo(ruleInfoBuilder); } - if (!attribute.isMandatory()) { - Object defaultValue = Attribute.valueToStarlark(attribute.getDefaultValueUnchecked()); - builder.setDefaultValue(new Printer().repr(stringifyLabels(defaultValue)).toString()); + @Override + protected void visitProvider(String qualifiedName, StarlarkProvider provider) { + ProviderInfo.Builder providerInfoBuilder = ProviderInfo.newBuilder(); + // Record the name under which this symbol is made accessible, which may differ from the + // symbol's exported name. + // Note that it's possible that qualifiedName != getDocumentedProviderName() if the same + // provider symbol is made accessible under more than one qualified name. + // TODO(b/276733504): if a provider (or any other documentable entity) is made accessible + // under two different qualified names, record them in a repeated field inside a single *Info + // object, instead of producing a separate *Info object for each alias. + providerInfoBuilder.setProviderName(qualifiedName); + // Record the origin provider key for cross references. + providerInfoBuilder.setOriginKey( + OriginKey.newBuilder() + .setName(provider.getName()) + .setFile(provider.getKey().getExtensionLabel().getDisplayForm(repositoryMapping))); + provider.getDocumentation().ifPresent(providerInfoBuilder::setDocString); + ImmutableMap<String, Optional<String>> schema = provider.getSchema(); + if (schema != null) { + for (Map.Entry<String, Optional<String>> entry : schema.entrySet()) { + if (isPublicName(entry.getKey())) { + ProviderFieldInfo.Builder fieldInfoBuilder = ProviderFieldInfo.newBuilder(); + fieldInfoBuilder.setName(entry.getKey()); + entry.getValue().ifPresent(fieldInfoBuilder::setDocString); + providerInfoBuilder.addFieldInfo(fieldInfoBuilder.build()); + } + } + } + moduleInfoBuilder.addProviderInfo(providerInfoBuilder); } - return builder.build(); - } - private ProviderNameGroup buildProviderNameGroup( - ImmutableSet<StarlarkProviderIdentifier> providerGroup) { - ProviderNameGroup.Builder providerNameGroupBuilder = ProviderNameGroup.newBuilder(); - for (StarlarkProviderIdentifier provider : providerGroup) { - // TODO(b/276733504): if this module exports a provider under a different name or in a - // namespace, document it under that exported name rather than the provider's key name. - providerNameGroupBuilder.addProviderName(provider.toString()); - OriginKey.Builder providerKeyBuilder = OriginKey.newBuilder().setName(provider.toString()); + @Override + protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect) + throws ExtractionException { + AspectInfo.Builder aspectInfoBuilder = AspectInfo.newBuilder(); + // Record the name under which this symbol is made accessible, which may differ from the + // symbol's exported name + aspectInfoBuilder.setAspectName(qualifiedName); + // ... but record the origin aspect key for cross references. + aspectInfoBuilder.setOriginKey( + OriginKey.newBuilder() + .setName(aspect.getAspectClass().getExportedName()) + .setFile( + aspect.getAspectClass().getExtensionLabel().getDisplayForm(repositoryMapping))); + aspect.getDocumentation().ifPresent(aspectInfoBuilder::setDocString); + aspectInfoBuilder.addAllAspectAttribute(aspect.getAttributeAspects()); + aspectInfoBuilder.addAttribute(IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first + for (Attribute attribute : aspect.getAttributes()) { + if (isPublicName(attribute.getPublicName())) { + aspectInfoBuilder.addAttribute(buildAttributeInfo(attribute, "aspect " + qualifiedName)); + } + } + moduleInfoBuilder.addAspectInfo(aspectInfoBuilder); + } + + /** + * 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<Object, Object> 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<Object> 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 static AttributeType getAttributeType(Attribute attribute, String where) + throws ExtractionException { + Type<?> type = attribute.getType(); + if (type.equals(Type.INTEGER)) { + return AttributeType.INT; + } else if (type.equals(BuildType.LABEL)) { + return AttributeType.LABEL; + } else if (type.equals(Type.STRING)) { + if (attribute.getPublicName().equals("name")) { + return AttributeType.NAME; + } else { + return AttributeType.STRING; + } + } else if (type.equals(Type.STRING_LIST)) { + return AttributeType.STRING_LIST; + } else if (type.equals(Type.INTEGER_LIST)) { + return AttributeType.INT_LIST; + } else if (type.equals(BuildType.LABEL_LIST)) { + return AttributeType.LABEL_LIST; + } else if (type.equals(Type.BOOLEAN)) { + return AttributeType.BOOLEAN; + } else if (type.equals(BuildType.LABEL_KEYED_STRING_DICT)) { + return AttributeType.LABEL_STRING_DICT; + } else if (type.equals(Type.STRING_DICT)) { + return AttributeType.STRING_DICT; + } else if (type.equals(Type.STRING_LIST_DICT)) { + return AttributeType.STRING_LIST_DICT; + } else if (type.equals(BuildType.OUTPUT)) { + return AttributeType.OUTPUT; + } else if (type.equals(BuildType.OUTPUT_LIST)) { + return AttributeType.OUTPUT_LIST; + } + + throw new ExtractionException( + String.format( + "in %s attribute %s: unsupported type %s", + where, attribute.getPublicName(), type.getClass().getSimpleName())); + } + + private AttributeInfo buildAttributeInfo(Attribute attribute, String where) + throws ExtractionException { + AttributeInfo.Builder builder = AttributeInfo.newBuilder(); + builder.setName(attribute.getPublicName()); + Optional.ofNullable(attribute.getDoc()).ifPresent(builder::setDocString); + builder.setType(getAttributeType(attribute, where)); + builder.setMandatory(attribute.isMandatory()); + for (ImmutableSet<StarlarkProviderIdentifier> providerGroup : + attribute.getRequiredProviders().getStarlarkProviders()) { + builder.addProviderNameGroup(buildProviderNameGroup(providerGroup)); + } + + if (!attribute.isMandatory()) { + Object defaultValue = Attribute.valueToStarlark(attribute.getDefaultValueUnchecked()); + builder.setDefaultValue(new Printer().repr(stringifyLabels(defaultValue)).toString()); + } + return builder.build(); + } + + /** + * Returns the provider name suitable for use in this module's documentation. For a provider + * loadable from this module, this is the qualified name (or more precisely, the first qualified + * name) under which a user of this module may access it. For local providers and for providers + * loaded but not re-exported via a global, it's the provider key name (a.k.a. {@code + * provider.toString()}). For legacy struct providers, it's the legacy ID (which also happens to + * be {@code provider.toString()}). + */ + private String getDocumentedProviderName(StarlarkProviderIdentifier provider) { if (!provider.isLegacy()) { - if (provider.getKey() instanceof StarlarkProvider.Key) { - Label definingModule = ((StarlarkProvider.Key) provider.getKey()).getExtensionLabel(); - providerKeyBuilder.setFile(definingModule.getDisplayForm(repositoryMapping)); - } else if (provider.getKey() instanceof BuiltinProvider.Key) { - providerKeyBuilder.setFile("<native>"); + String qualifiedName = providerQualifiedNames.get(provider.getKey()); + if (qualifiedName != null) { + return qualifiedName; } } - providerNameGroupBuilder.addOriginKey(providerKeyBuilder.build()); + return provider.toString(); } - return providerNameGroupBuilder.build(); - } - private void addRuleInfo( - ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkRuleFunction ruleFunction) - throws ExtractionException { - RuleInfo.Builder ruleInfoBuilder = RuleInfo.newBuilder(); - // Allow rules to be exported under a different name (e.g. in a struct) - ruleInfoBuilder.setRuleName(exportedName); - // ... but record the origin rule key for cross references. - ruleInfoBuilder.setOriginKey( - OriginKey.newBuilder() - .setName(ruleFunction.getName()) - .setFile(ruleFunction.getExtensionLabel().getDisplayForm(repositoryMapping))); - ruleFunction.getDocumentation().ifPresent(ruleInfoBuilder::setDocString); - RuleClass ruleClass = ruleFunction.getRuleClass(); - ruleInfoBuilder.addAttribute(IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first - for (Attribute attribute : ruleClass.getAttributes()) { - if (attribute.starlarkDefined() - && attribute.isDocumented() - && isExportableName(attribute.getPublicName())) { - ruleInfoBuilder.addAttribute(buildAttributeInfo(attribute, "rule " + exportedName)); - } - } - ImmutableSet<StarlarkProviderIdentifier> advertisedProviders = - ruleClass.getAdvertisedProviders().getStarlarkProviders(); - if (!advertisedProviders.isEmpty()) { - ruleInfoBuilder.setAdvertisedProviders(buildProviderNameGroup(advertisedProviders)); - } - moduleInfoBuilder.addRuleInfo(ruleInfoBuilder); - } - - private void addProviderInfo( - ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkProvider provider) { - ProviderInfo.Builder providerInfoBuilder = ProviderInfo.newBuilder(); - // Allow providers to be exported under a different name (e.g. in a struct) - providerInfoBuilder.setProviderName(exportedName); - // ... but record the origin provider key for cross references. - providerInfoBuilder.setOriginKey( - OriginKey.newBuilder() - .setName(provider.getName()) - .setFile(provider.getKey().getExtensionLabel().getDisplayForm(repositoryMapping))); - provider.getDocumentation().ifPresent(providerInfoBuilder::setDocString); - ImmutableMap<String, Optional<String>> schema = provider.getSchema(); - if (schema != null) { - for (Map.Entry<String, Optional<String>> entry : schema.entrySet()) { - if (isExportableName(entry.getKey())) { - ProviderFieldInfo.Builder fieldInfoBuilder = ProviderFieldInfo.newBuilder(); - fieldInfoBuilder.setName(entry.getKey()); - entry.getValue().ifPresent(fieldInfoBuilder::setDocString); - providerInfoBuilder.addFieldInfo(fieldInfoBuilder.build()); + private ProviderNameGroup buildProviderNameGroup( + ImmutableSet<StarlarkProviderIdentifier> providerGroup) { + ProviderNameGroup.Builder providerNameGroupBuilder = ProviderNameGroup.newBuilder(); + for (StarlarkProviderIdentifier provider : providerGroup) { + providerNameGroupBuilder.addProviderName(getDocumentedProviderName(provider)); + OriginKey.Builder providerKeyBuilder = OriginKey.newBuilder().setName(provider.toString()); + if (!provider.isLegacy()) { + if (provider.getKey() instanceof StarlarkProvider.Key) { + Label definingModule = ((StarlarkProvider.Key) provider.getKey()).getExtensionLabel(); + providerKeyBuilder.setFile(definingModule.getDisplayForm(repositoryMapping)); + } else if (provider.getKey() instanceof BuiltinProvider.Key) { + providerKeyBuilder.setFile("<native>"); + } } + providerNameGroupBuilder.addOriginKey(providerKeyBuilder.build()); } + return providerNameGroupBuilder.build(); } - moduleInfoBuilder.addProviderInfo(providerInfoBuilder); - } - - private void addAspectInfo( - ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkDefinedAspect aspect) - throws ExtractionException { - AspectInfo.Builder aspectInfoBuilder = AspectInfo.newBuilder(); - // Allow aspects to be exported under a different name (e.g. in a struct) - aspectInfoBuilder.setAspectName(exportedName); - // ... but record the origin aspect key for cross references. - aspectInfoBuilder.setOriginKey( - OriginKey.newBuilder() - .setName(aspect.getAspectClass().getExportedName()) - .setFile( - aspect.getAspectClass().getExtensionLabel().getDisplayForm(repositoryMapping))); - aspect.getDocumentation().ifPresent(aspectInfoBuilder::setDocString); - aspectInfoBuilder.addAllAspectAttribute(aspect.getAttributeAspects()); - aspectInfoBuilder.addAttribute(IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first - for (Attribute attribute : aspect.getAttributes()) { - if (isExportableName(attribute.getPublicName())) { - aspectInfoBuilder.addAttribute(buildAttributeInfo(attribute, "aspect " + exportedName)); - } - } - moduleInfoBuilder.addAspectInfo(aspectInfoBuilder); } }
diff --git a/src/main/java/com/google/devtools/build/skydoc/rendering/proto/stardoc_output.proto b/src/main/java/com/google/devtools/build/skydoc/rendering/proto/stardoc_output.proto index 82439b7..f2abf86 100644 --- a/src/main/java/com/google/devtools/build/skydoc/rendering/proto/stardoc_output.proto +++ b/src/main/java/com/google/devtools/build/skydoc/rendering/proto/stardoc_output.proto
@@ -68,8 +68,8 @@ // Representation of a Starlark rule definition. message RuleInfo { - // The name under which the rule is exported by this module, including any - // structs it is nested in, for example "foo.foo_library". + // The name under which the rule is made accessible to a user of this module, + // including any structs it is nested in, for example "foo.foo_library". string rule_name = 1; // The documentation string of the rule. @@ -146,8 +146,9 @@ // Representation of Starlark function definition. message StarlarkFunctionInfo { - // The name under which the function is exported by this module, including any - // structs it is nested in, for example "foo.frobnicate". + // The name under which the function is made accessible to a user of this + // module, including any structs it is nested in, for example + // "foo.frobnicate". string function_name = 1; // The parameters for the function. @@ -213,8 +214,8 @@ // Representation of a Starlark provider definition. message ProviderInfo { - // The name under which the provider is exported by this module, including any - // structs it is nested in, for example "foo.FooInfo". + // The name under which the provider is made accessible to a user of this + // module, including any structs it is nested in, for example "foo.FooInfo". string provider_name = 1; // The description of the provider. @@ -232,8 +233,9 @@ // Representation of a Starlark aspect definition. message AspectInfo { - // The name under which the aspect is exported by this module, including any - // structs it is nested in, for example "foo.foo_aspect". + // The name under which the aspect is made accessible to a user of this + // module, including any structs it is nested in, for example + // "foo.foo_aspect". string aspect_name = 1; // The documentation string of the aspect. @@ -267,4 +269,4 @@ // Java. Unset when there is no module file (such as for legacy struct // providers, when the module is a REPL, or in Bazel's internal tests). string file = 2; -} \ No newline at end of file +}
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 8022136..4196613 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
@@ -70,8 +70,8 @@ return new ModuleInfoExtractor(name -> true, RepositoryMapping.ALWAYS_FALLBACK); } - private static ModuleInfoExtractor getExtractor(Predicate<String> isWantedName) { - return new ModuleInfoExtractor(isWantedName, RepositoryMapping.ALWAYS_FALLBACK); + private static ModuleInfoExtractor getExtractor(Predicate<String> isWantedGlobal) { + return new ModuleInfoExtractor(isWantedGlobal, RepositoryMapping.ALWAYS_FALLBACK); } private static ModuleInfoExtractor getExtractor(RepositoryMapping repositoryMapping) { @@ -89,21 +89,21 @@ } @Test - public void extractOnlyWantedExportableNames() throws Exception { + public void extractOnlyWantedLoadableNames() throws Exception { Module module = exec( - "def exported_unwanted():", + "def loadable_unwanted():", " pass", - "def exported_wanted():", + "def loadable_wanted():", " pass", - "def _nonexported():", + "def _nonloadable():", " pass", - "def _nonexported_matches_wanted_predicate():", + "def _nonloadable_matches_wanted_predicate():", " pass"); ModuleInfo moduleInfo = getExtractor(name -> name.contains("_wanted")).extractFrom(module); assertThat(moduleInfo.getFuncInfoList().stream().map(StarlarkFunctionInfo::getFunctionName)) - .containsExactly("exported_wanted"); + .containsExactly("loadable_wanted"); } @Test @@ -568,6 +568,42 @@ } @Test + public void providerNameGroups_useFirstDocumentableProviderName() throws Exception { + Module module = + exec( + "_MyInfo = provider()", + "def _my_impl(ctx):", + " pass", + "my_lib = rule(", + " implementation = _my_impl,", + " attrs = {", + " 'foo': attr.label(providers = [_MyInfo]),", + " },", + " provides = [_MyInfo],", + ")", + "namespace1 = struct(_MyUndocumentedInfo = _MyInfo)", + "namespace2 = struct(MyInfoB = _MyInfo, MyInfoA = _MyInfo)", + "namespace3 = struct(MyInfo = _MyInfo)"); + ModuleInfo moduleInfo = getExtractor().extractFrom(module); + assertThat(moduleInfo.getRuleInfoList().get(0).getAdvertisedProviders().getProviderName(0)) + // Struct fields are extracted in field name alphabetical order, so namespace2.MyInfoA + // (despite being declared after namespace2.MyInfoB) wins. + .isEqualTo("namespace2.MyInfoA"); + assertThat( + moduleInfo + .getRuleInfoList() + .get(0) + .getAttribute(1) // 0 is the implicit name attribute + .getProviderNameGroup(0) + .getProviderName(0)) + .isEqualTo("namespace2.MyInfoA"); + assertThat(moduleInfo.getProviderInfoList().stream().map(ProviderInfo::getProviderName)) + .containsExactly("namespace2.MyInfoA", "namespace2.MyInfoB", "namespace3.MyInfo"); + // TODO(arostovtsev): instead of producing a separate ProviderInfo message per each alias, add a + // repeated alias name field, and produce a single ProviderInfo message listing its aliases. + } + + @Test public void labelStringification() throws Exception { Module module = exec(
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 30ca10b..47e3b7e 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
@@ -189,26 +189,31 @@ "def my_macro():", " pass", "MyInfo = provider()", + "MyOtherInfo = provider()", "my_rule = rule(", " implementation = lambda ctx: None,", - " attrs = {'a': attr.label(providers = [MyInfo])},", - " provides = [MyInfo],", + " attrs = {'a': attr.label(providers = [MyInfo, MyOtherInfo])},", + " provides = [MyInfo, MyOtherInfo],", ")", "my_aspect = aspect(implementation = lambda target, ctx: None)"); scratch.file( "renamer.bzl", // - "load(':origin.bzl', 'my_macro', 'MyInfo', 'my_rule', 'my_aspect')", + "load(':origin.bzl', 'my_macro', 'MyInfo', 'MyOtherInfo', 'my_rule', 'my_aspect')", "namespace = struct(", " renamed_macro = my_macro,", " RenamedInfo = MyInfo,", " renamed_rule = my_rule,", " renamed_aspect = my_aspect,", + ")", + "other_namespace = struct(", + " RenamedOtherInfo = MyOtherInfo,", ")"); scratch.file( "BUILD", // "starlark_doc_extract(", " name = 'extract_renamed',", " src = 'renamer.bzl',", + " symbol_names = ['namespace'],", ")"); ModuleInfo moduleInfo = @@ -233,8 +238,6 @@ .containsExactly( OriginKey.newBuilder().setName("my_rule").setFile("//:origin.bzl").build()); - // TODO(b/276733504): arguably, provider_name in provider_name_group-s here should be - // "namespace.RenamedInfo", not "MyInfo". assertThat(moduleInfo.getRuleInfo(0).getAttributeList()) .containsExactly( ModuleInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO, @@ -244,15 +247,21 @@ .setDefaultValue("None") .addProviderNameGroup( ProviderNameGroup.newBuilder() - .addProviderName("MyInfo") + .addProviderName("namespace.RenamedInfo") + .addProviderName("other_namespace.RenamedOtherInfo") .addOriginKey( - OriginKey.newBuilder().setName("MyInfo").setFile("//:origin.bzl"))) + OriginKey.newBuilder().setName("MyInfo").setFile("//:origin.bzl")) + .addOriginKey( + OriginKey.newBuilder().setName("MyOtherInfo").setFile("//:origin.bzl"))) .build()); assertThat(moduleInfo.getRuleInfo(0).getAdvertisedProviders()) .isEqualTo( ProviderNameGroup.newBuilder() - .addProviderName("MyInfo") + .addProviderName("namespace.RenamedInfo") + .addProviderName("other_namespace.RenamedOtherInfo") .addOriginKey(OriginKey.newBuilder().setName("MyInfo").setFile("//:origin.bzl")) + .addOriginKey( + OriginKey.newBuilder().setName("MyOtherInfo").setFile("//:origin.bzl")) .build()); assertThat(moduleInfo.getAspectInfoList().stream().map(AspectInfo::getAspectName)) @@ -272,17 +281,6 @@ BzlmodTestUtil.createModuleKey("origin_repo", "0.1"), "module(name='origin_repo', version='0.1')"); Path originRepoPath = moduleRoot.getRelative("origin_repo~0.1"); - - rewriteWorkspace( - "local_repository(", - " name = 'origin_canonical',", - " path = '/origin_canonical',", - ")", - "local_repository(", - " name = 'renamer_repo',", - " path = '/renamer_repo',", - " repo_mapping = {'@origin_repo': '@origin_canonical'},", - ")"); scratch.file(originRepoPath.getRelative("WORKSPACE").getPathString()); scratch.file( originRepoPath.getRelative("BUILD").getPathString(), //