Rewrite MacroEvaluator to support `flatten`, and container-making macros #1019

popematt · 2024-12-18T18:04:55Z

Issue #, if available:

Description of changes:

I've added PR tour comments, indicated with a 🗺️.

There are no comments on MacroEvaluator because if there's anything there that needs further explanation, I've tried to put it in the JavaDoc. If I've forgotten anything there, it should also go in the Javadoc. I recommend looking at the MacroEvaluator files individually rather than trying to look at the diff.

At a high level, this rewrite creates something sort of like a 2-D stack, where containers are the first dimension, and macro expansions are the second dimension, so that the evaluator can look at the top of the container stack and the bottom of the macro stack in order to get the next value. See the MacroEvaluator's doc comments for more details.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2024-12-18T18:10:36Z

Codecov Report

Attention: Patch coverage is 82.02020% with 89 lines in your changes missing coverage. Please review.

Please upload report for BASE (ion-11-encoding@620228b). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...n/java/com/amazon/ion/impl/macro/MacroEvaluator.kt	81.11%	51 Missing and 30 partials ⚠️
...main/java/com/amazon/ion/impl/macro/Environment.kt	0.00%	5 Missing ⚠️
.../main/java/com/amazon/ion/impl/macro/Expression.kt	50.00%	2 Missing ⚠️
src/main/java/com/amazon/ion/util/Assumptions.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             ion-11-encoding    #1019   +/-   ##
==================================================
  Coverage                   ?   70.81%           
  Complexity                 ?     7375           
==================================================
  Files                      ?      214           
  Lines                      ?    29533           
  Branches                   ?     5319           
==================================================
  Hits                       ?    20914           
  Misses                     ?     6960           
  Partials                   ?     1659

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

popematt · 2024-12-18T18:23:29Z

src/main/java/com/amazon/ion/impl/macro/EExpressionArgsReader.java

