Recursive macro expansion in TDL containers #647

zslayton · 2023-09-27T21:29:09Z

Builds on #645.

This PR adds the logic needed to recursively evaluate macro invocations found inside containers in TDL (the Template Definition Language).

Prior to this change, e-expressions (macro invocations in the data stream) inside containers would be recursively expanded:

[1, 2, (:values 3 4), 5, 6, (:make_string (:values foo bar) baz), 7] 

// Evaluates to:

[1, 2, 3, 4, 5, 6, "foobarbaz", 7]

but macro invocations in a TDL expression would not:

(values [1, 2, (values 3 4), 5, (void), 6])

// "Evaluates" to:

[1, 2, (values 3 4), 5, (void), 6]

with the first level of invocation (the outer (values ...) being evaluated, but invocations nested within a container (here: a list) would not.

Following this patch, TDL macro invocations found in lists and structs are evaluated correctly:

(values [1, 2, (values 3 4), 5, (void), 6])

// Evaluates to:

[1, 2, 3, 4, 5, 6]

// and

(values {a: 1, b: (values 2 3), c: (void), d: 4})

// Evaluates to:

{a: 1, b: 2, b: 3, d: 4}

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

codecov · 2023-09-27T21:31:54Z

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
src/lazy/binary/raw/sequence.rs	`47.22% <100.00%> (ø)`
src/lazy/binary/raw/struct.rs	`45.33% <100.00%> (ø)`
src/lazy/encoding.rs	`38.46% <ø> (ø)`
src/lazy/expanded/e_expression.rs	`100.00% <100.00%> (ø)`
src/lazy/expanded/tdl_macro.rs	`80.43% <100.00%> (ø)`
src/lazy/struct.rs	`70.00% <100.00%> (-0.18%)`	⬇️
src/lazy/system_reader.rs	`87.73% <ø> (-0.10%)`	⬇️
src/lazy/text/buffer.rs	`94.91% <100.00%> (ø)`
src/lazy/text/raw/sequence.rs	`68.55% <100.00%> (ø)`
src/lazy/value.rs	`80.83% <100.00%> (-1.34%)`	⬇️
... and 10 more

... and 3 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

zslayton

🗺️ PR tour

zslayton · 2023-09-28T14:28:11Z

src/lazy/any_encoding.rs

@@ -12,7 +12,7 @@ use crate::lazy::binary::raw::value::LazyRawBinaryValue;
 use crate::lazy::decoder::private::{LazyContainerPrivate, LazyRawValuePrivate};
 use crate::lazy::decoder::{
    LazyDecoder, LazyRawFieldExpr, LazyRawReader, LazyRawSequence, LazyRawStruct, LazyRawValue,
-    LazyRawValueExpr,
+    LazyRawValueExpr, RawFieldExpr, RawValueExpr,


🗺️ The enum LazyRawValueExpr<'data, D> can be either a value literal (D::Value) or a macro invocation (D::MacroInvocation). Starting with this PR, we needed the ability to talk about value expressions that might have come from templates, which are not a kind of stream encoding and so do not have a corresponding implementation D: LazyDecoder<'data>.

The new enum RawValueExpr<V, M> allows any types to be used as the value (V) and the macro invocation (M). LazyRawValueExpr<'data, D> is now a type alias for the most common kind of value expression:

pub type LazyRawValueExpr<'data, D> = RawValueExpr< <D as LazyDecoder<'data>>::Value, <D as LazyDecoder<'data>>::MacroInvocation >;

An analogous change has been made for the LazyRawFieldExpr/RawFieldExpr pair.

zslayton · 2023-09-28T14:34:09Z

src/lazy/any_encoding.rs

-                Some(Ok(LazyRawValueExpr::ValueLiteral(value))) => Some(Ok(
-                    LazyRawValueExpr::ValueLiteral(LazyRawAnyValue::from(value)),
-                )),
-                Some(Ok(LazyRawValueExpr::MacroInvocation(invocation))) => Some(Ok(
-                    LazyRawValueExpr::MacroInvocation(LazyRawAnyMacroInvocation {
+                Some(Ok(RawValueExpr::ValueLiteral(value))) => {
+                    Some(Ok(RawValueExpr::ValueLiteral(LazyRawAnyValue::from(value))))
+                }
+                Some(Ok(RawValueExpr::MacroInvocation(invocation))) => Some(Ok(
+                    RawValueExpr::MacroInvocation(LazyRawAnyMacroInvocation {


🗺️ Now that LazyRawValueExpr<'data, D> is a type alias, existing concrete uses of it would need to supply type annotations:

// Before: LazyRawValueExpr::ValueLiteral(v) // After: LazyRawValueExpr::<'data, D>::ValueLiteral(v)

However, the compiler is able to infer the correct types for RawValuExpr, to which the LazyRawValueExpr alias points. I've switched instantiations of LazyRawValueExpr to RawValueExpr for concision.

Unfortunately, that change makes up the lion's share of the line count in this diff. x_x

An analogous change has been made for the LazyRawFieldExpr/RawFieldExpr pair.

zslayton · 2023-09-28T14:40:21Z

src/lazy/expanded/macro_evaluator.rs

-pub enum ArgumentKind<'top, 'data: 'top, D: LazyDecoder<'data>, M: MacroInvocation<'data, D>> {
+pub enum ArgumentKind<'top, 'data, D: LazyDecoder<'data>, M: MacroInvocation<'data, D>> {


🗺️ In a few places, I've removed an "outlives" constraint: 'data: 'top. While it is always true throughout our API, only some methods and trait implementations "care." Anywhere else it's just line noise and--since lifetimes are one of the tougher bits of Rust to grok--probably a barrier to understanding.

zslayton · 2023-09-28T14:41:54Z

src/lazy/expanded/macro_evaluator.rs

-        M: 'data + 'top,
+        'data: 'top,
+        M: 'top,


🗺️ This is an equivalent formulation, but I liked that it had the familiar 'data: 'top constraint and emphasizes the bit that our macro invocation type will live as long as 'top.

zslayton · 2023-09-28T14:43:58Z

src/lazy/expanded/macro_evaluator.rs

-    spooky: PhantomData<(&'data D, &'data M)>,
+    spooky: PhantomData<(&'data D, M)>,


🗺️ It took way too long to realize that this throwaway &'data M added to a PhantomData was causing a lifetime constraint in the struct expander to be unsatisfiable.

Is'data strictly necessary for D? (Also, since it's Phantom, does the & matter?)

I needed 'data to appear somewhere in MacroEvaluator because it's being used in the type's trait bounds. I can leave it with D because D is guaranteed to outlive 'data, but I couldn't have it with M because the types that represent a macro invocation include 'top lifetime template values. (&'top Element)

zslayton · 2023-09-28T14:47:13Z

src/lazy/expanded/macro_evaluator.rs

 }

 impl<
        'iter,
-        'top: 'iter,
+        'top,


🗺️ Similar to an earlier change: this lifetime constraint is true, but the implementation doesn't care so it's just line noise.

zslayton · 2023-09-28T14:47:38Z

src/lazy/expanded/macro_evaluator.rs

@@ -391,7 +398,6 @@ impl<
            evaluator,
            context,
            initial_stack_depth,
-            spooky: PhantomData,


🗺️ An earlier version of this type needed PhantomData, but it no longer does.

zslayton · 2023-09-28T14:58:35Z

src/lazy/expanded/macro_evaluator.rs

+            MacroEvaluator::<'data, D, M, BumpVec<'top, MacroExpansion<D, M>>>::new_transient(
+                context,
+            );


🗺️ There's a subtle bug here. Even though the type annotations specify BumpVec<'top, _>, the new() method always constructs an evaluator backed by a Vec.

I've opened #648 to track fixing this.

zslayton · 2023-09-28T15:06:04Z

src/lazy/expanded/struct.rs

@@ -177,101 +229,212 @@ impl<'top, 'data, D: LazyDecoder<'data>> Iterator for ExpandedStructIterator<'to
            ref mut state,
        } = *self;
        match source {
-            ExpandedStructIteratorSource::ValueLiteral(evaluator, iter) => {


🗺️ This change happens to address feedback in #645.

zslayton · 2023-09-28T15:11:17Z

src/lazy/sequence.rs

@@ -50,7 +50,6 @@ use crate::{IonError, IonResult, SymbolTable};
 /// ```
 pub struct LazyList<'top, 'data, D: LazyDecoder<'data>> {
    pub(crate) expanded_list: LazyExpandedList<'top, 'data, D>,
-    pub(crate) symbol_table: &'top SymbolTable,


🗺️ The Lazy[Value|List|SExp|Struct types don't need their own SymbolTable reference; the expanded value they wrap has an EncodingContext<'top> they can use.

popematt · 2023-09-28T19:43:03Z

src/lazy/decoder.rs

+// `RawValueExpr` above has no ties to a particular encoding. The `LazyRawValueExpr` type alias
+// below uses the `Value` and `MacroInvocation` associated types from the decoder `D`. In most
+// places, this is a helpful constraint; we can talk about the value expression in terms of the
+// LazyDecoder it's associated with. However, in some places (primarily when expanding template
+// values that don't have a LazyDecoder) we need to be able to use it without constraints.


This should be moved to the doc comments of one or both of RawValueExpr and LazyRawValueExpr since it's the best explanation of why you would use one vs the other.

Good call, I'll add this to #652.

popematt · 2023-09-28T19:55:13Z

src/lazy/expanded/macro_evaluator.rs

-    spooky: PhantomData<(&'data D, &'data M)>,
+    spooky: PhantomData<(&'data D, M)>,


Is'data strictly necessary for D? (Also, since it's Phantom, does the & matter?)

zslayton added 2 commits September 25, 2023 11:29

Initial implementation of 1.1 text reader

2185cc3

Fixed private doc links

222bcb2

zslayton changed the base branch from main to 1_1-text-reader September 27, 2023 21:29

Recursive expansion of TDL containers

fb37931

zslayton force-pushed the tdl_containers branch from 453df06 to fb37931 Compare September 28, 2023 14:10

zslayton commented Sep 28, 2023

View reviewed changes

Relocate From<LazyExpandedValue> for LazyValue impls

22ca798

zslayton marked this pull request as ready for review September 28, 2023 15:13

zslayton requested a review from popematt September 28, 2023 15:13

zslayton changed the title ~~Tdl containers~~ Recursive macro expansion in TDL containers Sep 28, 2023

This was referenced Sep 28, 2023

Initial implementation of the Ion 1.1 text reader #645

Merged

Incorporates feedback from PR #645 #652

Merged

Base automatically changed from 1_1-text-reader to main September 28, 2023 19:54

Merge remote-tracking branch 'origin/main' into tdl_containers

871205e

popematt approved these changes Sep 29, 2023

View reviewed changes

zslayton merged commit d73f6bb into main Sep 29, 2023

zslayton deleted the tdl_containers branch September 29, 2023 12:29

zslayton mentioned this pull request Oct 16, 2023

Evaluating template macro invocations #657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive macro expansion in TDL containers #647

Recursive macro expansion in TDL containers #647

zslayton commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading

zslayton left a comment

zslayton Sep 28, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

popematt Sep 28, 2023

zslayton Sep 29, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

zslayton Sep 28, 2023

popematt Sep 28, 2023

zslayton Sep 29, 2023

popematt Sep 28, 2023

		pub enum ArgumentKind<'top, 'data: 'top, D: LazyDecoder<'data>, M: MacroInvocation<'data, D>> {
		pub enum ArgumentKind<'top, 'data, D: LazyDecoder<'data>, M: MacroInvocation<'data, D>> {

		spooky: PhantomData<(&'data D, &'data M)>,
		spooky: PhantomData<(&'data D, M)>,

Recursive macro expansion in TDL containers #647

Recursive macro expansion in TDL containers #647

Conversation

zslayton commented Sep 27, 2023 • edited Loading

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

zslayton left a 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

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

zslayton commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading