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;
 }