-        if (isMacroInvocation()) {
+        if (isImplicitRest && !isContainerAnExpressionGroup()) {
+            readStreamAsExpressionGroup(expressions);
+            return;
+        } else if (isMacroInvocation()) {


🗺️ Had to change the order of some checks in this method so that it would correctly capture rest args when the first of the rest args was a macro invocation.

popematt · 2024-12-18T18:24:50Z

src/main/java/com/amazon/ion/impl/macro/EExpressionArgsReader.java

-        if (isImplicitRest && !isContainerAnExpressionGroup()) {
-            readStreamAsExpressionGroup(expressions);
-        } else if (IonType.isContainer(type)) {
+        if (IonType.isContainer(type) && !reader.isNullValue()) {


🗺️ Added a check for null here because in the Expression model, all null values are treated as scalars. This was causing some of the conformance tests to fail for e.g. make_list.

popematt · 2024-12-18T18:25:22Z

src/main/java/com/amazon/ion/impl/macro/Environment.kt

@@ -21,6 +21,16 @@ data class Environment private constructor(
    val parentEnvironment: Environment?,
 ) {
    fun createChild(arguments: List<Expression>, argumentIndices: List<Int>) = Environment(arguments, argumentIndices, this)
+
+    override fun toString() = """


🗺️ The only change in this file was adding a custom toString() implementation to help when I was debugging things.

popematt · 2024-12-18T18:26:59Z

src/main/java/com/amazon/ion/impl/macro/Expression.kt

@@ -221,21 +237,25 @@ sealed interface Expression {
     */
    data class VariableRef(val signatureIndex: Int) : TemplateBodyExpression

+    sealed interface InvokableExpression : HasStartAndEnd, Expression {


🗺️ We're able to reduce some code duplication by unifying the code paths for TDL and E-Expression macros. This interface helps to facilitate that unification.

popematt · 2024-12-18T18:30:16Z

src/main/java/com/amazon/ion/impl/macro/Expression.kt

+    /**
+     * Indicates to the macro evaluator that the current expansion did not produce a value this time, but it may
+     * produce more expressions. The macro evaluator should request another expression from that macro.
+     */
+    data object ContinueExpansion : ExpansionOutputExpressionOrContinue
+
+    /** Signals the end of an expansion in the macro evaluator. */
+    data object EndOfExpansion : ExpansionOutputExpression


🗺️ It was getting confusing to keep track of what null meant when returned from some of the internal functions of the macro evaluator. I added the singleton types ContinueExpansion and EndOfExpansion (and marker interfaces that combine them with DataModelExpression) so that we can be explicit about the result of a call to the macro expansion function instead of returning null.

popematt · 2024-12-18T18:36:42Z

src/test/java/com/amazon/ion/impl/macro/MacroEvaluatorTest.kt

@@ -560,9 +601,13 @@ class MacroEvaluatorTest {
            }
        }

+        assertIsInstance<StructValue>(evaluator.expandNext())


🗺️ This test for make_field was wrong. It was testing that the macro produced a field name and a value, but should have been testing that it produced a struct containing a single name-value pair.

popematt · 2024-12-18T18:40:27Z

src/test/java/com/amazon/ion/conformance/structure.kt

-    val firstExpression = continuation.first()
-    firstExpression as? SeqElement ?: builder.reportSyntaxError(firstExpression, "continuation")
+    val firstExpression = continuation.firstOrNull()
+    firstExpression as? SeqElement ?: builder.reportSyntaxError(sexp, "continuation")


🗺️ If there was no firstExpression, calling first() would throw NoSuchElementException, which didn't have any information about where the syntax error occurred in the conformance DSL. This change just ensures that more syntax errors are reported in a useful way.

popematt · 2024-12-18T18:41:54Z

src/test/java/com/amazon/ion/conformance/expectations.kt

@@ -53,6 +54,7 @@ fun TestCaseSupport.assertSignals(sexp: SeqElement, r: IonReader) {
 private fun IonReader.walk(): List<String> {
    val events = mutableListOf<String>()
    fun recordEvent(eventType: String = type.toString(), value: Any? = "") {
+        if (events.size > 10_000_000) fail("Ion stream does not appear to terminate.")


🗺️ Useful safeguard for when you introduce an infinite loop, or you start writing a conformance DSL test case consisting of a "Billion Laughs" attack.

popematt · 2024-12-18T18:49:48Z

src/test/java/com/amazon/ion/conformance/ConformanceTestRunner.kt

+            // FIXME: Implicit rest args don't always work
+            "implicit rest args" in completeTestName -> false


🗺️ Specifically, rest args for the if_* tests are failing. I haven't yet isolated whether this is because of an issue with those macros (unlikely), TDL in general (would also be surprising to me), or something else.

popematt · 2024-12-18T18:53:32Z

src/main/java/com/amazon/ion/impl/macro/SystemMacro.kt

+    _Private_FlattenStruct(-1, _systemSymbol = null, listOf(zeroToManyTagged("structs"))),
+    _Private_MakeFieldNameAndValue(-1, _systemSymbol = null, listOf(exactlyOneTagged("fieldName"), exactlyOneTagged("value"))),


🗺️ These have no integer ID and no name, so they are not possible to invoke in an Ion stream or in a user-defined template macro (because the definition of a macro that uses these cannot be serialized). They are only used for supporting make_struct and make_field respectively.

The methods of SystemMacro were also updated to never return these, so the only way to get your hands on it is by explicitly using SystemMacro._Private_.... It's not great, but it's good enough for now.

tgregg · 2024-12-27T00:29:19Z

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

+ * One might visualize it like this:
+ * ```
+ * 3. List     : Stream --> Delta --> Variable
+ * 2. List     : Stream --> Flatten --> Stream
+ * 1. Struct   : Stream --> Variable --> TemplateBody --> Stream --> TemplateBody
+ * 0. TopLevel : Stream --> TemplateBody --> TemplateBody
+ * ```


More explanation here might help. By itself, I wasn't sure what it was trying to help me visualize. Should it be accompanied by some sample data?

tgregg · 2024-12-27T21:00:38Z

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

+            // TODO: This check is O(n). Consider removing this when confident there are no double frees.
+            check(ex !in expanderPool)
+            expanderPool.add(ex)


Could also consider using something like Set<ExpansionInfo> expanderPool = Collections.newSetFromMap(new IdentityHashMap<>()), preventing you from adding the same instance back to the pool more than once, though you'd need some extra logic to poll from the pool.

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

Co-authored-by: Tyler Gregg <[email protected]>

popematt added 8 commits December 16, 2024 16:15

Everything is awesome!

fea45ff

Working rewrite of MacroEvaluator

fbef69b

Cleanup

d4a7f6c

More cleanup

474be3f

Even more cleanup

1bd9701

Update ion-tests submodule

1e8bc03

Even moar cleanup

92560f5

Rearrange some lines to minimize the diff

a9fc6ea

popematt commented Dec 18, 2024

View reviewed changes

popematt marked this pull request as ready for review December 18, 2024 19:00

popematt requested a review from tgregg December 20, 2024 17:03

tgregg approved these changes Dec 27, 2024

View reviewed changes

popematt and others added 2 commits January 2, 2025 11:14

Update src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

75e012d

Co-authored-by: Tyler Gregg <[email protected]>

Adds suggested changes

14f315d

popematt merged commit f816953 into amazon-ion:ion-11-encoding Jan 2, 2025
18 of 35 checks passed

popematt deleted the evaluator-rewrite-complete branch January 2, 2025 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite MacroEvaluator to support `flatten`, and container-making macros #1019

Rewrite MacroEvaluator to support `flatten`, and container-making macros #1019

popematt commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

popematt Dec 18, 2024

tgregg Dec 27, 2024

tgregg Dec 27, 2024

		// FIXME: Implicit rest args don't always work
		"implicit rest args" in completeTestName -> false

		_Private_FlattenStruct(-1, _systemSymbol = null, listOf(zeroToManyTagged("structs"))),
		_Private_MakeFieldNameAndValue(-1, _systemSymbol = null, listOf(exactlyOneTagged("fieldName"), exactlyOneTagged("value"))),

Rewrite MacroEvaluator to support flatten, and container-making macros #1019

Rewrite MacroEvaluator to support flatten, and container-making macros #1019

Conversation

popematt commented Dec 18, 2024 • edited Loading

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rewrite MacroEvaluator to support `flatten`, and container-making macros #1019

Rewrite MacroEvaluator to support `flatten`, and container-making macros #1019

popematt commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading