Skip to content

Commit

Permalink
Make attribute and rule info extraction non-throwing
Browse files Browse the repository at this point in the history
Until `bazel query` can properly handle formatter exceptions which are not i/o
exceptions, buildRuleInfo must not throw in order to be usable in query output.

PiperOrigin-RevId: 680575449
Change-Id: If0b55bebc62ca92a1ca0731b6489a2b1ab90eec7
  • Loading branch information
tetromino authored and copybara-github committed Sep 30, 2024
1 parent 6f946cf commit 6e6c9a8
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@ public final class AttributeInfoExtractor {
+ " might create will be this name with a string suffix.")
.build();

static AttributeInfo buildAttributeInfo(
ExtractorContext context, Attribute attribute, String where) throws ExtractionException {
@VisibleForTesting public static final String UNREPRESENTABLE_VALUE = "<unrepresentable value>";

static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attribute) {
AttributeInfo.Builder builder =
AttributeInfo.newBuilder()
.setName(attribute.getPublicName())
.setType(getAttributeType(attribute, where))
.setType(getAttributeType(context, attribute.getType(), attribute.getPublicName()))
.setMandatory(attribute.isMandatory());
Optional.ofNullable(attribute.getDoc()).ifPresent(builder::setDocString);
if (!attribute.isConfigurable()) {
Expand All @@ -76,22 +77,14 @@ static AttributeInfo buildAttributeInfo(
Object defaultValue = Attribute.valueToStarlark(attribute.getDefaultValueUnchecked());
builder.setDefaultValue(context.labelRenderer().reprWithoutLabelConstructor(defaultValue));
} catch (InvalidStarlarkValueException e) {
throw new ExtractionException(
String.format(
"in %s attribute %s: failed to convert default value to Starlark: %s",
where, attribute.getPublicName(), e.getMessage()),
e);
builder.setDefaultValue(UNREPRESENTABLE_VALUE);
}
}
return builder.build();
}

