Skip to content

Commit

Permalink
Make TypeCheckedTag a FailureCause
Browse files Browse the repository at this point in the history
By passing the Starlark representation of a module extension tag to
the new `causes` parameter of `fail`, module extensions can generate
more concise and helpful error messages on user errors.

Before:
```
ERROR: Traceback (most recent call last):
	File "/home/user/.cache/bazel/_bazel_user/466e6feac151a1e22847383f4e503361/external/gazelle~override/internal/bzlmod/go_deps.bzl", line 165, column 21, in _go_deps_impl
		fail("Multiple overrides defined for Go module path \"{}\" in module \"{}\".".format(override_tag.path, module.name))
Error in fail: Multiple overrides defined for Go module path "github.com/stretchr/testify" in module "gazelle_bcr_tests".
```

After:
```
ERROR: Multiple overrides defined for Go module path "github.com/stretchr/testify" in module "gazelle_bcr_tests".
Causes:
	bazel_module_tag in file "<root>/MODULE.bazel", line 39, column 25, with value:
		go_deps.gazelle_override(
		    path = "github.com/stretchr/testify",
		    directives = ["gazelle:go_naming_convention go_default_library"],
		)
```
  • Loading branch information
fmeum committed Mar 28, 2023
1 parent 9b8a063 commit 8346e89
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ private ModuleExtensionContext createContext(
StarlarkBazelModule.create(
abridgedModule,
extension,
extensionId,
usagesValue.getRepoMappings().get(moduleKey),
usagesValue.getExtensionUsages().get(moduleKey)));
} catch (ExternalDepsException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ private StarlarkBazelModule(String name, String version, Tags tags, boolean isRo
public static StarlarkBazelModule create(
AbridgedModule module,
ModuleExtension extension,
ModuleExtensionId extensionId,
RepositoryMapping repoMapping,
@Nullable ModuleExtensionUsage usage)
throws ExternalDepsException {
Expand Down Expand Up @@ -130,7 +131,7 @@ public static StarlarkBazelModule create(
// (for example, String to Label).
typeCheckedTags
.get(tag.getTagName())
.add(TypeCheckedTag.create(tagClass, tag, labelConverter));
.add(TypeCheckedTag.create(tagClass, tag, labelConverter, tag.getTagName(), extensionId));
}
return new StarlarkBazelModule(
module.getName(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,42 @@
import javax.annotation.Nullable;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.FailureCause;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.Structure;
import net.starlark.java.spelling.SpellChecker;
import net.starlark.java.syntax.Location;

/**
* A {@link Tag} whose attribute values have been type-checked against the attribute schema define
* in the {@link TagClass}.
*/
@StarlarkBuiltin(name = "bazel_module_tag", documented = false)
public class TypeCheckedTag implements Structure {
public class TypeCheckedTag implements FailureCause, Structure {

private final TagClass tagClass;
private final Object[] attrValues;

private TypeCheckedTag(TagClass tagClass, Object[] attrValues) {
// The properties below are only used for error reporting.
private final Location location;
private final String tagClassName;
private final String extensionName;

private TypeCheckedTag(TagClass tagClass, Object[] attrValues, Location location,
String tagClassName, String extensionName) {
this.tagClass = tagClass;
this.attrValues = attrValues;
this.location = location;
this.tagClassName = tagClassName;
this.extensionName = extensionName;
}

/** Creates a {@link TypeCheckedTag}. */
public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter labelConverter)
/**
* Creates a {@link TypeCheckedTag}.
*/
public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter labelConverter,
String tagClassName, ModuleExtensionId extensionId)
throws ExternalDepsException {
Object[] attrValues = new Object[tagClass.getAttributes().size()];
for (Map.Entry<String, Object> attrValue : tag.getAttributeValues().entrySet()) {
Expand Down Expand Up @@ -95,7 +112,8 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l
attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked());
}
}
return new TypeCheckedTag(tagClass, attrValues);
return new TypeCheckedTag(tagClass, attrValues, tag.getLocation(), tagClassName,
extensionId.getExtensionName());
}

@Override
Expand Down Expand Up @@ -123,4 +141,28 @@ public ImmutableCollection<String> getFieldNames() {
public String getErrorMessageForUnknownField(String field) {
return "unknown attribute " + field;
}

@Override
public void debugPrint(Printer printer, StarlarkSemantics semantics) {
printer.append(extensionName);
printer.append('.');
printer.append(tagClassName);
printer.append("(\n");

for (int i = 0; i < attrValues.length; i++) {
Attribute attr = tagClass.getAttributes().get(i);
printer.append(" ");
printer.append(attr.getPublicName());
printer.append(" = ");
printer.repr(attrValues[i]);
printer.append(",\n");
}

printer.append(")");
}

@Override
public Location getLocation() {
return location;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ public void basic() throws Exception {

StarlarkBazelModule moduleProxy =
StarlarkBazelModule.create(
abridgedModule, extension, module.getRepoMappingWithBazelDepsOnly(), usage);
abridgedModule, extension,
ModuleExtensionId.create(Label.parseCanonicalUnchecked("@foo//:extensions.bzl"), "ext"),
module.getRepoMappingWithBazelDepsOnly(), usage);

assertThat(moduleProxy.getName()).isEqualTo("foo");
assertThat(moduleProxy.getVersion()).isEqualTo("1.0");
Expand Down Expand Up @@ -136,7 +138,11 @@ public void unknownTagClass() throws Exception {
ExternalDepsException.class,
() ->
StarlarkBazelModule.create(
abridgedModule, extension, module.getRepoMappingWithBazelDepsOnly(), usage));
abridgedModule, extension,
ModuleExtensionId.create(Label.parseCanonicalUnchecked("@foo//:extensions.bzl"),
"ext"),
module.getRepoMappingWithBazelDepsOnly(),
usage));
assertThat(e).hasMessageThat().contains("does not have a tag class named blep");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,15 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Tests for {@link TypeCheckedTag}. */
/**
* Tests for {@link TypeCheckedTag}.
*/
@RunWith(JUnit4.class)
public class TypeCheckedTagTest {

private static final ModuleExtensionId EXT_ID = ModuleExtensionId.create(
Label.parseCanonicalUnchecked("@foo//:extensions.bzl"), "ext");

private static Object getattr(Structure structure, String fieldName) throws Exception {
return Starlark.getattr(
Mutability.IMMUTABLE,
Expand All @@ -61,8 +66,8 @@ public void basic() throws Exception {
TypeCheckedTag typeCheckedTag =
TypeCheckedTag.create(
createTagClass(attr("foo", Type.INTEGER).build()),
buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).build(),
/*labelConverter=*/ null);
buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).build(), /*labelConverter=*/
null, "tag_name", EXT_ID);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(StarlarkInt.of(3));
}
Expand All @@ -76,10 +81,11 @@ public void label() throws Exception {
buildTag("tag_name")
.addAttr(
"foo", StarlarkList.immutableOf(":thing1", "//pkg:thing2", "@repo//pkg:thing3"))
.build(),
new LabelConverter(
.build(), new LabelConverter(
PackageIdentifier.parse("@myrepo//mypkg"),
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo")));
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo")),
"tag_name", EXT_ID
);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo"))
.isEqualTo(
Expand All @@ -95,10 +101,11 @@ public void label_withoutDefaultValue() throws Exception {
TypeCheckedTag.create(
createTagClass(
attr("foo", BuildType.LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build()),
buildTag("tag_name").build(),
new LabelConverter(
buildTag("tag_name").build(), new LabelConverter(
PackageIdentifier.parse("@myrepo//mypkg"),
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo")));
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo")),
"tag_name", EXT_ID
);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(Starlark.NONE);
}
Expand All @@ -110,9 +117,8 @@ public void stringListDict_default() throws Exception {
createTagClass(
attr("foo", Type.STRING_LIST_DICT)
.value(ImmutableMap.of("key", ImmutableList.of("value1", "value2")))
.build()),
buildTag("tag_name").build(),
null);
.build()), buildTag("tag_name").build(), null, "tag_name", EXT_ID
);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo"))
.isEqualTo(
Expand All @@ -128,12 +134,10 @@ public void multipleAttributesAndDefaults() throws Exception {
createTagClass(
attr("foo", Type.STRING).mandatory().build(),
attr("bar", Type.INTEGER).value(StarlarkInt.of(3)).build(),
attr("quux", Type.STRING_LIST).build()),
buildTag("tag_name")
attr("quux", Type.STRING_LIST).build()), buildTag("tag_name")
.addAttr("foo", "fooValue")
.addAttr("quux", StarlarkList.immutableOf("quuxValue1", "quuxValue2"))
.build(),
/*labelConverter=*/ null);
.build(), /*labelConverter=*/null, "tag_name", EXT_ID);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo", "bar", "quux");
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo("fooValue");
assertThat(getattr(typeCheckedTag, "bar")).isEqualTo(StarlarkInt.of(3));
Expand All @@ -149,8 +153,7 @@ public void mandatory() throws Exception {
() ->
TypeCheckedTag.create(
createTagClass(attr("foo", Type.STRING).mandatory().build()),
buildTag("tag_name").build(),
/*labelConverter=*/ null));
buildTag("tag_name").build(), /*labelConverter=*/null, "tag_name", EXT_ID));
assertThat(e).hasMessageThat().contains("mandatory attribute foo isn't being specified");
}

Expand All @@ -165,8 +168,9 @@ public void allowedValues() throws Exception {
attr("foo", Type.STRING)
.allowedValues(new AllowedValueSet("yes", "no"))
.build()),
buildTag("tag_name").addAttr("foo", "maybe").build(),
/*labelConverter=*/ null));
buildTag("tag_name").addAttr("foo", "maybe").build(), /*labelConverter=*/null,
"tag_name", EXT_ID
));
assertThat(e)
.hasMessageThat()
.contains("the value for attribute foo has to be one of 'yes' or 'no' instead of 'maybe'");
Expand All @@ -180,8 +184,9 @@ public void unknownAttr() throws Exception {
() ->
TypeCheckedTag.create(
createTagClass(attr("foo", Type.STRING).build()),
buildTag("tag_name").addAttr("bar", "maybe").build(),
/*labelConverter=*/ null));
buildTag("tag_name").addAttr("bar", "maybe").build(), /*labelConverter=*/null,
"tag_name", EXT_ID
));
assertThat(e).hasMessageThat().contains("unknown attribute bar provided");
}
}

0 comments on commit 8346e89

Please sign in to comment.