From 762db144ceb571f6c08800dd9cbbf90975d56218 Mon Sep 17 00:00:00 2001 From: Jen Basch Date: Mon, 17 Aug 2026 17:52:54 -0400 Subject: [PATCH] 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 --- .../org/pkl/core/ast/member/ClassNode.java | 66 +++++++++++-------- .../java/org/pkl/core/runtime/VmClass.java | 53 ++++++++++++++- .../org/pkl/core/runtime/VmLocalContext.java | 30 +++++++++ .../org/pkl/core/errorMessages.properties | 7 ++ .../input-helper/classes/AbstractModule.pkl | 3 + .../errors/abstractMethodNotImplemented1.pkl | 8 +++ .../errors/abstractMethodNotImplemented2.pkl | 9 +++ .../errors/abstractMethodNotImplemented3.pkl | 1 + .../errors/abstractMethodNotImplemented4.pkl | 10 +++ .../errors/abstractMethodNotImplemented1.err | 6 ++ .../errors/abstractMethodNotImplemented2.err | 8 +++ .../errors/abstractMethodNotImplemented3.err | 6 ++ .../errors/abstractMethodNotImplemented4.err | 6 ++ 13 files changed, 182 insertions(+), 31 deletions(-) create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input-helper/classes/AbstractModule.pkl create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented1.pkl create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented2.pkl create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented3.pkl create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented4.pkl create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented1.err create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented2.err create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented3.err create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented4.err diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/ClassNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/ClassNode.java index 79a6f4cb3..ed5e3989d 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/ClassNode.java +++ b/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 supertypeNode = unresolvedSupertypeNode.execute(frame); - var superclass = supertypeNode.getVmClass(); + var localContext = VmLanguage.get(this).localContext.get(); + localContext.beginClassInit(cachedClass); - checkSupertype(supertypeNode, superclass); - cachedClass.initSupertype(supertypeNode, superclass); + try { + 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) { diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java index 68614aa86..dc41f0862 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java +++ b/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(abstractMethods.size()); + for (var method : abstractMethods) { + methodList.add(method.getCallSignature()); + } + throw new VmExceptionBuilder() + .evalError( + "noImplementationForAbstractMethods", getDisplayName(), MultilineValue.of(methodList)) + .withSourceSection(getHeaderSection()) + .build(); + } + + private List getAbstractMethods() { + assert this.superclass != null; + var result = new ArrayList(); + 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. + * + *

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(); } diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmLocalContext.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmLocalContext.java index f3f799ce8..36c213277 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmLocalContext.java +++ b/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 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; diff --git a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties index b1e52a977..7d335d4e0 100644 --- a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties +++ b/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} diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input-helper/classes/AbstractModule.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input-helper/classes/AbstractModule.pkl new file mode 100644 index 000000000..ab2e65b43 --- /dev/null +++ b/pkl-core/src/test/files/LanguageSnippetTests/input-helper/classes/AbstractModule.pkl @@ -0,0 +1,3 @@ +abstract module Foo + +abstract function bar(): Int diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented1.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented1.pkl new file mode 100644 index 000000000..7f8838864 --- /dev/null +++ b/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 diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented2.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented2.pkl new file mode 100644 index 000000000..8a7dca4b5 --- /dev/null +++ b/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 diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented3.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented3.pkl new file mode 100644 index 000000000..d84a1a6e3 --- /dev/null +++ b/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented3.pkl @@ -0,0 +1 @@ +extends "../../input-helper/classes/AbstractModule.pkl" diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented4.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/errors/abstractMethodNotImplemented4.pkl new file mode 100644 index 000000000..dd4b5f5ff --- /dev/null +++ b/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 diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented1.err b/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented1.err new file mode 100644 index 000000000..8a9fef450 --- /dev/null +++ b/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) diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented2.err b/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented2.err new file mode 100644 index 000000000..0d50208d1 --- /dev/null +++ b/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) diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented3.err b/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented3.err new file mode 100644 index 000000000..04b7a1d0b --- /dev/null +++ b/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) diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented4.err b/pkl-core/src/test/files/LanguageSnippetTests/output/errors/abstractMethodNotImplemented4.err new file mode 100644 index 000000000..d0a18cc9c --- /dev/null +++ b/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)