From 17f431370a9560776d7e23c5ebe3459d4273eb54 Mon Sep 17 00:00:00 2001 From: odenix Date: Thu, 19 Dec 2024 09:58:44 -0800 Subject: [PATCH] Fix exception handling of PklRootNode's (#837) Motivation: - Perform same exception handling for every implementation of PklRootNode.execute(). - Avoid code duplication. Changes: - Change PklRootNode.execute() to be a final method that performs exception handling and calls abstract method executeImpl(), which is implemented by subclasses. - Remove executeBody() methods, which served a similar purpose but were more limited. - Remove duplicate exception handling code. Result: - More reliable exception handling. This should fix known problems such as misclassifying stack overflows as internal errors and displaying errors without a stack trace. - Less code duplication. --- .../java/org/pkl/core/ast/MemberNode.java | 5 --- .../java/org/pkl/core/ast/PklRootNode.java | 12 +++++- .../java/org/pkl/core/ast/SimpleRootNode.java | 4 +- .../org/pkl/core/ast/member/FunctionNode.java | 43 ++++++------------- .../member/ListingOrMappingTypeCastNode.java | 11 +---- .../ast/member/LocalTypedPropertyNode.java | 26 +++-------- .../org/pkl/core/ast/member/ModuleNode.java | 4 +- .../pkl/core/ast/member/ObjectMethodNode.java | 2 +- .../pkl/core/ast/member/PropertyTypeNode.java | 38 ++++++---------- .../pkl/core/ast/member/SharedMemberNode.java | 4 +- .../ast/member/TypeCheckedPropertyNode.java | 6 +-- .../core/ast/member/TypedPropertyNode.java | 4 +- .../ast/member/UntypedObjectMemberNode.java | 4 +- .../core/ast/repl/ResolveClassMemberNode.java | 2 +- .../pkl/core/ast/type/IdentityMixinNode.java | 24 +++-------- .../stdlib/benchmark/MicrobenchmarkNodes.java | 25 +++-------- 16 files changed, 71 insertions(+), 143 deletions(-) diff --git a/pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java b/pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java index 6d9d738d..b56a1505 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java @@ -16,7 +16,6 @@ package org.pkl.core.ast; import com.oracle.truffle.api.frame.FrameDescriptor; -import com.oracle.truffle.api.frame.VirtualFrame; import com.oracle.truffle.api.source.SourceSection; import java.util.function.Function; import org.pkl.core.ast.member.DefaultPropertyBodyNode; @@ -48,10 +47,6 @@ public abstract class MemberNode extends PklRootNode { bodyNode = insert(replacer.apply(bodyNode)); } - protected final Object executeBody(VirtualFrame frame) { - return executeBody(frame, bodyNode); - } - protected final VmExceptionBuilder exceptionBuilder() { return new VmExceptionBuilder().withSourceSection(getHeaderSection()); } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/PklRootNode.java b/pkl-core/src/main/java/org/pkl/core/ast/PklRootNode.java index 089f6c77..ef8c85e5 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/PklRootNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/PklRootNode.java @@ -22,6 +22,7 @@ import com.oracle.truffle.api.frame.VirtualFrame; import com.oracle.truffle.api.nodes.NodeInfo; import com.oracle.truffle.api.nodes.RootNode; import com.oracle.truffle.api.source.SourceSection; +import org.pkl.core.ast.type.VmTypeMismatchException; import org.pkl.core.runtime.*; import org.pkl.core.util.Nullable; @@ -36,9 +37,16 @@ public abstract class PklRootNode extends RootNode { public abstract @Nullable String getName(); - protected final Object executeBody(VirtualFrame frame, ExpressionNode bodyNode) { + // name must start with `execute` (see Javadoc of @Specialization) + protected abstract Object executeImpl(VirtualFrame frame); + + @Override + public final Object execute(VirtualFrame frame) { try { - return bodyNode.executeGeneric(frame); + return executeImpl(frame); + } catch (VmTypeMismatchException e) { + CompilerDirectives.transferToInterpreter(); + throw e.toVmException(); } catch (VmException e) { CompilerDirectives.transferToInterpreter(); throw e; diff --git a/pkl-core/src/main/java/org/pkl/core/ast/SimpleRootNode.java b/pkl-core/src/main/java/org/pkl/core/ast/SimpleRootNode.java index d5a2388d..77b21c2c 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/SimpleRootNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/SimpleRootNode.java @@ -49,7 +49,7 @@ public final class SimpleRootNode extends PklRootNode { } @Override - public Object execute(VirtualFrame frame) { - return executeBody(frame, bodyNode); + protected Object executeImpl(VirtualFrame frame) { + return bodyNode.executeGeneric(frame); } } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/FunctionNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/FunctionNode.java index 8adbbefc..2fec17c1 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/FunctionNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/FunctionNode.java @@ -29,7 +29,6 @@ import org.pkl.core.TypeParameter; import org.pkl.core.ast.ExpressionNode; import org.pkl.core.ast.VmModifier; import org.pkl.core.ast.type.TypeNode; -import org.pkl.core.ast.type.VmTypeMismatchException; import org.pkl.core.runtime.*; import org.pkl.core.util.CollectionUtils; import org.pkl.core.util.Nullable; @@ -106,7 +105,7 @@ public final class FunctionNode extends RegularMemberNode { @Override @ExplodeLoop - public Object execute(VirtualFrame frame) { + protected Object executeImpl(VirtualFrame frame) { var totalArgCount = frame.getArguments().length; if (totalArgCount != totalParamCount) { CompilerDirectives.transferToInterpreter(); @@ -114,37 +113,23 @@ public final class FunctionNode extends RegularMemberNode { } var isInIterable = (boolean) frame.getArguments()[2]; - try { - for (var i = 0; i < parameterTypeNodes.length; i++) { - var argument = frame.getArguments()[IMPLICIT_PARAM_COUNT + i]; - if (isInIterable) { - parameterTypeNodes[i].executeEagerlyAndSet(frame, argument); - } else { - parameterTypeNodes[i].executeAndSet(frame, argument); - } - } - var result = bodyNode.executeGeneric(frame); - - if (checkedReturnTypeNode != null) { - return checkedReturnTypeNode.execute(frame, result); - } - - return result; - } catch (VmTypeMismatchException e) { - CompilerDirectives.transferToInterpreter(); - throw e.toVmException(); - } catch (StackOverflowError e) { - CompilerDirectives.transferToInterpreter(); - throw new VmStackOverflowException(e); - } catch (Exception e) { - CompilerDirectives.transferToInterpreter(); - if (e instanceof VmException) { - throw e; + for (var i = 0; i < parameterTypeNodes.length; i++) { + var argument = frame.getArguments()[IMPLICIT_PARAM_COUNT + i]; + if (isInIterable) { + parameterTypeNodes[i].executeEagerlyAndSet(frame, argument); } else { - throw exceptionBuilder().bug(e.getMessage()).withCause(e).build(); + parameterTypeNodes[i].executeAndSet(frame, argument); } } + + var result = bodyNode.executeGeneric(frame); + + if (checkedReturnTypeNode != null) { + return checkedReturnTypeNode.execute(frame, result); + } + + return result; } public VmMap getParameterMirrors() { diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/ListingOrMappingTypeCastNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/ListingOrMappingTypeCastNode.java index 16a327d7..6ac3e304 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/ListingOrMappingTypeCastNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/ListingOrMappingTypeCastNode.java @@ -15,13 +15,11 @@ */ package org.pkl.core.ast.member; -import com.oracle.truffle.api.CompilerDirectives; import com.oracle.truffle.api.frame.FrameDescriptor; import com.oracle.truffle.api.frame.VirtualFrame; import com.oracle.truffle.api.source.SourceSection; import org.pkl.core.ast.PklRootNode; import org.pkl.core.ast.type.TypeNode; -import org.pkl.core.ast.type.VmTypeMismatchException; import org.pkl.core.runtime.VmLanguage; import org.pkl.core.util.Nullable; @@ -53,12 +51,7 @@ public final class ListingOrMappingTypeCastNode extends PklRootNode { } @Override - public Object execute(VirtualFrame frame) { - try { - return typeNode.execute(frame, frame.getArguments()[2]); - } catch (VmTypeMismatchException e) { - CompilerDirectives.transferToInterpreter(); - throw e.toVmException(); - } + protected Object executeImpl(VirtualFrame frame) { + return typeNode.execute(frame, frame.getArguments()[2]); } } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/LocalTypedPropertyNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/LocalTypedPropertyNode.java index 5cfe2443..7998bea8 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/LocalTypedPropertyNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/LocalTypedPropertyNode.java @@ -21,8 +21,6 @@ import com.oracle.truffle.api.frame.VirtualFrame; import org.pkl.core.ast.ExpressionNode; import org.pkl.core.ast.type.TypeNode; import org.pkl.core.ast.type.UnresolvedTypeNode; -import org.pkl.core.ast.type.VmTypeMismatchException; -import org.pkl.core.runtime.VmException; import org.pkl.core.runtime.VmLanguage; import org.pkl.core.util.LateInit; import org.pkl.core.util.Nullable; @@ -58,25 +56,13 @@ public final class LocalTypedPropertyNode extends RegularMemberNode { } @Override - public Object execute(VirtualFrame frame) { - try { - if (typeNode == null) { - CompilerDirectives.transferToInterpreter(); - typeNode = insert(unresolvedTypeNode.execute(frame)); - unresolvedTypeNode = null; - } - var result = bodyNode.executeGeneric(frame); - return typeNode.execute(frame, result); - } catch (VmTypeMismatchException e) { + protected Object executeImpl(VirtualFrame frame) { + if (typeNode == null) { CompilerDirectives.transferToInterpreter(); - throw e.toVmException(); - } catch (Exception e) { - CompilerDirectives.transferToInterpreter(); - if (e instanceof VmException) { - throw e; - } else { - throw exceptionBuilder().bug(e.getMessage()).withCause(e).build(); - } + typeNode = insert(unresolvedTypeNode.execute(frame)); + unresolvedTypeNode = null; } + var result = bodyNode.executeGeneric(frame); + return typeNode.execute(frame, result); } } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/ModuleNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/ModuleNode.java index 48beb376..ea9d84ff 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/ModuleNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/ModuleNode.java @@ -52,8 +52,8 @@ public final class ModuleNode extends PklRootNode { } @Override - public Object execute(VirtualFrame frame) { - var module = executeBody(frame, moduleNode); + protected Object executeImpl(VirtualFrame frame) { + var module = moduleNode.executeGeneric(frame); if (module instanceof VmClass vmClass) { return vmClass.getPrototype(); } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMethodNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMethodNode.java index 416a86e6..aa62321c 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMethodNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMethodNode.java @@ -59,7 +59,7 @@ public final class ObjectMethodNode extends RegularMemberNode { } @Override - public CallTarget execute(VirtualFrame frame) { + protected CallTarget executeImpl(VirtualFrame frame) { if (functionNode == null) { CompilerDirectives.transferToInterpreter(); diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java index 09bd3caa..184890be 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java @@ -15,7 +15,6 @@ */ package org.pkl.core.ast.member; -import com.oracle.truffle.api.CompilerDirectives; import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; import com.oracle.truffle.api.frame.FrameDescriptor; import com.oracle.truffle.api.frame.VirtualFrame; @@ -24,7 +23,6 @@ import org.pkl.core.PType; import org.pkl.core.ast.PklRootNode; import org.pkl.core.ast.type.TypeNode; import org.pkl.core.ast.type.TypeNode.UnknownTypeNode; -import org.pkl.core.ast.type.VmTypeMismatchException; import org.pkl.core.runtime.*; import org.pkl.core.util.Nullable; @@ -67,31 +65,19 @@ public final class PropertyTypeNode extends PklRootNode { } @Override - public Object execute(VirtualFrame frame) { - try { - if (isInIterable(frame)) { - // There is currently a bug around resolving variables within the iterable of a for - // generator or spread syntax (https://github.com/apple/pkl/issues/741) - // - // Normally, mappings/listings are type-checked lazily. However, this results in said - // bug getting widened, for any object members declared in the iterable. - // - // As a workaround for now, prevent the bug from being any worse by ensuring that these - // object members are eagerly typechecked. - return typeNode.executeEagerly(frame, frame.getArguments()[2]); - } - return typeNode.execute(frame, frame.getArguments()[2]); - } catch (VmTypeMismatchException e) { - CompilerDirectives.transferToInterpreter(); - throw e.toVmException(); - } catch (Exception e) { - CompilerDirectives.transferToInterpreter(); - if (e instanceof VmException) { - throw e; - } else { - throw exceptionBuilder().bug(e.getMessage()).withCause(e).build(); - } + protected Object executeImpl(VirtualFrame frame) { + if (isInIterable(frame)) { + // There is currently a bug around resolving variables within the iterable of a for + // generator or spread syntax (https://github.com/apple/pkl/issues/741) + // + // Normally, mappings/listings are type-checked lazily. However, this results in said + // bug getting widened, for any object members declared in the iterable. + // + // As a workaround for now, prevent the bug from being any worse by ensuring that these + // object members are eagerly typechecked. + return typeNode.executeEagerly(frame, frame.getArguments()[2]); } + return typeNode.execute(frame, frame.getArguments()[2]); } public @Nullable Object getDefaultValue() { diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/SharedMemberNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/SharedMemberNode.java index 5237def4..cddb3208 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/SharedMemberNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/SharedMemberNode.java @@ -58,7 +58,7 @@ public class SharedMemberNode extends MemberNode { } @Override - public Object execute(VirtualFrame frame) { - return executeBody(frame); + protected Object executeImpl(VirtualFrame frame) { + return bodyNode.executeGeneric(frame); } } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/TypeCheckedPropertyNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/TypeCheckedPropertyNode.java index 08bc1b53..6f763101 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/TypeCheckedPropertyNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/TypeCheckedPropertyNode.java @@ -51,7 +51,7 @@ public abstract class TypeCheckedPropertyNode extends RegularMemberNode { @Cached("getProperty(cachedOwnerClass)") ClassProperty property, @Cached("createTypeCheckCallNode(property)") @Nullable DirectCallNode callNode) { - var result = executeBody(frame); + var result = bodyNode.executeGeneric(frame); // TODO: propagate SUPER_CALL_MARKER to disable constraint (but not type) check if (callNode != null && VmUtils.shouldRunTypeCheck(frame)) { @@ -66,7 +66,7 @@ public abstract class TypeCheckedPropertyNode extends RegularMemberNode { protected Object eval( VirtualFrame frame, VmObjectLike owner, @Cached("create()") IndirectCallNode callNode) { - var result = executeBody(frame); + var result = bodyNode.executeGeneric(frame); if (VmUtils.shouldRunTypeCheck(frame)) { var property = getProperty(owner.getVmClass()); @@ -86,7 +86,7 @@ public abstract class TypeCheckedPropertyNode extends RegularMemberNode { @Specialization protected Object eval(VirtualFrame frame, @SuppressWarnings("unused") VmDynamic owner) { - return executeBody(frame); + return bodyNode.executeGeneric(frame); } protected ClassProperty getProperty(VmClass ownerClass) { diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/TypedPropertyNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/TypedPropertyNode.java index 8677d02b..d24c2fb9 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/TypedPropertyNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/TypedPropertyNode.java @@ -43,8 +43,8 @@ public final class TypedPropertyNode extends RegularMemberNode { } @Override - public Object execute(VirtualFrame frame) { - var propertyValue = executeBody(frame); + protected Object executeImpl(VirtualFrame frame) { + var propertyValue = bodyNode.executeGeneric(frame); if (VmUtils.shouldRunTypeCheck(frame)) { return typeCheckCallNode.call( VmUtils.getReceiver(frame), diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/UntypedObjectMemberNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/UntypedObjectMemberNode.java index 777fbdb1..bd1f11b2 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/member/UntypedObjectMemberNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/member/UntypedObjectMemberNode.java @@ -32,7 +32,7 @@ public final class UntypedObjectMemberNode extends RegularMemberNode { } @Override - public Object execute(VirtualFrame frame) { - return executeBody(frame); + protected Object executeImpl(VirtualFrame frame) { + return bodyNode.executeGeneric(frame); } } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/repl/ResolveClassMemberNode.java b/pkl-core/src/main/java/org/pkl/core/ast/repl/ResolveClassMemberNode.java index d8ced2d5..c9d587c7 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/repl/ResolveClassMemberNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/repl/ResolveClassMemberNode.java @@ -49,7 +49,7 @@ public final class ResolveClassMemberNode extends PklRootNode { } @Override - public Object execute(VirtualFrame frame) { + protected Object executeImpl(VirtualFrame frame) { return unresolvedNode.execute(frame, clazz); } } diff --git a/pkl-core/src/main/java/org/pkl/core/ast/type/IdentityMixinNode.java b/pkl-core/src/main/java/org/pkl/core/ast/type/IdentityMixinNode.java index 652b9dcf..e9c89b62 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/type/IdentityMixinNode.java +++ b/pkl-core/src/main/java/org/pkl/core/ast/type/IdentityMixinNode.java @@ -20,7 +20,6 @@ import com.oracle.truffle.api.frame.FrameDescriptor; import com.oracle.truffle.api.frame.VirtualFrame; import com.oracle.truffle.api.source.SourceSection; import org.pkl.core.ast.PklRootNode; -import org.pkl.core.runtime.VmException; import org.pkl.core.runtime.VmLanguage; import org.pkl.core.util.Nullable; @@ -53,7 +52,7 @@ public final class IdentityMixinNode extends PklRootNode { } @Override - public Object execute(VirtualFrame frame) { + protected Object executeImpl(VirtualFrame frame) { var arguments = frame.getArguments(); if (arguments.length != 4) { CompilerDirectives.transferToInterpreter(); @@ -62,23 +61,10 @@ public final class IdentityMixinNode extends PklRootNode { .withSourceSection(sourceSection) .build(); } - - try { - var argument = arguments[3]; - if (argumentTypeNode != null) { - return argumentTypeNode.execute(frame, argument); - } - return argument; - } catch (VmTypeMismatchException e) { - CompilerDirectives.transferToInterpreter(); - throw e.toVmException(); - } catch (Exception e) { - CompilerDirectives.transferToInterpreter(); - if (e instanceof VmException) { - throw e; - } else { - throw exceptionBuilder().bug(e.getMessage()).withCause(e).build(); - } + var argument = arguments[3]; + if (argumentTypeNode != null) { + return argumentTypeNode.execute(frame, argument); } + return argument; } } diff --git a/pkl-core/src/main/java/org/pkl/core/stdlib/benchmark/MicrobenchmarkNodes.java b/pkl-core/src/main/java/org/pkl/core/stdlib/benchmark/MicrobenchmarkNodes.java index c8272b1f..757e4144 100644 --- a/pkl-core/src/main/java/org/pkl/core/stdlib/benchmark/MicrobenchmarkNodes.java +++ b/pkl-core/src/main/java/org/pkl/core/stdlib/benchmark/MicrobenchmarkNodes.java @@ -17,7 +17,6 @@ package org.pkl.core.stdlib.benchmark; import static org.pkl.core.stdlib.benchmark.BenchmarkUtils.runBenchmark; -import com.oracle.truffle.api.CompilerDirectives; import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; import com.oracle.truffle.api.dsl.Specialization; import com.oracle.truffle.api.frame.FrameDescriptor; @@ -29,7 +28,6 @@ import org.pkl.core.ast.PklRootNode; import org.pkl.core.ast.internal.BlackholeNode; import org.pkl.core.ast.internal.BlackholeNodeGen; import org.pkl.core.runtime.Identifier; -import org.pkl.core.runtime.VmException; import org.pkl.core.runtime.VmLanguage; import org.pkl.core.runtime.VmTyped; import org.pkl.core.runtime.VmUtils; @@ -83,23 +81,14 @@ public final class MicrobenchmarkNodes { } @Override - public @Nullable Object execute(VirtualFrame frame) { - try { - var repetitions = (long) frame.getArguments()[2]; - for (long i = 0; i < repetitions; i++) { - blackholeNode.executeGeneric(frame); - } - LoopNode.reportLoopCount( - this, repetitions > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int) repetitions); - return null; - } catch (Exception e) { - CompilerDirectives.transferToInterpreter(); - if (e instanceof VmException) { - throw e; - } else { - throw exceptionBuilder().bug(e.getMessage()).withCause(e).build(); - } + protected @Nullable Object executeImpl(VirtualFrame frame) { + var repetitions = (long) frame.getArguments()[2]; + for (long i = 0; i < repetitions; i++) { + blackholeNode.executeGeneric(frame); } + LoopNode.reportLoopCount( + this, repetitions > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int) repetitions); + return null; } } }