Skip to content

Commit

Permalink
Add module_ctx.fail_on_user_error
Browse files Browse the repository at this point in the history
The new function allows module extensions to fail in a way that is more
appropriate for user errors: No Starlark stack trace is prepended to a
failure, instead the optionally provided tags and their locations are
appended to the error message.
  • Loading branch information
fmeum committed Mar 27, 2023
1 parent 130703a commit e99efdb
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.bazel.bzlmod;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
Expand All @@ -22,11 +23,16 @@
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import java.util.List;
import java.util.Map;
import java.util.StringJoiner;
import javax.annotation.Nullable;
import net.starlark.java.annot.Param;
import net.starlark.java.annot.ParamType;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Sequence;
import net.starlark.java.eval.StarlarkList;
import net.starlark.java.eval.StarlarkSemantics;

Expand Down Expand Up @@ -99,4 +105,62 @@ protected ImmutableMap<String, String> getRemoteExecProperties() throws EvalExce
public StarlarkList<StarlarkBazelModule> getModules() {
return modules;
}

@StarlarkMethod(
name = "fail_on_user_error",
doc = "Fails the module extension with the given error message in reponse to a user error "
+ "such as a semantically invalid tag. Compared to <a "
+ "href=\"globals.html#fail\">fail</a>, the error message will not include a Starlark "
+ "stack trace, but instead emits information about the given tags and their locations "
+ "after the message.",
parameters = {
@Param(
name = "message",
allowedTypes = {@ParamType(type = String.class)},
doc = "The error message to display to the user."),
},
extraPositionals = @Param(
name = "tags",
allowedTypes = {@ParamType(type = Sequence.class, generic1 = TypeCheckedTag.class)}
)
)
public void fail(String message, Sequence<?> tags) throws EvalException {
Sequence<TypeCheckedTag> checkedTags = Sequence.cast(tags, TypeCheckedTag.class, "tags");
throw new InvalidTagEvalException(message, checkedTags, extensionId);
}

static final class InvalidTagEvalException extends EvalException {

private final ImmutableList<TypeCheckedTag> tags;
private final ModuleExtensionId extensionId;

InvalidTagEvalException(String message, List<TypeCheckedTag> tags,
ModuleExtensionId extensionId) {
super(message);
this.tags = ImmutableList.copyOf(tags);
this.extensionId = extensionId;
}

public String getMessageWithTags() {
String tagInfo = tags.stream()
.map(this::formatTag)
.collect(
() -> new StringJoiner("\n", "\n\n", "").setEmptyValue(""),
StringJoiner::add,
StringJoiner::merge)
.toString();
return String.format(
"Reported by module extension %s in %s:\n%s%s",
extensionId.getExtensionName(), extensionId.getBzlFileLabel(), getMessage(), tagInfo);
}

private String formatTag(TypeCheckedTag tag) {
// Tag instantiations are expected to be top-level statements, so the column number isn't
// useful.
return String.format("File \"%s\", line %d, evaluated to:\n%s",
tag.getLocation().file(),
tag.getLocation().line(),
extensionId.getExtensionName() + "." + tag.prettyPrint());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleExtensionContext.InvalidTagEvalException;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.cmdline.BazelModuleContext;
import com.google.devtools.build.lib.cmdline.LabelConstants;
Expand Down Expand Up @@ -195,6 +196,16 @@ public SkyValue compute(SkyKey skyKey, Environment env)
throw new SingleExtensionEvalFunctionException(e1, Transience.TRANSIENT);
}
return null;
} catch (InvalidTagEvalException e) {
env.getListener().handle(Event.error(e.getMessageWithTags()));
throw new SingleExtensionEvalFunctionException(
ExternalDepsException.withCauseAndMessage(
Code.BAD_MODULE,
e,
"Error evaluating module extension %s in %s",
extensionId.getExtensionName(),
extensionId.getBzlFileLabel()),
Transience.TRANSIENT);
} catch (EvalException e) {
env.getListener().handle(Event.error(e.getMessageWithStack()));
throw new SingleExtensionEvalFunctionException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,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.getTagName(), tag, labelConverter));
}
return new StarlarkBazelModule(
module.getName(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,36 @@
import javax.annotation.Nullable;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
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 {

private final TagClass tagClass;
private final String tagClassName;
private final Object[] attrValues;
private final Location location;

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

/** Creates a {@link TypeCheckedTag}. */
public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter labelConverter)
/**
* Creates a {@link TypeCheckedTag}.
*/
public static TypeCheckedTag create(TagClass tagClass, String tagClassName, Tag tag,
LabelConverter labelConverter)
throws ExternalDepsException {
Object[] attrValues = new Object[tagClass.getAttributes().size()];
for (Map.Entry<String, Object> attrValue : tag.getAttributeValues().entrySet()) {
Expand Down Expand Up @@ -95,7 +106,7 @@ 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, tagClassName, attrValues, tag.getLocation());
}

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

public Location getLocation() {
return location;
}

public String prettyPrint() {
StringBuilder sb = new StringBuilder();
sb.append(tagClassName);
sb.append("(\n");
for (int i = 0; i < attrValues.length; i++) {
Attribute attr = tagClass.getAttributes().get(i);
sb.append(" ");
sb.append(attr.getPublicName());
sb.append(" = ");
sb.append(Starlark.repr(attrValues[i]));
sb.append(",\n");
}
sb.append(")");
return sb.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ private static Object getattr(Structure structure, String fieldName) throws Exce
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);
createTagClass(attr("foo", Type.INTEGER).build()), "tag_name",
/*labelConverter=*/ buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).build(),
null);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(StarlarkInt.of(3));
}
Expand All @@ -73,11 +73,11 @@ public void label() throws Exception {
TypeCheckedTag.create(
createTagClass(
attr("foo", BuildType.LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE).build()),
"tag_name",
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")));
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
Expand All @@ -95,8 +95,8 @@ public void label_withoutDefaultValue() throws Exception {
TypeCheckedTag.create(
createTagClass(
attr("foo", BuildType.LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build()),
buildTag("tag_name").build(),
new LabelConverter(
"tag_name",
buildTag("tag_name").build(), new LabelConverter(
PackageIdentifier.parse("@myrepo//mypkg"),
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo")));
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
Expand All @@ -110,9 +110,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()), "tag_name",
buildTag("tag_name").build(), null);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo"))
.isEqualTo(
Expand All @@ -128,12 +127,11 @@ 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()), "tag_name",
/*labelConverter=*/ buildTag("tag_name")
.addAttr("foo", "fooValue")
.addAttr("quux", StarlarkList.immutableOf("quuxValue1", "quuxValue2"))
.build(),
/*labelConverter=*/ null);
.build(), null);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo", "bar", "quux");
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo("fooValue");
assertThat(getattr(typeCheckedTag, "bar")).isEqualTo(StarlarkInt.of(3));
Expand All @@ -148,9 +146,8 @@ public void mandatory() throws Exception {
ExternalDepsException.class,
() ->
TypeCheckedTag.create(
createTagClass(attr("foo", Type.STRING).mandatory().build()),
buildTag("tag_name").build(),
/*labelConverter=*/ null));
createTagClass(attr("foo", Type.STRING).mandatory().build()), "tag_name",
/*labelConverter=*/ buildTag("tag_name").build(), null));
assertThat(e).hasMessageThat().contains("mandatory attribute foo isn't being specified");
}

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

0 comments on commit e99efdb

Please sign in to comment.