bazel syntax: enforce that ClassObject.getValue returns legal Starlark values
EvalUtils.getAttr now enforces that the value returned by
ClassObject.getValue is a legal value.
Previously, only the ternary SkylarkClassObject.getValue method
offered this postcondition, and it wasn't enforced.
This turned up BuildConfiguration.Fragment,
which was used as a value (e.g. in ctx.fragments.android)
but had forgotten to declare its relationship to StarlarkValue.
Also:
- remove obsolete 'instanceof Class' check.
- remove try/catch of IllegalArgumentException.
Instead do this check with narrower scope in the fragment code.
We will fix other cases as they appear.
PiperOrigin-RevId: 284644428
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index f167e8a..0810a03 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -475,18 +475,25 @@
}
@Nullable
- public Fragment getSkylarkFragment(String name, ConfigurationTransition transition) {
+ public Fragment getSkylarkFragment(String name, ConfigurationTransition transition)
+ throws EvalException {
Class<? extends Fragment> fragmentClass =
getConfiguration(transition).getSkylarkFragmentByName(name);
if (fragmentClass == null) {
return null;
}
- return getFragment(fragmentClass, name,
- String.format(
- " Please update the '%1$sfragments' argument of the rule definition "
- + "(for example: %1$sfragments = [\"%2$s\"])",
- (transition.isHostTransition()) ? "host_" : "", name),
- transition);
+ try {
+ return getFragment(
+ fragmentClass,
+ name,
+ String.format(
+ " Please update the '%1$sfragments' argument of the rule definition "
+ + "(for example: %1$sfragments = [\"%2$s\"])",
+ transition.isHostTransition() ? "host_" : "", name),
+ transition);
+ } catch (IllegalArgumentException ex) { // fishy
+ throw new EvalException(null, ex.getMessage());
+ }
}
public ImmutableCollection<String> getSkylarkFragmentNames(ConfigurationTransition transition) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 9b07ff9..5fbd7bd 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -48,6 +48,7 @@
import com.google.devtools.build.lib.skylarkbuildapi.BuildConfigurationApi;
import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.syntax.StarlarkValue;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.RegexFilter;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -110,8 +111,10 @@
* <p>All implementations must be immutable and communicate this as clearly as possible (e.g.
* declare {@link ImmutableList} signatures on their interfaces vs. {@link List}). This is because
* fragment instances may be shared across configurations.
+ *
+ * <p>Fragments are Starlark values, as returned by {@code ctx.fragments.android}, for example.
*/
- public abstract static class Fragment {
+ public abstract static class Fragment implements StarlarkValue {
/**
* Validates the options for this Fragment. Issues warnings for the use of deprecated options,
* and warnings or errors for any option settings that conflict.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentCollection.java
index d396bcc..11f2ab7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentCollection.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skylarkbuildapi.FragmentCollectionApi;
+import com.google.devtools.build.lib.syntax.EvalException;
import javax.annotation.Nullable;
/** Represents a collection of configuration fragments in Skylark. */
@@ -35,7 +36,7 @@
@Override
@Nullable
- public Object getValue(String name) {
+ public Object getValue(String name) throws EvalException {
return ruleContext.getSkylarkFragment(name, transition);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 94c7aff..307e0fd 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -447,48 +447,37 @@
Eval.setDebugger(dbg);
}
- /** Returns the named field or method of the specified object. */
- static Object getAttr(StarlarkThread thread, Location loc, Object object, String name)
+ /** Returns the named field or method of value {@code x}, or null if not found. */
+ static Object getAttr(StarlarkThread thread, Location loc, Object x, String name)
throws EvalException, InterruptedException {
- MethodDescriptor method =
- object instanceof Class<?>
- ? CallUtils.getMethod(thread.getSemantics(), (Class<?>) object, name)
- : CallUtils.getMethod(thread.getSemantics(), object.getClass(), name);
+ // @SkylarkCallable-annotated field?
+ MethodDescriptor method = CallUtils.getMethod(thread.getSemantics(), x.getClass(), name);
if (method != null && method.isStructField()) {
return method.call(
- object,
+ x,
CallUtils.extraInterpreterArgs(method, /*ast=*/ null, loc, thread).toArray(),
loc,
thread);
}
- if (object instanceof SkylarkClassObject) {
- Object result = null;
- try {
- result = ((SkylarkClassObject) object).getValue(loc, thread.getSemantics(), name);
- } catch (IllegalArgumentException ex) { // TODO(adonovan): why necessary?
- throw new EvalException(loc, ex);
- }
- if (result != null) {
- return result;
- }
-
- } else if (object instanceof ClassObject) {
- Object result = null;
- try {
- result = ((ClassObject) object).getValue(name);
- } catch (IllegalArgumentException ex) {
- throw new EvalException(loc, ex);
- }
- // ClassObjects may have fields that are annotated with @SkylarkCallable.
- // Since getValue() does not know about those, we cannot expect that result is a valid object.
- if (result != null) {
- return Starlark.fromJava(result, thread.mutability());
+ // user-defined field?
+ if (x instanceof ClassObject) {
+ // TODO(adonovan): get rid of SkylarkClassObject once
+ // --incompatible_disable_target_provider_fields goes away,
+ // then remove the thread and loc parameters and publish
+ // this method as Starlark.getattr.
+ Object field =
+ x instanceof SkylarkClassObject
+ ? ((SkylarkClassObject) x).getValue(loc, thread.getSemantics(), name)
+ : ((ClassObject) x).getValue(name);
+ if (field != null) {
+ return Starlark.checkValid(field);
}
}
+ // @SkylarkCallable-annotated method?
if (method != null) {
- return new BuiltinCallable(object, name);
+ return new BuiltinCallable(x, name);
}
return null;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java
index 1532afe..c71b7a2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java
@@ -16,30 +16,17 @@
import com.google.devtools.build.lib.events.Location;
import javax.annotation.Nullable;
-/**
- * A marker interface for a {@link ClassObject} whose {@link #getValue} always returns a
- * Skylark-friendly value, with no defensive conversion required.
- *
- * <p>An value is Skylark-friendly if its class (or a supertype) implements {@link
- * com.google.devtools.build.lib.syntax.StarlarkValue}, is annotated with {@link
- * com.google.devtools.build.lib.skylarkinterface.SkylarkModule}, or is a Skylark primitive like
- * {@link String}.
- *
- * <p>Ideally, this class should not be needed, and all {@link ClassObject}s should only expose
- * Skylark-friendly values.
- */
+/** A variant of ClassObject for implementations that require a StarlarkSemantics. */
public interface SkylarkClassObject extends ClassObject {
/**
* Returns the value of the field with the given name, or null if the field does not exist.
*
- * @param loc the location in the current Starlark evaluation context
- * @param starlarkSemantics the current starlark semantics, which may affect which fields are
- * available, or the semantics of the available fields
- * @param name the name of the field to return the value of
- * @throws EvalException if a user-visible error occurs (other than non-existent field).
+ * @param loc the location of the field access operation
+ * @param semantics the Starlark semantics, which determine the available fields
+ * @param name the name of the field to retrieve
+ * @throws EvalException if the field exists could not be retrieved
*/
@Nullable
- Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name)
- throws EvalException;
+ Object getValue(Location loc, StarlarkSemantics semantics, String name) throws EvalException;
}
