Skip to content
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

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

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,16 @@ private void readStreamAsExpressionGroup(
* @param expressions receives the expressions as they are materialized.
*/
protected void readValueAsExpression(boolean isImplicitRest, List<Expression.EExpressionBodyExpression> expressions) {
if (isMacroInvocation()) {
if (isImplicitRest && !isContainerAnExpressionGroup()) {
readStreamAsExpressionGroup(expressions);
return;
} else if (isMacroInvocation()) {
Comment on lines -234 to +237
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ 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.

collectEExpressionArgs(expressions); // TODO avoid recursion
return;
}
IonType type = reader.encodingType();
List<SymbolToken> annotations = getAnnotations();
if (isImplicitRest && !isContainerAnExpressionGroup()) {
readStreamAsExpressionGroup(expressions);
} else if (IonType.isContainer(type)) {
if (IonType.isContainer(type) && !reader.isNullValue()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ 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.

readContainerValueAsExpression(type, annotations, expressions);
} else {
readScalarValueAsExpression(type, annotations, expressions);
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/com/amazon/ion/impl/macro/Environment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@
val parentEnvironment: Environment?,
) {
fun createChild(arguments: List<Expression>, argumentIndices: List<Int>) = Environment(arguments, argumentIndices, this)

override fun toString() = """

Check warning on line 25 in src/main/java/com/amazon/ion/impl/macro/Environment.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L25 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

|Environment(
| argumentIndices: $argumentIndices,
| argumentExpressions: [${arguments.mapIndexed { index, expression -> "\n| $index. $expression" }.joinToString() }

Check warning on line 28 in src/main/java/com/amazon/ion/impl/macro/Environment.kt

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/ion/impl/macro/Environment.kt#L27-L28

Added lines #L27 - L28 were not covered by tests
| ],
| parent: ${parentEnvironment.toString().lines().joinToString("\n| ")},

Check warning on line 30 in src/main/java/com/amazon/ion/impl/macro/Environment.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L30 was not covered by tests
|)
""".trimMargin()

Check warning on line 32 in src/main/java/com/amazon/ion/impl/macro/Environment.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L32 was not covered by tests

companion object {
@JvmStatic
val EMPTY = Environment(emptyList(), emptyList(), null)
Expand Down
30 changes: 25 additions & 5 deletions src/main/java/com/amazon/ion/impl/macro/Expression.kt
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,21 @@
* These expressions are the only ones that may be the output from the macro evaluator.
* All [DataModelExpression]s are also valid to use as [TemplateBodyExpression]s and [EExpressionBodyExpression]s.
*/
sealed interface DataModelExpression : Expression, EExpressionBodyExpression, TemplateBodyExpression
sealed interface DataModelExpression : Expression, EExpressionBodyExpression, TemplateBodyExpression, ExpansionOutputExpression

/** Output of a macro expansion (internal to the macro evaluator) */
sealed interface ExpansionOutputExpressionOrContinue
/** Output of the macro evaluator */
sealed interface ExpansionOutputExpression : ExpansionOutputExpressionOrContinue

/**
* 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
Comment on lines +62 to +69
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ 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.


/**
* Interface for expressions that are _values_ in the Ion data model.
Expand All @@ -69,6 +83,8 @@

/**
* A temporary placeholder that is used only while a macro or e-expression is partially compiled.
*
* TODO: See if we can get rid of this by e.g. using nulls during macro compilation.
*/
object Placeholder : TemplateBodyExpression, EExpressionBodyExpression

Expand Down Expand Up @@ -221,21 +237,25 @@
*/
data class VariableRef(val signatureIndex: Int) : TemplateBodyExpression

sealed interface InvokableExpression : HasStartAndEnd, Expression {

Check warning on line 240 in src/main/java/com/amazon/ion/impl/macro/Expression.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L240 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ 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.

val macro: Macro
}

Check warning on line 242 in src/main/java/com/amazon/ion/impl/macro/Expression.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L242 was not covered by tests

/**
* A macro invocation that needs to be expanded.
*/
data class MacroInvocation(
val macro: Macro,
override val macro: Macro,
override val selfIndex: Int,
override val endExclusive: Int
) : TemplateBodyExpression, HasStartAndEnd
) : TemplateBodyExpression, HasStartAndEnd, InvokableExpression

/**
* An e-expression that needs to be expanded.
*/
data class EExpression(
val macro: Macro,
override val macro: Macro,
override val selfIndex: Int,
override val endExclusive: Int
) : EExpressionBodyExpression, HasStartAndEnd
) : EExpressionBodyExpression, HasStartAndEnd, InvokableExpression
}
Loading
Loading