Add stubs for macro evaluator #923
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #923 +/- ##
==================================================
Coverage ? 69.52%
Complexity ? 6589
==================================================
Files ? 189
Lines ? 26057
Branches ? 4691
==================================================
Hits ? 18115
Misses ? 6537
Partials ? 1405 ☔ View full report in Codecov by Sentry. |
| @@ -0,0 +1,15 @@ | |||
| package com.amazon.ion.impl.macro | |||
There was a problem hiding this comment.
| package com.amazon.ion.impl.macro | |
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |
| // SPDX-License-Identifier: Apache-2.0 | |
| package com.amazon.ion.impl.macro |
| ExactlyOne('!'), | ||
| OneOrMore('+'), | ||
| ZeroOrMore('*'); | ||
| enum class ParameterCardinality(@JvmField val sigil: Char, @JvmField val canBeVoid: Boolean, @JvmField val canBeMulti: Boolean) { |
There was a problem hiding this comment.
🗺️ Added these because as I was working on the macro evaluator, I came across situations where I wanted to simply check "can this be void" or "can this be multiple expressions".
| is Expression.BigIntValue -> intExpression.value | ||
| } | ||
|
|
||
| override fun doubleValue(): Double = (currentValueExpression as Expression.FloatValue).value |
There was a problem hiding this comment.
🗺️
Error handling is inconsistent with other [IonReader] implementations
This is one specific instance here. If the reader is on e.g. a StringValue expression, and you call doubleValue(), this will throw a ClassCastException. This will get fixed up in subsequent PRs once I have the macro evaluator and I'm able to more easily run tests on this class.
| MacroInvocation(ByName("values"), startInclusive = 1, endInclusive = 2), | ||
| Variable(0), | ||
| SExpValue(emptyList(), startInclusive = 3, endInclusive = 5), | ||
| MacroInvocation(ByName("values"), selfIndex = 0, endExclusive = 6), |
There was a problem hiding this comment.
🗺️ It's not easily noticeable in the diff (because the file was renamed), but I changed the start and end fields in the list, sexp, etc. expressions. I found that I needed the exclusive end rather than the inclusive end in the macro evaluator, and I also found that the "start" I needed was actually startInclusive + 1, so I renamed startInclusive to be selfIndex (since it is the index of this expression instance), and redefined startInclusive to specifically refer to the children of the expression.
(ktlint complained that Expression should be in a file named Expression.kt, otherwise I probably would have left it to make the diff smaller.)
| NullValue(emptyList(), IonType.NULL), | ||
| BoolValue(emptyList(), true), | ||
| IntValue(emptyList(), 1), | ||
| LongIntValue(emptyList(), 1), |
There was a problem hiding this comment.
🗺️ Another change that's not particularly noticeable in Expression.kt. I thought that LongIntValue and BigIntValue was a little more descriptive, and then I was able to use IntValue as a sealed supertype of those two.
| } | ||
|
|
||
| /** Marker interface representing expressions that can be present in E-Expressions. */ | ||
| sealed interface EncodingExpression : Expression |
There was a problem hiding this comment.
🗺️ The name EncodingExpression is a little confusing because it is not an EExpression (that's further down), but I haven't been able to come up with a better name yet.
There was a problem hiding this comment.
EExpressionBodyExpression
I like it.
There was a problem hiding this comment.
I think this basically looks good. A few questions:
- Can you provide more detail about the relationship between the MacroExpressionizer and the MacroEvaluator? How will the two interact in order to expand an invocation? Do both of them interact with a lower-level IonReader or cursor, or just the Expressionizer?
- Where is the multiplexing between the encoding and the template body performed?
- It sounds like the MacroExpressionizer is supposed to recursively handle the case where an argument is an e-expression. Does it return a list containing all e-expression arguments at all depths?
The "expressionizer" is somewhat of a placeholder because it is too much for me to, at the same time, implement lazily evaluated containers. Essentially...
My expectation is that all of this logic will eventually have to move into I've also kept things in separate classes for now because I am hopeful that the core macro evaluator might be reusable for both binary and text.
That is performed in the evaluator.
At this point, yes. Everything is eagerly read at all depths. However, I do think that my approach is fundamentally compatible with iterators that are backed by lazy-parsing in the low-level reader. |
Issue #, if available:
#729, #730
Description of changes:
TemplateBodyExpressionintoExpression, a generalized form that is useful for macro evaluationMacroCompilerto use the correct syntax for expression groupsMacroEvaluator,MacroExpressionizer(not a great name, I know), and an example of how to bridge between the macro evaluator and anIonReader.There are some comments in the PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.