-
Notifications
You must be signed in to change notification settings - Fork 334
Pattern match does not evaluate suspended atom fields #14525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
5f1be0c
695f202
47c18cb
e1d0f71
cd7fce4
0a08513
0e9b2b0
bc515b7
a403dab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package org.enso.interpreter.node.controlflow.caseexpr; | ||
|
|
||
| import com.oracle.truffle.api.CompilerAsserts; | ||
| import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; | ||
| import com.oracle.truffle.api.RootCallTarget; | ||
| import com.oracle.truffle.api.dsl.Cached; | ||
| import com.oracle.truffle.api.dsl.Cached.Shared; | ||
|
|
@@ -10,8 +11,11 @@ | |
| import com.oracle.truffle.api.library.CachedLibrary; | ||
| import com.oracle.truffle.api.nodes.ExplodeLoop; | ||
| import com.oracle.truffle.api.nodes.NodeInfo; | ||
| import com.oracle.truffle.api.nodes.RootNode; | ||
| import com.oracle.truffle.api.profiles.CountingConditionProfile; | ||
| import org.enso.interpreter.EnsoLanguage; | ||
| import org.enso.interpreter.node.expression.builtin.meta.IsValueOfTypeNode; | ||
| import org.enso.interpreter.runtime.callable.function.Function; | ||
| import org.enso.interpreter.runtime.data.EnsoMultiValue; | ||
| import org.enso.interpreter.runtime.data.atom.Atom; | ||
| import org.enso.interpreter.runtime.data.atom.AtomConstructor; | ||
|
|
@@ -85,9 +89,41 @@ private static Object[] fieldsFromObject( | |
| Object obj, AtomConstructor cons, StructsLibrary structsLib) { | ||
| CompilerAsserts.partialEvaluationConstant(cons); | ||
| var arr = new Object[cons.getArity()]; | ||
| var fields = cons.getFields(); | ||
| for (var i = 0; i < arr.length; i++) { | ||
| arr[i] = structsLib.getField(obj, i); | ||
| // Wrap suspended fields in functions, so that they are not evaluated | ||
| // too eagerly. | ||
| if (fields[i].isSuspended() && obj instanceof Atom atom) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems like a better idea than this hack with |
||
| var func = createSuspendedFieldFunction(atom, i); | ||
| arr[i] = func; | ||
| } else { | ||
| arr[i] = structsLib.getField(obj, i); | ||
| } | ||
| } | ||
| return arr; | ||
| } | ||
|
|
||
| @TruffleBoundary | ||
| private static Function createSuspendedFieldFunction(Atom atom, int fieldIdx) { | ||
| var getFieldNode = new GetSuspendedFieldNode(atom, fieldIdx); | ||
| return Function.fullyApplied(getFieldNode.getCallTarget()); | ||
| } | ||
|
|
||
| private static final class GetSuspendedFieldNode extends RootNode { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| private final Atom atom; | ||
| private final int fieldIdx; | ||
| @Child private StructsLibrary structsLib = StructsLibrary.getFactory().createDispatched(3); | ||
|
|
||
| GetSuspendedFieldNode(Atom atom, int fieldIdx) { | ||
| super(EnsoLanguage.get(null)); | ||
| this.atom = atom; | ||
| this.fieldIdx = fieldIdx; | ||
| } | ||
|
|
||
| @Override | ||
| public Object execute(VirtualFrame frame) { | ||
| // Forces evaluation of the suspended field. | ||
| return structsLib.getField(atom, fieldIdx); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1481,17 +1481,6 @@ private[runtime] class IrToTruffle( | |
| ) | ||
| } | ||
|
|
||
| val fieldNames = cons.unsafeFieldsAsNamed | ||
| val fieldsAsArgs = fieldNames.map(genArgFromMatchField) | ||
|
|
||
| val branchCodeNode = childProcessor.processFunctionBody( | ||
| fieldsAsArgs, | ||
| branch.expression, | ||
| branch.location, | ||
| subjectToInstrumentation = subjectToInstrumentation, | ||
| defineRoot = false | ||
| ) | ||
|
|
||
| constructor match { | ||
| case err: errors.Resolution => | ||
| Left(BadPatternMatch.NonVisibleConstructor(err.name)) | ||
|
|
@@ -1502,6 +1491,14 @@ private[runtime] class IrToTruffle( | |
| case Some( | ||
| BindingsMap.Resolution(BindingsMap.ResolvedModule(mod)) | ||
| ) => | ||
| val branchCodeNode = | ||
| createBranchCodeNodeForConstructorPattern( | ||
| branch, | ||
| cons, | ||
| None, | ||
| childProcessor, | ||
| subjectToInstrumentation | ||
| ) | ||
| Right( | ||
| ObjectEqualityBranchNode.build( | ||
| branchCodeNode.getCallTarget, | ||
|
|
@@ -1511,11 +1508,19 @@ private[runtime] class IrToTruffle( | |
| ) | ||
| case Some( | ||
| BindingsMap.Resolution( | ||
| BindingsMap.ResolvedConstructor(tp, cons) | ||
| BindingsMap.ResolvedConstructor(tp, resolvedCons) | ||
| ) | ||
| ) => | ||
| val atomCons = | ||
| asType(tp).getConstructors.get(cons.name) | ||
| asType(tp).getConstructors.get(resolvedCons.name) | ||
| val branchCodeNode = | ||
| createBranchCodeNodeForConstructorPattern( | ||
| branch, | ||
| cons, | ||
| Some(atomCons), | ||
| childProcessor, | ||
| subjectToInstrumentation | ||
| ) | ||
| val r = if (atomCons == getBuiltins.bool().getTrue) { | ||
| BooleanBranchNode.build( | ||
| true, | ||
|
|
@@ -1543,6 +1548,14 @@ private[runtime] class IrToTruffle( | |
| ) => | ||
| val tpe = | ||
| asType(binding) | ||
| val branchCodeNode = | ||
| createBranchCodeNodeForConstructorPattern( | ||
| branch, | ||
| cons, | ||
| None, | ||
| childProcessor, | ||
| subjectToInstrumentation | ||
| ) | ||
| val polyglot = getBuiltins.polyglot | ||
| val branchNode = if (tpe == polyglot) { | ||
| PolyglotBranchNode.build( | ||
|
|
@@ -1563,6 +1576,14 @@ private[runtime] class IrToTruffle( | |
| BindingsMap.ResolvedPolyglotSymbol(mod, symbol) | ||
| ) | ||
| ) => | ||
| val branchCodeNode = | ||
| createBranchCodeNodeForConstructorPattern( | ||
| branch, | ||
| cons, | ||
| None, | ||
| childProcessor, | ||
| subjectToInstrumentation | ||
| ) | ||
| val polyglotSymbol = | ||
| asScope(mod.unsafeAsModule()) | ||
| .getPolyglotSymbolSupplier(symbol.name) | ||
|
|
@@ -1581,6 +1602,14 @@ private[runtime] class IrToTruffle( | |
| BindingsMap.ResolvedPolyglotField(typ, symbol) | ||
| ) | ||
| ) => | ||
| val branchCodeNode = | ||
| createBranchCodeNodeForConstructorPattern( | ||
| branch, | ||
| cons, | ||
| None, | ||
| childProcessor, | ||
| subjectToInstrumentation | ||
| ) | ||
| val mod = typ.module | ||
| val polyClass = asScope(mod.unsafeAsModule()) | ||
| .getPolyglotSymbolSupplier(typ.symbol.name) | ||
|
|
@@ -1801,6 +1830,62 @@ private[runtime] class IrToTruffle( | |
| } | ||
| } | ||
|
|
||
| /** Case branch of a [[Pattern.Constructor]] is represented as a function with parameters matching the | ||
| * fields of the constructor. This method converts those fields into function arguments. | ||
| * If a field is suspended, the corresponding argument is marked as suspended too. | ||
| * @param cons Constructor pattern. | ||
| * @param resolvedCons The actual constructor resolved at runtime. | ||
| * @return | ||
| */ | ||
| private def atomFieldsAsArguments( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be a method in |
||
| cons: Pattern.Constructor, | ||
| resolvedCons: AtomConstructor | ||
| ): List[DefinitionArgument] = { | ||
| val resolvedFields = resolvedCons.getFields.toList | ||
| val fieldNames = cons.unsafeFieldsAsNamed | ||
| fieldNames.zip(resolvedFields).map { case (fieldName, resolvedField) => | ||
| DefinitionArgument.Specified | ||
| .builder() | ||
| .name(fieldName.name) | ||
| .suspended(resolvedField.isSuspended) | ||
| .location(fieldName.identifiedLocation) | ||
| .passData(fieldName.name.passData()) | ||
| .diagnostics(fieldName.name.diagnostics()) | ||
| .build() | ||
| } | ||
| } | ||
|
|
||
| private def createBranchCodeNodeForConstructorPattern( | ||
| branch: Case.Branch, | ||
| cons: Pattern.Constructor, | ||
| resolvedConsOpt: Option[AtomConstructor], | ||
| childProcessor: ExpressionProcessor, | ||
| subjectToInstrumentation: Boolean | ||
| ): CreateFunctionNode = { | ||
| val fieldNames = cons.unsafeFieldsAsNamed | ||
| resolvedConsOpt match { | ||
| case None => | ||
| val fieldsAsArgs = fieldNames.map(genArgFromMatchField) | ||
| childProcessor.processFunctionBody( | ||
| fieldsAsArgs, | ||
| branch.expression, | ||
| branch.location, | ||
| subjectToInstrumentation = subjectToInstrumentation, | ||
| defineRoot = false | ||
| ) | ||
| case Some(resolvedOpt) => | ||
| val fieldsAsSuspendedArgs = | ||
| atomFieldsAsArguments(cons, resolvedOpt) | ||
| childProcessor.processFunctionBody( | ||
| fieldsAsSuspendedArgs, | ||
| branch.expression, | ||
| branch.location, | ||
| subjectToInstrumentation = subjectToInstrumentation, | ||
| defineRoot = false | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /* Note [Pattern Match Fallbacks] | ||
| * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| * Enso in its current state has no coverage checking for constructors on | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My_Ref.Lazy l:Integercase match does work as expected (e.g. it does evaluate thelfield)My_Ref.Lazy l:TextfailsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for it in a403dab. Currently, they are failing, because the type assertion forces the field to be evaluated. I will need to implement also lazy type checking for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.