Properly gate dynamic type checking with flag

Previously f60a22e replaced `--experimental_starlark_types` with two separate flags, `--experimental_starlark_type_syntax` and `--experimental_starlark_type_checking`. However, it failed to wire the latter through to the actual type checking logic. This makes it impossible to process bzl files that contain type annotations without also attempting to check those types -- a no-no for Bazel 9.0, which should tolerate but ignore type annotations.

- Clarify type checking flag's description.
- Moved key constant for type checking flag from `BuildLanguageOptions` into `StarlarkSemantics`, since it is referenced within the Starlark interpreter itself.
- Rename `FileOptions#allowArbitraryTypeExpressions` -> `tolerateInvalidTypeExpressions` to make the intent clearer to the reader.
- Guard on flag in `Eval.java` and `StarlarkFunction.java`.
- Rename `TypeCheckTest` -> `DynamicTypeCheckTest` in anticipation of upcoming static type checking.

Fixes #27716.

PiperOrigin-RevId: 839269449
Change-Id: Ieb01c00a30758d7d486ef5ffdb03198fcff233ac

Author: brandjon

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -700,7 +700,8 @@ public final class BuildLanguageOptions extends OptionsBase {
       metadataTags = {OptionMetadataTag.EXPERIMENTAL},
       help =
           "Enables type checking in files and functions that contain type annotations or related "
-              + "syntax.")
+              + "syntax. (When this flag is disabled, Bazel is more forgiving of invalid types in "
+              + "type annotations.)")
   public boolean experimentalStarlarkTypeChecking;
 
   // TODO: b/350661266 - Delete this flag.
