Skip to content

Commit

Permalink
In starlark_doc_extract, apply symbol_names filter to qualified names…
Browse files Browse the repository at this point in the history 
…, not just globals

In other words, if symbol_names = ["foo.bar"], it means:
* if "foo.bar" is the qualified name of an exported function / provider / rule / aspect - we
  want to document it;
* if "foo.bar" is the qualified name of an exported struct - we want to document documentable
  entities (transitively) embedded in it

This matches legacy Stardoc behavior.

PiperOrigin-RevId: 542970711
Change-Id: I1b31e6a2be9929473a3c1c1c90e599c114980df1
  • Loading branch information
@tetromino @copybara-github
tetromino authored and copybara-github committed Jun 23, 2023
1 parent 5f518bd commit 5a02197
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 45 deletions.
101 changes: 62 additions & 39 deletions ...main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@

/** API documentation extractor for a compiled, loaded Starlark module. */
final class ModuleInfoExtractor {
private final Predicate<String> isWantedGlobal;
private final Predicate<String> isWantedQualifiedName;
private final RepositoryMapping repositoryMapping;

@VisibleForTesting
Expand All @@ -74,15 +74,17 @@ final class ModuleInfoExtractor {
/**
* Constructs an instance of {@code ModuleInfoExtractor}.
*
* @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 isWantedQualifiedName a predicate to filter the module's qualified names. A qualified
* name is documented if and only if (1) each component of the qualified name is public (in
* other words, the first character of each component of the qualified name is alphabetic) and
* (2) the qualified name, or one of its ancestor qualified names, satisfies the wanted
* predicate.
* @param repositoryMapping the repository mapping for the repo in which we want to render labels
* as strings
*/
public ModuleInfoExtractor(
Predicate<String> isWantedGlobal, RepositoryMapping repositoryMapping) {
this.isWantedGlobal = isWantedGlobal;
Predicate<String> isWantedQualifiedName, RepositoryMapping repositoryMapping) {
this.isWantedQualifiedName = isWantedQualifiedName;
this.repositoryMapping = repositoryMapping;
}

Expand All @@ -104,7 +106,7 @@ public ModuleInfo extractFrom(Module module) throws ExtractionException {
DocumentationExtractor documentationExtractor =
new DocumentationExtractor(
builder,
isWantedGlobal,
isWantedQualifiedName,
repositoryMapping,
providerQualifiedNameCollector.buildQualifiedNames());
documentationExtractor.traverse(module);
Expand Down Expand Up @@ -138,34 +140,49 @@ 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());
if (isPublicName(globalSymbol)) {
maybeVisit(globalSymbol, entry.getValue(), /* shouldVisitVerifiedForAncestor= */ false);
}
}
}

/** Returns whether the visitor should visit (and possibly recurse into) the given global. */
protected abstract boolean shouldVisitGlobal(String globalSymbol);
/**
* Returns whether the visitor should visit (and possibly recurse into) the value with the given
* qualified name. Note that the visitor will not visit global names and struct fields for which
* {@link #isPublicName} is false, regardless of {@code shouldVisit}.
*/
protected abstract boolean shouldVisit(String qualifiedName);

/**
* @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
* @param shouldVisitVerifiedForAncestor whether {@link #shouldVisit} was verified true for an
* ancestor struct's qualified name; e.g. {@code qualifiedName} is "a.b.c.d" and {@code
* shouldVisit("a.b") == true}
*/
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);
private void maybeVisit(
String qualifiedName, Object value, boolean shouldVisitVerifiedForAncestor)
throws ExtractionException {
if (shouldVisitVerifiedForAncestor || shouldVisit(qualifiedName)) {
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) {
recurseIntoStructure(
qualifiedName, (Structure) value, /* shouldVisitVerifiedForAncestor= */ true);
}
} else if (value instanceof Structure) {
visitStructure(qualifiedName, (Structure) value);
recurseIntoStructure(
qualifiedName, (Structure) value, /* shouldVisitVerifiedForAncestor= */ false);
}
// 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.
// If the value is a constant (string, list etc.), 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.
Expand All @@ -182,14 +199,18 @@ protected void visitFunction(String qualifiedName, StarlarkFunction value)
protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect)
throws ExtractionException {}

