Correctly type check Class<T>

[HT154]

This PR removes the unconditional type-erasure of Class<T> to Class.

This is a breaking change when T is a union type, nullable type, string literal type, parameterized type, or nothing; these will never type-check.

For convenience, erasure still occurs when T is unknown or Any. Similarly, the subclass check is skipped when T is an un-replaced type variable, e.g. in generic classes/methods (type aliases replace the type arg during instantiation).

Resolves #1729
Closes apple/pkl-intellij#222

[bioball]

General comment: given this:

typealias MyNumber = Number
foo: Class<MyNumber> = Number

This seems like an incorrect error, because MyNumber is just an alias for Number.

–– Pkl Error ––
`Class` type arguments may only be class types, module types, `unknown`, or `module`.

9 | foo: Class<MyNumber> = Number
         ^^^^^^^^^^^^^^^
at test (/Users/danielchao/code/apple/pkl/.dan-scripts/test.pkl:9)

Might also be okay to not throw for type arguments which don't point to classes. In these cases, the type would just never typecheck, and it would behave just like the nothing type. For example:

a: Class<Foo?>
b: Class<"foo" | "bar>

[HT154]

General comment: given this:

typealias MyNumber = Number
foo: Class<MyNumber> = Number

This seems like an incorrect error, because MyNumber is just an alias for Number.

Can you say more about how this would be useful? The only case I can think of is the compatibility shim typealias when a class is moved out into its own module.

If this was supported, it would only work for aliases to bare class types, which is of limited utility. An alias may hide that type is not an ordinary class and result in an error (as-is) or a nothing type (as you suggest below), especially if a library author later refactors an alias to include a constraint where one did not exist previously.

Might also be okay to not throw for type arguments which don't point to classes. In these cases, the type would just never typecheck, and it would behave just like the nothing type. For example:

a: Class<Foo?>
b: Class<"foo" | "bar>

Again, I think it's more surprising that users could inadvertently build a nothing type without realizing and have all their type checks silently fail. This would still be a breaking change compared to the existing behavior (where these are essentially erased to unknown) and I'd prefer to break in the direction of providing helpful user feedback than silent and potentially-surprising behavior.

[bioball]

Can you say more about how this would be useful? The only case I can think of is the compatibility shim typealias when a class is moved out into its own module.

An invariant of a typealias is that: on the type level, it just stands for its aliased type; therefore Class<MyNumber> should be the same as Class<Number>.

Again, I think it's more surprising that users could inadvertently build a nothing type without realizing and have all their type checks silently fail

I'd expect the failures to be loud, rather than silent! If you try to assign anything to Class<Foo?>, Pkl throws an exception. Also, I'd expect that any usage would also be caught statically (e.g. red squiggly lines in the editor during assignment). We can also add a warning diagnostic if you declare Class<Foo?> (or similar).

[HT154]

Updated this to unwrap typealiases during class calculation, so aliases (to aliases)+ to classes now behave as if the underlying class was used directly.

Also updated this to error at runtime rather than during ClassClassTypeNode init (or alias instantiation) with a hint that such type checks can never succeed. I think the errors are less helpful in cases where the Class type node or the type arg are behind aliases (since previously it relied on the same machinery as ReferenceTypeNode's constraint-in-referent checking). The only real difference other than error message content is that "bad" type annotations that never execute never raise errors. I can live with either approach, but I think the runtime errors aren't as good.

[RushikeshGandhmal]

While comparing #1763 with this PR, I noticed one case that may still need coverage: exported schemas. ClassClassTypeNode exposes the type argument through mirrors but does not override doExport(), so the inherited implementation appears to export Class<A> as raw Class.

In #1763 I added a doExport() implementation returning PType.Class(Class, typeArgument) and a regression test using Evaluator.evaluateSchema() to verify that Class<A> preserves A. I’d be happy to contribute that focused test/change here if useful.

[stackoverflow]

LGTM.
Worth adding the breaking change to the changelog already.

[bioball]

Overall, looks good! Some comments.

[bioball]

[nit] Move this to errorMessages.properties

Also, what does "a module" mean? This error message already contains module; duplicated by mistake?

[bioball]

I'm not sure if the SimpleClassTypeNode here is doing too much; TypeNode already has VmClass getVmClass().

We can keep the existing class hierarchy (only ClassTypeNode), and getViolatingNode()'s check can just be:

    @Override
    public @Nullable Node getViolatingNode() {
      // use the validation hook to recalculate clazz after typealias instantiation
      CompilerDirectives.transferToInterpreterAndInvalidate();

      var node = typeNode;
      while (node instanceof TypeAliasTypeNode typeAliasTypeNode) {
        node = typeAliasTypeNode.getAliasedTypeNode();
      }

      if (node instanceof UnknownTypeNode || node instanceof TypeVariableNode) {
        clazz = BaseModule.getAnyClass();
      } else {
        clazz = node.getVmClass();
      }
      return null;
    }

[bioball]

Class<T> is parameterized, so this should be:

Suggested change

	return consumer.accept(this);
	if (visitTypeArguments) {
	return consumer.accept(this) && typeNode.acceptTypeNode(true, consumer);
	}
	return consumer.accept(this);

[bioball]

Shouldn't need to extend ValidatingObjectSlotTypeNode; this can be:

  public abstract static class ClassClassTypeNode extends ObjectSlotTypeNode {

    @Child private TypeNode typeNode;
    @CompilationFinal private @Nullable VmClass clazz;

    public ClassClassTypeNode(SourceSection sourceSection, TypeNode typeNode) {
      super(sourceSection);
      this.typeNode = typeNode;
    }

    private void initVmClass() {
      if (clazz != null) {
        return;
      }
      CompilerDirectives.transferToInterpreterAndInvalidate();
      var node = typeNode;
      while (node instanceof TypeAliasTypeNode typeAliasTypeNode) {
        node = typeAliasTypeNode.getAliasedTypeNode();
      }

      if (node instanceof UnknownTypeNode || node instanceof TypeVariableNode) {
        clazz = BaseModule.getAnyClass();
      } else {
        clazz = node.getVmClass();
      }
    }

    @Specialization
    protected Object eval(VmClass value) {
      initVmClass();
      // rest of the method
    }
}

[bioball]

Then, we can get rid of all the overrides that don't do anything.

[bioball]

Since these are also changing other public API surface areas, can we add some tests around:

• reflect API giving type arguments for Class<T>
• Java schema evaluator giving metadata about type arguments

[bioball]

Let's also add a test for parenthesized types:

C is Class<(C)>
C is Class<((C))>