@@ -950,7 +951,9 @@ private void setFlags(FlagConsumer consumer) {
             .setBool(EXPERIMENTAL_RULE_EXTENSION_API, experimentalRuleExtensionApi)
             .setBool(EXPERIMENTAL_DORMANT_DEPS, experimentalDormantDeps)
             .setBool(EXPERIMENTAL_STARLARK_TYPE_SYNTAX, experimentalStarlarkTypeSyntax)
-            .setBool(EXPERIMENTAL_STARLARK_TYPE_CHECKING, experimentalStarlarkTypeChecking)
+            .setBool(
+                StarlarkSemantics.EXPERIMENTAL_STARLARK_TYPE_CHECKING,
+                experimentalStarlarkTypeChecking)
             .set(EXPERIMENTAL_STARLARK_TYPES_ALLOWED_PATHS, experimentalStarlarkTypesAllowedPaths)
             .setBool(INCOMPATIBLE_ENABLE_DEPRECATED_LABEL_APIS, enableDeprecatedLabelApis)
             .setBool(
@@ -1126,8 +1129,6 @@ public FlagConsumer setBool(String key, boolean ignored) {
 
   public static final String EXPERIMENTAL_STARLARK_TYPE_SYNTAX =
       FlagConstants.EXPERIMENTAL_STARLARK_TYPE_SYNTAX_FLAG_NAME;
-  public static final String EXPERIMENTAL_STARLARK_TYPE_CHECKING =
-      "-experimental_starlark_type_checking";
   public static final String INCOMPATIBLE_ENABLE_DEPRECATED_LABEL_APIS =
       "+incompatible_enable_deprecated_label_apis";
   public static final String INCOMPATIBLE_STOP_EXPORTING_BUILD_FILE_PATH =

src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java

@@ -212,8 +212,8 @@ static BzlCompileValue computeInline(
             .allowTypeSyntax(
                 semantics.getBool(BuildLanguageOptions.EXPERIMENTAL_STARLARK_TYPE_SYNTAX)
                     && typeSyntaxAllowlistMatchesPath)
-            .allowArbitraryTypeExpressions(
-                !semantics.getBool(BuildLanguageOptions.EXPERIMENTAL_STARLARK_TYPE_CHECKING))
+            .tolerateInvalidTypeExpressions(
+                !semantics.getBool(StarlarkSemantics.EXPERIMENTAL_STARLARK_TYPE_CHECKING))
             .build();
     StarlarkFile file = StarlarkFile.parse(input, options);
 

src/main/java/net/starlark/java/eval/Eval.java

@@ -171,6 +171,8 @@ private static StarlarkFunction newFunction(StarlarkThread.Frame fr, Resolver.Fu
     int nparams =
         rfn.getParameters().size() - (rfn.hasKwargs() ? 1 : 0) - (rfn.hasVarargs() ? 1 : 0);
     @Nullable CallableType functionType = rfn.getFunctionType();
+    boolean dynamicTypeCheckingEnabled =
+        fr.thread.getSemantics().getBool(StarlarkSemantics.EXPERIMENTAL_STARLARK_TYPE_CHECKING);
     for (int i = 0; i < nparams; i++) {
       Expression expr = rfn.getParameters().get(i).getDefaultValue();
       if (expr == null && defaults == null) {
@@ -182,7 +184,7 @@ private static StarlarkFunction newFunction(StarlarkThread.Frame fr, Resolver.Fu
       Object defaultValue = expr == null ? StarlarkFunction.MANDATORY : eval(fr, expr);
       defaults[i - (nparams - defaults.length)] = defaultValue;
 
-      if (functionType != null) {
+      if (dynamicTypeCheckingEnabled && functionType != null) {
         // Typecheck the default value
         StarlarkType parameterType = functionType.getParameterTypeByPos(i);
         if (!TypeChecker.isValueSubtypeOf(defaultValue, parameterType)) {

src/main/java/net/starlark/java/eval/StarlarkFunction.java

@@ -537,9 +537,14 @@ public Object call(StarlarkThread thread) throws EvalException, InterruptedExcep
             kwargs == null ? Dict.of(thread.mutability()) : Dict.wrap(thread.mutability(), kwargs);
       }
 
-      // Runtime type check
-      StarlarkType type = owner.getStarlarkType();
-      if (type instanceof Types.CallableType functionType) {
+      Types.CallableType functionType =
+          thread.getSemantics().getBool(StarlarkSemantics.EXPERIMENTAL_STARLARK_TYPE_CHECKING)
+                  && owner.getStarlarkType() instanceof Types.CallableType
+              ? (Types.CallableType) owner.getStarlarkType()
+              : null;
+
+      // Argument value dynamic type check, if enabled.
+      if (functionType != null) {
         for (int i = 0; i < functionType.getParameterTypes().size(); i++) {
           if (locals[i] == null) {
             continue; // the default value is already type checked
@@ -572,8 +577,8 @@ public Object call(StarlarkThread thread) throws EvalException, InterruptedExcep
       fr.locals = locals;
       Object returnValue = Eval.execFunctionBody(fr, rfn.getBody());
 
-      // Return value check
-      if (type instanceof Types.CallableType functionType) {
+      // Return value dynamic type check, if enabled.
+      if (functionType != null) {
         if (!TypeChecker.isValueSubtypeOf(returnValue, functionType.getReturnType())) {
           throw Starlark.errorf(
               "%s(): returns value of type '%s', declares '%s'",

src/main/java/net/starlark/java/eval/StarlarkSemantics.java

@@ -267,6 +267,11 @@ public final String toString() {
   public static final String INTERNAL_BAZEL_ONLY_UTF_8_BYTE_STRINGS =
       "-internal_bazel_only_utf_8_byte_strings";
 
+  /** Whether (static and/or dynamic) type checking should be performed. */
+  // TODO: #27370 - Consider splitting this into separate options for static vs dynamic.
+  public static final String EXPERIMENTAL_STARLARK_TYPE_CHECKING =
+      "-experimental_starlark_type_checking";
+
   /** Globally Override fail(stack_trace=) to true. Flag default is false. */
   public static final String FORCE_STARLARK_STACK_TRACE = "-force_starlark_stack_trace";
 }

src/main/java/net/starlark/java/syntax/FileOptions.java

@@ -82,15 +82,16 @@ public abstract class FileOptions {
   public abstract boolean allowTypeSyntax();
 
   /**
-   * If true, type expressions may be any valid expression except for unparenthesized tuples.
-   * Otherwise type expressions must represent a valid type.
+   * If true, type expressions in annotations and {@code type} declarations may be any valid
+   * expression (except for unparenthesized tuples, which are grammatically ambiguous). Otherwise
+   * type expressions must represent a valid type.
    *
    * <p>Enabling this boolean is helpful for backwards compatibility, but results in an AST that is
    * not usable for type checking.
    *
    * <p>This has no effect if {@link #allowTypeSyntax} is false.
    */
-  public abstract boolean allowArbitraryTypeExpressions();
+  public abstract boolean tolerateInvalidTypeExpressions();
 
   public static Builder builder() {
     // These are the DEFAULT values.
@@ -101,7 +102,7 @@ public static Builder builder() {
         .requireLoadStatementsFirst(true)
         .stringLiteralsAreAsciiOnly(false)
         .allowTypeSyntax(false)
-        .allowArbitraryTypeExpressions(false);
+        .tolerateInvalidTypeExpressions(false);
   }
 
   public abstract Builder toBuilder();
@@ -122,7 +123,7 @@ public abstract static class Builder {
 
     public abstract Builder allowTypeSyntax(boolean value);
 
-    public abstract Builder allowArbitraryTypeExpressions(boolean value);
+    public abstract Builder tolerateInvalidTypeExpressions(boolean value);
 
     public abstract FileOptions build();
   }

src/main/java/net/starlark/java/syntax/Parser.java

@@ -1089,7 +1089,7 @@ private Expression maybeParseTypeAnnotationAfter(TokenKind expectedToken) {
   private Expression parseTypeExprWithFallback() {
     Expression result;
     this.insideTypeExpr = true;
-    if (options.allowArbitraryTypeExpressions()) {
+    if (options.tolerateInvalidTypeExpressions()) {
       // parseTest, because allowing unparenthesized tuples here would consume subsequent params in
       // function signatures.
       result = parseTest();

src/test/java/net/starlark/java/eval/BUILD

@@ -13,6 +13,7 @@ filegroup(
 java_test(
     name = "EvalTests",
     srcs = [
+        "DynamicTypeCheckTest.java",
         "EvalTests.java",  # (suite)
         "EvalUtilsTest.java",
         "EvaluationTest.java",
@@ -32,7 +33,6 @@ java_test(
         "StarlarkThreadDebuggingTest.java",
         "StarlarkThreadTest.java",
         "SymbolGeneratorTest.java",
-        "TypeCheckTest.java",
     ],
     jvm_flags = [
         "-Dfile.encoding=UTF8",

…et/starlark/java/eval/TypeCheckTest.java …lark/java/eval/DynamicTypeCheckTest.javasrc/test/java/net/starlark/java/eval/TypeCheckTest.java renamed to src/test/java/net/starlark/java/eval/DynamicTypeCheckTest.java src/test/java/net/starlark/java/eval/TypeCheckTest.java renamed to src/test/java/net/starlark/java/eval/DynamicTypeCheckTest.java

@@ -28,16 +28,30 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/** Tests for Starlark types. */
+/** Tests for Starlark type checking at evaluation time. */
 @RunWith(JUnit4.class)
-public class TypeCheckTest {
+public class DynamicTypeCheckTest {
 
   private EvaluationTestCase ev;
 
   @Before
   public void setup() {
     ev = new EvaluationTestCase();
     ev.setFileOptions(FileOptions.builder().allowTypeSyntax(true).build());
+    ev.setSemantics(
+        StarlarkSemantics.builder()
+            .setBool(StarlarkSemantics.EXPERIMENTAL_STARLARK_TYPE_CHECKING, true)
+            .build());
+  }
+
+  @Test
+  public void typechecking_disabledByFlag() throws Exception {
+    ev.setSemantics(
+        StarlarkSemantics.builder()
+            .setBool(StarlarkSemantics.EXPERIMENTAL_STARLARK_TYPE_CHECKING, false)
+            .build());
+
+    ev.exec("def f(a : int): pass", "f('abc')");
   }
 
   @Test

src/test/java/net/starlark/java/eval/EvalTests.java

@@ -19,6 +19,7 @@
 /** EvalTests tests the Starlark evaluator. */
 @RunWith(Suite.class)
 @Suite.SuiteClasses({
+  DynamicTypeCheckTest.class,
   EvaluationTest.class,
   EvalUtilsTest.class,
   FunctionTest.class,
@@ -34,6 +35,5 @@
   StarlarkMutableTest.class,
   StarlarkThreadDebuggingTest.class,
   StarlarkThreadTest.class,
-  TypeCheckTest.class
 })
 public class EvalTests {}