Pattern match does not evaluate suspended atom fields #14525
Conversation
Akirathan
added
the
CI: No changelog needed
Akirathan
changed the title
# Conflicts: # engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/ConstructorBranchNode.java
Akirathan
added
the
CI: Clean build required
JaroslavTulach
left a comment
JaroslavTulach
left a comment
There was a problem hiding this comment.
- Nice
- If we are green, then let's go!
| case v2 of | ||
| My_Ref.Eager e -> e | ||
| My_Ref.Lazy l -> |
There was a problem hiding this comment.
- I'd be interested in ensuring that
My_Ref.Lazy l:Integercase match does work as expected (e.g. it does evaluate thelfield) - Maybe also that
My_Ref.Lazy l:Textfails
There was a problem hiding this comment.
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.
- I think you can change the tests, to "materialize" the lazy field.
- we need the real value to perform the type check
- so "materializing" the field is OK
- I just wanted to cover that behavior with a test
| 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) { |
There was a problem hiding this comment.
- idea: Shouldn't there be
structsLib.isSuspended(obj, i)?
There was a problem hiding this comment.
That seems like a better idea than this hack with instanceof. I will implement this new message.
| return Function.fullyApplied(getFieldNode.getCallTarget()); | ||
| } | ||
|
|
||
| private static final class GetSuspendedFieldNode extends RootNode { |
There was a problem hiding this comment.
- there already is LazyCheckRootNode which is quite similar
- is some unification possible/desirable?
- why is this a
RootNodeand notClosureEnsoNode? - will it be properly represented in stacktraces?
LazyCheckRootNodeprobably isn't...
| * @param resolvedCons The actual constructor resolved at runtime. | ||
| * @return | ||
| */ | ||
| private def atomFieldsAsArguments( |
There was a problem hiding this comment.
Shouldn't this be a method in AtomConstructor? Not taking Pattern.Constructor, but for example String[] fieldsAsName argument? It seems to me this is quite constructor specific behavior that'd be better encapsulated in AtomConstructor.
Fixes #10465
Pull Request Description
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java