static void addDocumentableAttributes(
ExtractorContext context,
Iterable<Attribute> attributes,
Consumer<AttributeInfo> builder,
String where)
throws ExtractionException {
ExtractorContext context, Iterable<Attribute> attributes, Consumer<AttributeInfo> builder) {
for (Attribute attribute : attributes) {
if (attribute.getName().equals(IMPLICIT_NAME_ATTRIBUTE_INFO.getName())) {
// We inject our own IMPLICIT_NAME_ATTRIBUTE_INFO with better documentation.
Expand All @@ -100,14 +93,13 @@ static void addDocumentableAttributes(
if ((attribute.starlarkDefined() || context.extractNonStarlarkAttrs())
&& attribute.isDocumented()
&& ExtractorContext.isPublicName(attribute.getPublicName())) {
builder.accept(buildAttributeInfo(context, attribute, where));
builder.accept(buildAttributeInfo(context, attribute));
}
}
}

static AttributeType getAttributeType(Attribute attribute, String where)
throws ExtractionException {
Type<?> type = attribute.getType();
static AttributeType getAttributeType(
ExtractorContext context, Type<?> type, String attributePublicName) {
if (type.equals(Type.INTEGER)) {
return AttributeType.INT;
} else if (type.equals(BuildType.LABEL)
Expand All @@ -116,7 +108,7 @@ static AttributeType getAttributeType(Attribute attribute, String where)
|| type.equals(BuildType.DORMANT_LABEL)) {
return AttributeType.LABEL;
} else if (type.equals(Type.STRING) || type.equals(Type.STRING_NO_INTERN)) {
if (attribute.getPublicName().equals("name")) {
if (attributePublicName.equals("name")) {
return AttributeType.NAME;
} else {
return AttributeType.STRING;
Expand Down Expand Up @@ -156,10 +148,7 @@ static AttributeType getAttributeType(Attribute attribute, String where)
return AttributeType.INT;
}

throw new ExtractionException(
String.format(
"in %s attribute %s: unsupported type %s",
where, attribute.getPublicName(), type.getClass().getSimpleName()));
return AttributeType.UNKNOWN;
}

private AttributeInfoExtractor() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ java_library(
"RuleInfoExtractor.java",
],
deps = [
":extractionexception",
":labelrenderer",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/packages",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,7 @@ protected void visitMacroFunction(String qualifiedName, MacroFunction macroFunct
// inject the name attribute; addDocumentableAttributes skips non-Starlark-defined attributes.
macroInfoBuilder.addAttribute(AttributeInfoExtractor.IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO);
AttributeInfoExtractor.addDocumentableAttributes(
context,
macroClass.getAttributes().values(),
macroInfoBuilder::addAttribute,
"macro " + qualifiedName);
context, macroClass.getAttributes().values(), macroInfoBuilder::addAttribute);

moduleInfoBuilder.addMacroInfo(macroInfoBuilder);
}
Expand Down Expand Up @@ -425,10 +422,7 @@ protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect)
aspectInfoBuilder.addAttribute(
AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first
AttributeInfoExtractor.addDocumentableAttributes(
context,
aspect.getAttributes(),
aspectInfoBuilder::addAttribute,
"aspect " + qualifiedName);
context, aspect.getAttributes(), aspectInfoBuilder::addAttribute);
moduleInfoBuilder.addAspectInfo(aspectInfoBuilder);
}

Expand All @@ -452,10 +446,7 @@ protected void visitModuleExtension(String qualifiedName, ModuleExtension module
tagClassInfoBuilder.setTagName(entry.getKey());
entry.getValue().getDoc().ifPresent(tagClassInfoBuilder::setDocString);
AttributeInfoExtractor.addDocumentableAttributes(
context,
entry.getValue().getAttributes(),
tagClassInfoBuilder::addAttribute,
String.format("module extension %s tag class %s", qualifiedName, entry.getKey()));
context, entry.getValue().getAttributes(), tagClassInfoBuilder::addAttribute);
moduleExtensionInfoBuilder.addTagClass(tagClassInfoBuilder);
}
moduleInfoBuilder.addModuleExtensionInfo(moduleExtensionInfoBuilder);
Expand All @@ -477,10 +468,7 @@ protected void visitRepositoryRule(
context.labelRenderer().render(repositoryRuleFunction.getExtensionLabel())))
.addAllAttribute(IMPLICIT_REPOSITORY_RULE_ATTRIBUTES);
AttributeInfoExtractor.addDocumentableAttributes(
context,
ruleClass.getAttributes(),
repositoryRuleInfoBuilder::addAttribute,
"repository rule " + qualifiedName);
context, ruleClass.getAttributes(), repositoryRuleInfoBuilder::addAttribute);
if (ruleClass.hasAttr("$environ", Types.STRING_LIST)) {
repositoryRuleInfoBuilder.addAllEnviron(
Types.STRING_LIST.cast(ruleClass.getAttributeByName("$environ").getDefaultValue(null)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ public final class RuleInfoExtractor {
* Starlark rule defined in a .bzl file, this would typically be the name under which users of
* the module would use the rule
* @param ruleClass a rule class; if it is a repository rule class, it must be an exported one
* @throws ExtractionException if extraction failed
*/
public static RuleInfo buildRuleInfo(
ExtractorContext context, String qualifiedName, RuleClass ruleClass)
throws ExtractionException {
ExtractorContext context, String qualifiedName, RuleClass ruleClass) {
RuleInfo.Builder ruleInfoBuilder = RuleInfo.newBuilder();
// Record the name under which this symbol is made accessible, which may differ from the
// symbol's exported name
Expand Down Expand Up @@ -77,7 +75,7 @@ public static RuleInfo buildRuleInfo(
ruleInfoBuilder.addAttribute(
AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first
AttributeInfoExtractor.addDocumentableAttributes(
context, ruleClass.getAttributes(), ruleInfoBuilder::addAttribute, "rule " + qualifiedName);
context, ruleClass.getAttributes(), ruleInfoBuilder::addAttribute);
ImmutableSet<StarlarkProviderIdentifier> advertisedProviders =
ruleClass.getAdvertisedProviders().getStarlarkProviders();
if (!advertisedProviders.isEmpty()) {
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ filegroup(

java_library(
name = "PackageTestsUtil",
srcs = ["WorkspaceFactoryTestHelper.java"],
srcs = [
"BuildTypeTestHelper.java",
"WorkspaceFactoryTestHelper.java",
],
deps = [
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.packages.BuildType.Selector;
import com.google.devtools.build.lib.packages.Type.ConversionException;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -603,7 +601,12 @@ public void selectableConvert_simplifyingUnconditionals_handlesUnconditionalNone
new SelectorValue(
ImmutableMap.of(BuildType.Selector.DEFAULT_CONDITION_KEY, Starlark.NONE), "");

for (Type<?> type : getAllBuildTypes()) {
ImmutableList<Type<?>> allBuildTypes =
BuildTypeTestHelper.getAllBuildTypes(/* publicOnly= */ false);
// Verify that we really collected both scalar and non-scalar types from all classes.
assertThat(allBuildTypes)
.containsAtLeast(Type.STRING, Types.STRING_LIST, BuildType.LABEL, BuildType.LABEL_LIST);
for (Type<?> type : allBuildTypes) {
// select({"//conditions:default": None}) simplifies to the type's default value.
assertThat(
BuildType.selectableConvert(
Expand Down Expand Up @@ -644,27 +647,6 @@ public void selectableConvert_simplifyingUnconditionals_handlesUnconditionalNone
}
}

private static void collectBuildTypeStaticFields(
ImmutableList.Builder<Type<?>> builder, Class<?> clazz) throws IllegalAccessException {
for (Field field : clazz.getDeclaredFields()) {
if (Modifier.isStatic(field.getModifiers()) && Type.class.isAssignableFrom(field.getType())) {
builder.add((Type<?>) field.get(null));
}
}
}

private static ImmutableList<Type<?>> getAllBuildTypes() throws IllegalAccessException {
ImmutableList.Builder<Type<?>> builder = ImmutableList.builder();
collectBuildTypeStaticFields(builder, Type.class);
collectBuildTypeStaticFields(builder, Types.class);
collectBuildTypeStaticFields(builder, BuildType.class);
ImmutableList<Type<?>> types = builder.build();
// Verify that we really collected both scalar and non-scalar types from all classes.
assertThat(types)
.containsAtLeast(Type.STRING, Types.STRING_LIST, BuildType.LABEL, BuildType.LABEL_LIST);
return types;
}

@Test
public void selectableConvert_simplifyingUnconditionals_failsCleanlyOnInvalidConcatenation()
throws Exception {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Copyright 2024 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.packages;

import com.google.common.collect.ImmutableList;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;

/** Utils for testing {@link Type}, {@link Types}, and {@link BuildType}. */
// Must live in the same package as BuildType to access non-public fields.
public final class BuildTypeTestHelper {

/**
* Returns all build types defined in {@link Type}, {@link Types}, and {@link BuildType}.
*
* @param publicOnly if true, only returns public types; otherwise, returns both public and
* internal package-private ones.
*/
public static ImmutableList<Type<?>> getAllBuildTypes(boolean publicOnly)
throws IllegalAccessException {
ImmutableList.Builder<Type<?>> builder = ImmutableList.builder();
collectBuildTypeStaticFields(builder, Type.class, publicOnly);
collectBuildTypeStaticFields(builder, Types.class, publicOnly);
collectBuildTypeStaticFields(builder, BuildType.class, publicOnly);
return builder.build();
}

private static void collectBuildTypeStaticFields(
ImmutableList.Builder<Type<?>> builder, Class<?> clazz, boolean publicOnly)
throws IllegalAccessException {
for (Field field : clazz.getDeclaredFields()) {
if (Modifier.isStatic(field.getModifiers())
&& (Modifier.isPublic(field.getModifiers()) || !publicOnly)
&& Type.class.isAssignableFrom(field.getType())) {
builder.add((Type<?>) field.get(null));
}
}
}

private BuildTypeTestHelper() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/starlarkdocextract:labelrenderer",
"//src/main/java/com/google/devtools/build/lib/starlarkdocextract:ruleinfoextractor",
"//src/main/protobuf:stardoc_output_java_proto",
"//src/test/java/com/google/devtools/build/lib/packages:PackageTestsUtil",
"//src/test/java/com/google/devtools/build/lib/packages:testutil",
"//third_party:junit4",
"//third_party:truth",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@
package com.google.devtools.build.lib.starlarkdocextract;

import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.Types.STRING_LIST;

import com.google.devtools.build.lib.packages.BuildTypeTestHelper;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeInfo;
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeType;
Expand Down Expand Up @@ -68,6 +71,17 @@ public void basicFunctionality() throws Exception {
.build());
}

@Test
public void allBuiltinAttributeTypesSupported() throws Exception {
ExtractorContext context =
ExtractorContext.builder().labelRenderer(LabelRenderer.DEFAULT).build();
for (Type<?> type : BuildTypeTestHelper.getAllBuildTypes(/* publicOnly= */ true)) {
assertWithMessage("attr type '%s'", type)
.that(AttributeInfoExtractor.getAttributeType(context, type, "test_attr"))
.isNotEqualTo(AttributeType.UNKNOWN);
}
}

@Test
public void allNativeRulesAreSupported() throws Exception {
ExtractorContext extractorContext =
Expand All @@ -80,11 +94,18 @@ public void allNativeRulesAreSupported() throws Exception {
RuleInfoExtractor.buildRuleInfo(extractorContext, ruleClass.getName(), ruleClass);
assertThat(ruleInfo.getRuleName()).isEqualTo(ruleClass.getName());
assertThat(ruleInfo.getOriginKey().getName()).isEqualTo(ruleClass.getName());
assertThat(ruleInfo.getOriginKey().getFile()).isEqualTo("<native>");
assertThat(ruleInfo.getAttributeList().getFirst())
assertWithMessage("rule '%s'", ruleClass.getName())
.that(ruleInfo.getOriginKey().getFile())
.isEqualTo("<native>");
assertWithMessage("rule '%s'", ruleClass.getName())
.that(ruleInfo.getAttributeList().getFirst())
.isEqualTo(AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO);
assertThat(ruleInfo.getAttributeList().stream().map(AttributeInfo::getName))
assertWithMessage("rule '%s'", ruleClass.getName())
.that(ruleInfo.getAttributeList().stream().map(AttributeInfo::getName))
.containsNoDuplicates();
assertWithMessage("rule '%s'", ruleClass.getName())
.that(ruleInfo.getAttributeList().stream().map(AttributeInfo::getDefaultValue))
.doesNotContain(AttributeInfoExtractor.UNREPRESENTABLE_VALUE);
}
}
}

0 comments on commit 6e6c9a8

Please sign in to comment.