private void visitStructure(String qualifiedName, Structure structure)
private void recurseIntoStructure(
String qualifiedName, Structure structure, boolean shouldVisitVerifiedForAncestor)
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);
maybeVisit(
String.format("%s.%s", qualifiedName, fieldName),
fieldValue,
shouldVisitVerifiedForAncestor);
}
} catch (EvalException e) {
throw new ExtractionException(
Expand Down Expand Up @@ -222,16 +243,16 @@ public ImmutableMap<StarlarkProvider.Key, String> buildQualifiedNames() {
}

/**
* Returns true if the symbol is a loadable name (starts with an alphabetic character, not '_').
* Returns true always.
*
* <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.
* ModuleInfoExtractor#isWantedQualifiedName, 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 isWantedQualifiedName filter.
*/
@Override
protected boolean shouldVisitGlobal(String globalSymbol) {
return isPublicName(globalSymbol);
protected boolean shouldVisit(String qualifiedName) {
return true;
}

@Override
Expand All @@ -243,34 +264,36 @@ protected void visitProvider(String qualifiedName, StarlarkProvider value) {
/** 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 Predicate<String> isWantedQualifiedName;
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 isWantedQualifiedName a predicate to filter the module's qualified names. A qualified
* name is documented if and only if (1) each component of the qualified name is public (in
* other words, the first character of each component of the qualified name is alphabetic)
* and (2) the qualified name, or one of its ancestor qualified names, satisfies the wanted
* predicate.
* @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,
Predicate<String> isWantedQualifiedName,
RepositoryMapping repositoryMapping,
ImmutableMap<StarlarkProvider.Key, String> providerQualifiedNames) {
this.moduleInfoBuilder = moduleInfoBuilder;
this.isWantedGlobal = isWantedGlobal;
this.isWantedQualifiedName = isWantedQualifiedName;
this.repositoryMapping = repositoryMapping;
this.providerQualifiedNames = providerQualifiedNames;
}

@Override
protected boolean shouldVisitGlobal(String globalSymbol) {
return isPublicName(globalSymbol) && isWantedGlobal.test(globalSymbol);
protected boolean shouldVisit(String qualifiedName) {
return isWantedQualifiedName.test(qualifiedName);
}

@Override
Expand Down
54 changes: 48 additions & 6 deletions .../java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ private static ModuleInfoExtractor getExtractor() {
return new ModuleInfoExtractor(name -> true, RepositoryMapping.ALWAYS_FALLBACK);
}

private static ModuleInfoExtractor getExtractor(Predicate<String> isWantedGlobal) {
return new ModuleInfoExtractor(isWantedGlobal, RepositoryMapping.ALWAYS_FALLBACK);
private static ModuleInfoExtractor getExtractor(Predicate<String> isWantedQualifiedName) {
return new ModuleInfoExtractor(isWantedQualifiedName, RepositoryMapping.ALWAYS_FALLBACK);
}

private static ModuleInfoExtractor getExtractor(RepositoryMapping repositoryMapping) {
Expand All @@ -89,7 +89,7 @@ public void moduleDocstring() throws Exception {
}

@Test
public void extractOnlyWantedLoadableNames() throws Exception {
public void extractOnlyWantedLoadablePublicNames() throws Exception {
Module module =
exec(
"def loadable_unwanted():",
Expand All @@ -99,15 +99,26 @@ public void extractOnlyWantedLoadableNames() throws Exception {
"def _nonloadable():",
" pass",
"def _nonloadable_matches_wanted_predicate():",
" pass");
" pass",
"def _f():",
" pass",
"def _g():",
" pass",
"def _h():",
" pass",
"namespace = struct(",
" public_field_wanted = _f,",
" public_field_unwanted = _g,",
" _hidden_field_matches_wanted_predicate = _h,",
")");

ModuleInfo moduleInfo = getExtractor(name -> name.contains("_wanted")).extractFrom(module);
assertThat(moduleInfo.getFuncInfoList().stream().map(StarlarkFunctionInfo::getFunctionName))
.containsExactly("loadable_wanted");
.containsExactly("loadable_wanted", "namespace.public_field_wanted");
}

@Test
public void namespaces() throws Exception {
public void namespacedEntities() throws Exception {
Module module =
exec(
"def _my_func(**kwargs):",
Expand Down Expand Up @@ -157,6 +168,37 @@ public void namespaces() throws Exception {
.containsExactly("_MyInfo");
}

@Test
public void isWantedQualifiedName_appliesToQualifiedNamePrefixes() throws Exception {
Module module =
exec(
"def _f(): pass", //
"def _g(): pass",
"def _h(): pass",
"def _i(): pass",
"def _j(): pass",
"foo = struct(",
" bar = struct(",
" f = _f,",
" ),",
" baz = struct(",
" g = _g,",
" ),",
" h = _h,",
")",
"baz = struct(",
" qux = struct(",
" i = _i,",
" ),",
" j = _j,",
")");

ModuleInfo moduleInfo =
getExtractor(name -> name.equals("foo.bar") || name.equals("baz")).extractFrom(module);
assertThat(moduleInfo.getFuncInfoList().stream().map(StarlarkFunctionInfo::getFunctionName))
.containsExactly("foo.bar.f", "baz.qux.i", "baz.j");
}

@Test
public void functionDocstring() throws Exception {
Module module =
Expand Down

0 comments on commit 5a02197

Please sign in to comment.