Make auto-generated types per-StarlarkBuiltin and independent of StarlarkSemantics The "per-StarlarkBuiltin" part fixes the problem that different subclasses of a StarlarkBuiltin had distinct types; we obviously want them to share a StarlarkType - since the StarlarkBuiltin annotation fully defines the value's eval-time interface for the interpreter. (Naturally, this also means renaming ClassStarlarkType to StarlarkBuiltinAutoType.) The "independent of StarlarkSemantics" part fixes the problem of incompatible types for the same class generated under different semantics, which formerly could happen e.g. when using the 2-argument Starlark.addMethods(), which forces default semantics. Working towards #27370 PiperOrigin-RevId: 952935466 Change-Id: Ie1a3277d53c6376d23c0bc3ce145b4ff64978500
diff --git a/src/main/java/net/starlark/java/eval/CallUtils.java b/src/main/java/net/starlark/java/eval/CallUtils.java index 1e64148..2f812d1 100644 --- a/src/main/java/net/starlark/java/eval/CallUtils.java +++ b/src/main/java/net/starlark/java/eval/CallUtils.java
@@ -126,30 +126,29 @@ @Nullable TypeConstructor typeConstructor; /** - * The value of {@link ClassStarlarkType#getSupertypes} for this class's {@link - * ClassStarlarkType} if it exists (i.e. if the {@link ClassStarlarkType} is non-null); or null - * otherwise. + * The value of {@link StarlarkBuiltinAutoType#getSupertypes} for this class's {@link + * StarlarkBuiltinAutoType} if it exists (i.e. if the {@link StarlarkBuiltinAutoType} is + * non-null); or null otherwise. * - * <p>Needs to be stored outside the {@link ClassStarlarkType} to avoid circular dependencies - * between a {@link ClassStarlarkType} and its methods' args/returns types. + * <p>Needs to be stored outside the {@link StarlarkBuiltinAutoType} to avoid circular + * dependencies between a {@link StarlarkBuiltinAutoType} and its methods' args/returns types. */ - @Nullable ImmutableList<StarlarkType> classStarlarkTypeSupertypes; + @Nullable ImmutableList<StarlarkType> starlarkBuiltinAutoTypeSupertypes; } - private static final class ClassStarlarkType extends StarlarkType { + private static final class StarlarkBuiltinAutoType extends StarlarkType { + // Invariant: a StarlarkBuiltinAutoType must not contain any pointer path to a ClassDescriptor. private final String name; private final Class<?> clazz; - private final BuiltinManager manager; - private ClassStarlarkType(String name, Class<?> clazz, BuiltinManager manager) { - this.name = name; + private StarlarkBuiltinAutoType(Class<?> clazz) { + this.name = StarlarkAnnotations.getStarlarkBuiltin(clazz).name(); this.clazz = clazz; - this.manager = manager; } // TODO: #28325 - Populate supertypes where possible. If a class implements eval.Sequence, its - // ClassStarlarkType should have `Sequence` as a supertype. The StarlarkType hierarchy should be - // compatible with the Java inheritance hierarchy. + // StarlarkBuiltinAutoType should have `Sequence` as a supertype. The StarlarkType hierarchy + // should be compatible with the Java inheritance hierarchy. static ImmutableList<StarlarkType> buildSupertypes( Class<?> clazz, ClassDescriptor classDescriptor) { ImmutableList.Builder<StarlarkType> builder = ImmutableList.builder(); @@ -179,14 +178,13 @@ @Override public ImmutableList<StarlarkType> getSupertypes(TypeContext context) { - return checkNotNull(manager.getClassDescriptor(clazz).classStarlarkTypeSupertypes); + return checkNotNull(context.getStarlarkBuiltinAutoTypeSupertypes(clazz)); } @Override @Nullable public StarlarkType getField(String name, TypeContext context) { - @Nullable MethodDescriptor method = manager.getClassDescriptor(clazz).methods.get(name); - return method == null ? null : method.getStarlarkType(); + return context.getStarlarkBuiltinFieldType(clazz, name); } @Override @@ -195,6 +193,42 @@ } } + private static final ClassValue<StarlarkType> starlarkBuiltinAutoTypeCache = + new ClassValue<StarlarkType>() { + @Override + @Nullable + protected StarlarkType computeValue(Class<?> clazz) { + Class<?> parentWithStarlarkBuiltin = + StarlarkAnnotations.getParentWithStarlarkBuiltin(clazz); + if (parentWithStarlarkBuiltin == null) { + // Not annotated as @StarlarkBuiltin - treat as Object. + return Types.OBJECT; + } else if (parentWithStarlarkBuiltin != clazz) { + // Subclasses of a @StarlarkBuiltin class share the same auto-generated type. + return starlarkBuiltinAutoTypeCache.get(parentWithStarlarkBuiltin); + } + + @Nullable StarlarkType fixedStarlarkType = getFixedStarlarkType(clazz); + if (fixedStarlarkType != null) { + return fixedStarlarkType; + } + if (!wantStarlarkBuiltinAutoType(clazz)) { + return null; + } + return new StarlarkBuiltinAutoType(clazz); + } + }; + + /** + * Returns the Starlark type to be used for valid Starlark values of the given class which doesn't + * override {@link StarlarkValue#getStarlarkType}; or null if it (or one of its superclasses) does + * override {@link StarlarkValue#getStarlarkType}. + */ + @Nullable + static StarlarkType getStarlarkBuiltinAutoType(Class<?> clazz) { + return starlarkBuiltinAutoTypeCache.get(clazz); + } + /** * A manager for obtaining descriptors for native-defined Starlark objects and methods, under a * specific {@code StarlarkSemantics}. @@ -217,41 +251,10 @@ } }; - private final ClassValue<StarlarkType> classStarlarkTypeCache = - new ClassValue<StarlarkType>() { - @Override - @Nullable - protected StarlarkType computeValue(Class<?> clazz) { - @Nullable StarlarkType fixedStarlarkType = getFixedStarlarkType(clazz); - if (fixedStarlarkType != null) { - return fixedStarlarkType; - } - if (!wantClassStarlarkType(clazz)) { - return null; - } - // TODO: #28325 - Use a superclass's/interface's ClassStarlarkType where it makes sense. - // In particular, classes sharing the same most-proximate StarlarkBuiltin and the same - // set of methods should share the same ClassStarlarkType. - @Nullable StarlarkBuiltin annotation = StarlarkAnnotations.getStarlarkBuiltin(clazz); - String typeName = annotation != null ? annotation.name() : clazz.getSimpleName(); - return new ClassStarlarkType(typeName, clazz, BuiltinManager.this); - } - }; - private BuiltinManager(StarlarkSemantics semantics) { this.semantics = semantics; } - /** - * Returns the Starlark type to be used for valid Starlark values of the given class which - * doesn't override {@link StarlarkValue#getStarlarkType}; or null if it (or one of its - * superclasses) does override {@link StarlarkValue#getStarlarkType}. - */ - @Nullable - StarlarkType getClassStarlarkType(Class<?> clazz) { - return classStarlarkTypeCache.get(clazz); - } - StarlarkSemantics getSemantics() { return semantics; } @@ -291,6 +294,15 @@ } /** + * Returns the supertypes of the generated Starlark type associated with the given Java class, + * or null if no such generated type exists. + */ + @Nullable + ImmutableList<StarlarkType> getStarlarkBuiltinAutoTypeSupertypes(Class<?> clazz) { + return getClassDescriptor(clazz).starlarkBuiltinAutoTypeSupertypes; + } + + /** * Returns a {@link MethodDescriptor} object representing a function which calls the selfCall * java method of the given object (the {@link StarlarkMethod} method with {@link * StarlarkMethod#selfCall()} set to true). Returns null if no such method exists. @@ -318,7 +330,7 @@ MethodDescriptor selfCall = null; LinkedHashMap<String, MethodDescriptor> methods = new LinkedHashMap<>(); - TypeConstructor typeConstructor = getAssociatedTypeConstructor(clazz); + TypeConstructor associatedTypeConstructor = getAssociatedTypeConstructor(clazz); // Sort non-synthetic methods ahead of synthetic ones, then by Java name for determinism. A // public method inherited from a non-public superclass is exposed by Class.getMethods() only as @@ -366,27 +378,29 @@ classDescriptor.manager = manager; classDescriptor.selfCall = selfCall; classDescriptor.methods = ImmutableMap.copyOf(methods); - classDescriptor.typeConstructor = typeConstructor; - if (getFixedStarlarkType(clazz) == null && wantClassStarlarkType(clazz)) { - classDescriptor.classStarlarkTypeSupertypes = - ClassStarlarkType.buildSupertypes(clazz, classDescriptor); + classDescriptor.typeConstructor = associatedTypeConstructor; + if (getFixedStarlarkType(clazz) == null && wantStarlarkBuiltinAutoType(clazz)) { + if (classDescriptor.typeConstructor == null) { + classDescriptor.typeConstructor = + Types.wrapType( + StarlarkAnnotations.getStarlarkBuiltin(clazz).name(), + () -> starlarkBuiltinAutoTypeCache.get(clazz)); + } + classDescriptor.starlarkBuiltinAutoTypeSupertypes = + StarlarkBuiltinAutoType.buildSupertypes(clazz, classDescriptor); } return classDescriptor; } /** - * Returns true if a {@link ClassStarlarkType} should be generated for the given class. This is - * the case if the class is a {@link StarlarkValue} and does not override {@link + * Returns true if a {@link StarlarkBuiltinAutoType} should be generated for the given class. This + * is the case if the class is annotated as {@link StarlarkBuiltin}, and does not override {@link * StarlarkValue#getStarlarkType}. */ - private static boolean wantClassStarlarkType(Class<?> clazz) { - if (!StarlarkValue.class.isAssignableFrom(clazz)) { + private static boolean wantStarlarkBuiltinAutoType(Class<?> clazz) { + if (StarlarkAnnotations.getStarlarkBuiltin(clazz) == null) { return false; } - @Nullable StarlarkBuiltin annotation = StarlarkAnnotations.getStarlarkBuiltin(clazz); - if (annotation == null) { - return true; - } Method getter; try { // LINT.IfChange @@ -403,8 +417,8 @@ /** * Certain Java classes/interfaces should be associated with a special fixed {@link StarlarkType} - * instead of a generated {@link ClassStarlarkType}. Returns that fixed {@link StarlarkType}, or - * null otherwise. + * instead of a generated {@link StarlarkBuiltinAutoType}. Returns that fixed {@link + * StarlarkType}, or null otherwise. */ @Nullable private static StarlarkType getFixedStarlarkType(Class<?> clazz) {
diff --git a/src/main/java/net/starlark/java/eval/MethodDescriptor.java b/src/main/java/net/starlark/java/eval/MethodDescriptor.java index 3957c89..d15a36a 100644 --- a/src/main/java/net/starlark/java/eval/MethodDescriptor.java +++ b/src/main/java/net/starlark/java/eval/MethodDescriptor.java
@@ -305,7 +305,7 @@ return Types.OBJECT; } else { if (cls instanceof Class<?> c) { - @Nullable StarlarkType classStarlarkType = manager.getClassStarlarkType(c); + @Nullable StarlarkType classStarlarkType = CallUtils.getStarlarkBuiltinAutoType(c); if (classStarlarkType != null) { return classStarlarkType; }
diff --git a/src/main/java/net/starlark/java/eval/Module.java b/src/main/java/net/starlark/java/eval/Module.java index 74cd4c0..d0771ec 100644 --- a/src/main/java/net/starlark/java/eval/Module.java +++ b/src/main/java/net/starlark/java/eval/Module.java
@@ -18,6 +18,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import java.util.Arrays; @@ -27,6 +28,7 @@ import java.util.Map; import java.util.Set; import javax.annotation.Nullable; +import net.starlark.java.annot.StarlarkAnnotations; import net.starlark.java.syntax.Resolver; import net.starlark.java.syntax.StarlarkType; import net.starlark.java.syntax.TypeConstructor; @@ -323,6 +325,23 @@ return desc == null ? null : desc.getStarlarkType(); } + @Override + @Nullable + public StarlarkType getStarlarkBuiltinFieldType(Class<?> clazz, String fieldName) { + if (StarlarkAnnotations.getStarlarkBuiltin(clazz) == null) { + // Support only @StarlarkBuiltin annotated classes, not @StarlarkLibrary ones. + return null; + } + MethodDescriptor desc = getMethods(clazz).get(fieldName); + return desc == null ? null : desc.getStarlarkType(); + } + + @Override + @Nullable + public ImmutableList<StarlarkType> getStarlarkBuiltinAutoTypeSupertypes(Class<?> clazz) { + return CallUtils.getBuiltinManager(semantics).getStarlarkBuiltinAutoTypeSupertypes(clazz); + } + /** * Returns the value of the specified global variable, or null if not bound. Does not look in the * predeclared environment.
diff --git a/src/main/java/net/starlark/java/eval/Starlark.java b/src/main/java/net/starlark/java/eval/Starlark.java index 25e3586..0b427e2 100644 --- a/src/main/java/net/starlark/java/eval/Starlark.java +++ b/src/main/java/net/starlark/java/eval/Starlark.java
@@ -340,7 +340,7 @@ case StarlarkValue x -> { @Nullable StarlarkType type = x.getStarlarkType(semantics); if (type == null) { - type = CallUtils.getBuiltinManager(semantics).getClassStarlarkType(value.getClass()); + type = CallUtils.getStarlarkBuiltinAutoType(value.getClass()); } yield type != null ? type : Types.ANY; }
diff --git a/src/main/java/net/starlark/java/syntax/TypeContext.java b/src/main/java/net/starlark/java/syntax/TypeContext.java index 1c8ae52..0bdc7f4 100644 --- a/src/main/java/net/starlark/java/syntax/TypeContext.java +++ b/src/main/java/net/starlark/java/syntax/TypeContext.java
@@ -14,6 +14,7 @@ package net.starlark.java.syntax; +import com.google.common.collect.ImmutableList; import javax.annotation.Nullable; /** @@ -23,7 +24,7 @@ * syntax/} package, e.g. the method APIs of {@link StarlarkList}. */ public interface TypeContext { - + /** Returns the type of the given field of a {@code str} type, or null if no such field exists. */ @Nullable StarlarkType getStrFieldType(String name); @@ -48,6 +49,26 @@ StarlarkType getSetFieldType(String name); /** + * Returns the type of the given field of a {@link net.starlark.java.annot.StarlarkBuiltin} + * annotated class (or a subclass of one), or null if the class is not a @StarlarkBuiltin or has + * no such field. + */ + @Nullable + default StarlarkType getStarlarkBuiltinFieldType(Class<?> clazz, String fieldName) { + return null; + } + + /** + * Returns the supertypes of the auto-generated Starlark type associated with the given {@link + * net.starlark.java.annot.StarlarkBuiltin} annotated class (or a subclass of one), or null if no + * such auto-generated type exists. + */ + @Nullable + default ImmutableList<StarlarkType> getStarlarkBuiltinAutoTypeSupertypes(Class<?> clazz) { + return null; + } + + /** * Returns the value type of a {@link Resolver.Scope#PREDECLARED} symbol, or null if there is no * such symbol. */
diff --git a/src/main/java/net/starlark/java/syntax/Types.java b/src/main/java/net/starlark/java/syntax/Types.java index a3266a1..0672528 100644 --- a/src/main/java/net/starlark/java/syntax/Types.java +++ b/src/main/java/net/starlark/java/syntax/Types.java
@@ -29,6 +29,7 @@ import java.util.Map; import java.util.function.BiFunction; import java.util.function.Function; +import java.util.function.Supplier; import javax.annotation.Nullable; /** @@ -1520,6 +1521,16 @@ }; } + public static TypeConstructor.AllowingNullary wrapType( + String name, Supplier<StarlarkType> typeSupplier) { + return argsTuple -> { + if (!argsTuple.isEmpty()) { + throw new TypeConstructor.Failure(String.format("'%s' does not accept arguments", name)); + } + return typeSupplier.get(); + }; + } + static ImmutableList<StarlarkType> toStarlarkTypes( String name, ImmutableList<TypeConstructor.Term> args) throws TypeConstructor.Failure { for (TypeConstructor.Term arg : args) { @@ -1616,6 +1627,7 @@ private static final TypeConstructor.AllowingNullary wrapStructConstructor() { return args -> { if (args.isEmpty()) { + // `struct` is equivalent to `struct[{}, ...]` return ANY_STRUCT; } else if (args.size() <= 2) { TypeConstructor.Term arg = args.getFirst();
diff --git a/src/test/java/net/starlark/java/eval/StaticTypeCheckTest.java b/src/test/java/net/starlark/java/eval/StaticTypeCheckTest.java index fda4894..c81a96d 100644 --- a/src/test/java/net/starlark/java/eval/StaticTypeCheckTest.java +++ b/src/test/java/net/starlark/java/eval/StaticTypeCheckTest.java
@@ -187,6 +187,12 @@ @StarlarkBuiltin(name = "MissingStaticMethodTypeBuiltin") public static final class MissingStaticMethodTypeBuiltin implements StarlarkValue { // no getAssociatedTypeConstructor() + + // Override ensures that we don't generate a StarlarkBuiltinAutoType for this class. + @Override + public StarlarkType getStarlarkType(StarlarkSemantics semantics) { + throw new UnsupportedOperationException("fail"); + } } @StarlarkLibrary @@ -342,13 +348,15 @@ } @StarlarkBuiltin(name = "MyType") - public static final class MyType implements StarlarkValue { + public static sealed class MyType implements StarlarkValue permits MyTypeSubclass { @StarlarkMethod(name = "foo", doc = "...") public int foo() { return 123; } } + public static final class MyTypeSubclass extends MyType {} + @StarlarkBuiltin(name = "MySelfCallType") public static final class MySelfCallType implements StarlarkValue { @StarlarkMethod(name = "MySelfCallType", doc = "...", selfCall = true) @@ -411,29 +419,27 @@ Module.withPredeclared( StarlarkSemantics.DEFAULT, ImmutableMap.of( - "my_unannotated_type_value", - new MyUnannotatedType(), "my_type_value", new MyType(), + "my_type_subclass_value", + new MyTypeSubclass(), "my_self_call_value", new MySelfCallType(), "my_explicitly_typed_value", new MyExplicitlyTypedType(), "my_explicitly_typed_self_call_value", new MyExplicitlyTypedSelfCallType())); + assertValid( """ - a: int = my_unannotated_type_value.foo() - b: int = my_type_value.foo() + a: int = my_type_value.foo() + b: int = my_type_subclass_value.foo() c: int = my_self_call_value() d: int = my_self_call_value.bar() e: int = my_explicitly_typed_value.some_field # typed as struct-of-Any f: int = my_explicitly_typed_self_call_value() """); - assertInvalid( - "cannot assign type 'MyUnannotatedType' to 'x' of type 'str'", - "x: str = my_unannotated_type_value"); assertInvalid("cannot assign type 'MyType' to 'x' of type 'str'", "x: str = my_type_value"); assertInvalid( "cannot assign type 'MySelfCallType' to 'x' of type 'str'", "x: str = my_self_call_value"); @@ -453,6 +459,37 @@ assertInvalid("'my_type_value' is not callable; got type 'MyType'", "_: str = my_type_value()"); } + @Test + public void unannotatedStarlarkValues_notAutoTyped() throws Exception { + module = + Module.withPredeclared( + StarlarkSemantics.DEFAULT, + ImmutableMap.of("unannotated_value", new MyUnannotatedType())); + // MyUnannotatedType has no @StarlarkBuiltin-annotated ancestor, so there's no + // StarlarkBuiltinAutoType generated for it; therefore, unannotated_value is typed as Object. + assertInvalid("cannot assign type 'object' to 'x' of type 'str'", "x: str = unannotated_value"); + } + + @Test + public void subclassesShareSameAutoType() throws Exception { + module = + Module.withPredeclared( + StarlarkSemantics.DEFAULT, + ImmutableMap.of( + "my_type_value", new MyType(), "my_type_subclass_value", new MyTypeSubclass())); + assertValid( + """ + _: None # ensure file uses type syntax + list_a = [my_type_value] # inferred as list[MyType] + list_a[0] = my_type_subclass_value + list_b = [my_type_subclass_value] # also inferred as list[MyType] + list_b[0] = my_type_value + """); + + assertInvalid( + "cannot assign type 'MyType' to 'x' of type 'str'", "x: str = my_type_subclass_value"); + } + @StarlarkBuiltin(name = "SelfReferentialType") public static final class SelfReferentialType implements StarlarkValue { @StarlarkMethod(