Enforce that abstract methods are implemented (#1785)

This adds a check that abstract members must be implemented.
If any members lack an implementation, an error is thrown describing
the missing members.

Co-authored-by: Dan Chao <dan.chao@apple.com>

pkl-core/src/main/java/org/pkl/core/ast/member/ClassNode.java

@@ -123,37 +123,45 @@ public final class ClassNode extends ExpressionNode {
             typeParameters,
             prototype);
 
-    if (unresolvedSupertypeNode != null) {
+    var localContext = VmLanguage.get(this).localContext.get();
-      var supertypeNode = unresolvedSupertypeNode.execute(frame);
+    localContext.beginClassInit(cachedClass);
-      var superclass = supertypeNode.getVmClass();
 
-      checkSupertype(supertypeNode, superclass);
+    try {
-      cachedClass.initSupertype(supertypeNode, superclass);
+      if (unresolvedSupertypeNode != null) {
+        var supertypeNode = unresolvedSupertypeNode.execute(frame);
+        var superclass = supertypeNode.getVmClass();
+
+        checkSupertype(supertypeNode, superclass);
+        cachedClass.initSupertype(supertypeNode, superclass);
+      }
+
+      // The superclass resolved above may not itself have completed the below initializations yet.
+      // That's because these initializations may have indirectly or directly triggered
+      // resolution of this class, in which case the `resolveSuperclass()` call above
+      // will have returned the partially initialized `cachedClass` of the superclass.
+      // As a consequence, initializations that require a fully initialized class hierarchy
+      // are done lazily in VmClass rather than here.
+      // A fully initialized class hierarchy is only required for initialization of internal caches,
+      // which is guaranteed to succeed (no impact on eager vs. lazy error reporting) and easy to
+      // defer.
+
+      VmUtils.evaluateAnnotations(frame, annotationNodes, annotations);
+
+      for (var node : unresolvedPropertyNodes) {
+        cachedClass.addProperty(node.execute(frame, cachedClass));
+      }
+
+      for (var node : unresolvedMethodNodes) {
+        cachedClass.addMethod(node.execute(frame, cachedClass));
+      }
+
+      cachedClass.onOwnClassInitialized();
+      localContext.endClassInit();
+      return cachedClass;
+    } catch (Throwable e) {
+      localContext.clearClassInitState();
+      throw e;
     }
-
-    // The superclass resolved above may not itself have completed the below initializations yet.
-    // That's because these initializations may have indirectly or directly triggered
-    // resolution of this class, in which case the `resolveSuperclass()` call above
-    // will have returned the partially initialized `cachedClass` of the superclass.
-    // As a consequence, initializations that require a fully initialized class hierarchy
-    // are done lazily in VmClass rather than here.
-    // A fully initialized class hierarchy is only required for initialization of internal caches,
-    // which is guaranteed to succeed (no impact on eager vs. lazy error reporting) and easy to
-    // defer.
-
-    VmUtils.evaluateAnnotations(frame, annotationNodes, annotations);
-
-    for (var node : unresolvedPropertyNodes) {
-      cachedClass.addProperty(node.execute(frame, cachedClass));
-    }
-
-    for (var node : unresolvedMethodNodes) {
-      cachedClass.addMethod(node.execute(frame, cachedClass));
-    }
-
-    cachedClass.notifyInitialized();
-
-    return cachedClass;
   }
 
   private void checkSupertype(TypeNode supertypeNode, @Nullable VmClass superclass) {

pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java

@@ -33,6 +33,7 @@ import org.pkl.core.TypeParameter;
 import org.pkl.core.ast.*;
 import org.pkl.core.ast.member.*;
 import org.pkl.core.ast.type.TypeNode;
+import org.pkl.core.runtime.VmExceptionBuilder.MultilineValue;
 import org.pkl.core.util.CollectionUtils;
 import org.pkl.core.util.EconomicMaps;
 import org.pkl.core.util.LateInit;
@@ -150,6 +151,45 @@ public final class VmClass extends VmValue {
     prototype.lateInitParent(superclass.getPrototype());
   }
 
+  @TruffleBoundary
+  private void checkAbstractMethods() {
+    if (isAbstract()) return;
+    // minimize allocations in the non-error case
+    var abstractMethods = getAbstractMethods();
+    if (abstractMethods.isEmpty()) return;
+    if (abstractMethods.size() == 1) {
+      throw new VmExceptionBuilder()
+          .evalError(
+              "noImplementationForAbstractMethod",
+              getDisplayName(),
+              abstractMethods.get(0).getCallSignature())
+          .withSourceSection(getHeaderSection())
+          .build();
+    }
+    var methodList = new ArrayList<String>(abstractMethods.size());
+    for (var method : abstractMethods) {
+      methodList.add(method.getCallSignature());
+    }
+    throw new VmExceptionBuilder()
+        .evalError(
+            "noImplementationForAbstractMethods", getDisplayName(), MultilineValue.of(methodList))
+        .withSourceSection(getHeaderSection())
+        .build();
+  }
+
+  private List<ClassMethod> getAbstractMethods() {
+    assert this.superclass != null;
+    var result = new ArrayList<ClassMethod>();
+    var methodCursor = getAllMethods().getEntries();
+    while (methodCursor.advance()) {
+      var method = methodCursor.getValue();
+      if (method.isAbstract()) {
+        result.add(method);
+      }
+    }
+    return result;
+  }
+
   @TruffleBoundary
   public void addProperty(ClassProperty property) {
     prototype.addProperty(property.getInitializer());
@@ -190,11 +230,20 @@ public final class VmClass extends VmValue {
     }
   }
 
-  // Note: Superclasses may not have finished their initialization when this method is called.
+  /**
-  public void notifyInitialized() {
+   * Called when this class itself has been initialized.
+   *
+   * <p>Superclasses may not have been initialized yet.
+   */
+  public void onOwnClassInitialized() {
     isInitialized = true;
   }
 
+  /** Called when the entire class hierarchy is completely initialized, including superclasses. */
+  public void onFullyInitialized() {
+    checkAbstractMethods();
+  }
+
   public int getTypeParameterCount() {
     return typeParameters.size();
   }

pkl-core/src/main/java/org/pkl/core/runtime/VmLocalContext.java

@@ -15,6 +15,9 @@
  */
 package org.pkl.core.runtime;
 
+import java.util.ArrayDeque;
+import java.util.Deque;
+
 /** A per-context thread-local value that can be used to influence execution. */
 public class VmLocalContext {
   private boolean shouldEagerTypecheck = false;
@@ -22,6 +25,12 @@ public class VmLocalContext {
   /** Whether we are currently inside a type test ({@code is} check). */
   private boolean inTypeTest = false;
 
+  /** The number of classes currently being initialized. */
+  private int classDepth = 0;
+
+  /** The classes currently being initialized. */
+  private final Deque<VmClass> pendingClasses = new ArrayDeque<>();
+
   /**
    * Number of active {@link VmValueTracker} instances. Used to determine if instrumentation is
    * already active.
@@ -48,6 +57,27 @@ public class VmLocalContext {
     return inTypeTest;
   }
 
+  public void beginClassInit(VmClass vmClass) {
+    classDepth++;
+    pendingClasses.add(vmClass);
+  }
+
+  public void endClassInit() {
+    classDepth--;
+    if (classDepth > 0) {
+      return;
+    }
+    while (!pendingClasses.isEmpty()) {
+      var clazz = pendingClasses.pop();
+      clazz.onFullyInitialized();
+    }
+  }
+
+  public void clearClassInitState() {
+    pendingClasses.clear();
+    classDepth = 0;
+  }
+
   public void enterTracker() {
     activeTrackerDepth++;
     instrumentationEverUsed = true;

pkl-core/src/main/resources/org/pkl/core/errorMessages.properties

@@ -1208,3 +1208,10 @@ invalidReferenceTypeAnnotationWithConstraint=\
 
 cannotInstallPackageWithNoCache=\
 Cannot install package to module cache dir when module cache is disabled.
+
+noImplementationForAbstractMethod=\
+Class `{0}` should either be declared `abstract`, or should implement method `{1}`.
+
+noImplementationForAbstractMethods=\
+Class `{0}` should either be declared `abstract`, or should implement the following methods:\n\
+{1}

pkl-core/src/test/files/LanguageSnippetTests/input-helper/classes/AbstractModule.pkl

@@ -0,0 +1,3 @@
+abstract module Foo
+
+abstract function bar(): Int

pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented1.pkl

@@ -0,0 +1,8 @@
+abstract class AbstractMethod {
+  abstract function foo(): Int
+}
+
+class MyClass extends AbstractMethod {
+}
+
+foo: MyClass

pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented2.pkl

@@ -0,0 +1,9 @@
+abstract class AbstractMethods {
+  abstract function foo(): Int
+  abstract function bar(): Int
+}
+
+class MyClass extends AbstractMethods {
+}
+
+foo: MyClass

pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented3.pkl

@@ -0,0 +1 @@
+extends "../../input-helper/classes/AbstractModule.pkl"

pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented4.pkl

@@ -0,0 +1,10 @@
+abstract class AbstractMethod {
+  abstract function foo(): Int
+}
+
+abstract class AbstractIntermediate extends AbstractMethod
+
+class MyClass extends AbstractIntermediate {
+}
+
+foo: MyClass

pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented1.err

@@ -0,0 +1,6 @@
+–– Pkl Error ––
+Class `abstractMethodNotImplemented1#MyClass` should either be declared `abstract`, or should implement method `foo()`.
+
+x | class MyClass extends AbstractMethod {
+    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+at abstractMethodNotImplemented1 (file:///$snippetsDir/input/errors/abstractMethodNotImplemented1.pkl)

pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented2.err

@@ -0,0 +1,8 @@
+–– Pkl Error ––
+Class `abstractMethodNotImplemented2#MyClass` should either be declared `abstract`, or should implement the following methods:
+foo()
+bar()
+
+x | class MyClass extends AbstractMethods {
+    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+at abstractMethodNotImplemented2 (file:///$snippetsDir/input/errors/abstractMethodNotImplemented2.pkl)

pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented3.err

@@ -0,0 +1,6 @@
+–– Pkl Error ––
+Class `abstractMethodNotImplemented3` should either be declared `abstract`, or should implement method `bar()`.
+
+x | extends "../../input-helper/classes/AbstractModule.pkl"
+    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+at abstractMethodNotImplemented3 (file:///$snippetsDir/input/errors/abstractMethodNotImplemented3.pkl)

pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented4.err

@@ -0,0 +1,6 @@
+–– Pkl Error ––
+Class `abstractMethodNotImplemented4#MyClass` should either be declared `abstract`, or should implement method `foo()`.
+
+x | class MyClass extends AbstractIntermediate {
+    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+at abstractMethodNotImplemented4 (file:///$snippetsDir/input/errors/abstractMethodNotImplemented4.pkl)