Overhauls system macro table bootstrapping #872

zslayton · 2024-12-06T11:11:25Z

This patch modifies EncodingContext to have an empty macro table when constructed for use in Ion 1.0 readers.

This allows the Ion 1.0 reader to be used in the process of bootstrapping the Ion 1.1 system macro table, which was previously not possible due to the (now eliminated) circular dependency between macros and readers.

System macros that can be expressed in TDL are now constructed from a template definition encoded in Ion 1.0. As a result, the system macro table initialization logic is now substantially easier to read and more straightforward to modify.

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

This patch modifies `EncodingContext` to have an empty macro table when constructed for use in v1.0 readers. This allows the Ion 1.0 reader to be used in the process of bootstrapping the Ion 1.1 system macro table, which was previously not possible due to the (not eliminated) circular dependency between macros and readers. System macros that can be expressed in TDL are now constructed from a template definition encoded in Ion 1.0. As a result, the system macro table initialization logic is now substantially easier to read and more straightforward to modify.

codecov · 2024-12-06T11:14:51Z

Codecov Report

Attention: Patch coverage is 89.38776% with 26 lines in your changes missing coverage. Please review.

Project coverage is 77.62%. Comparing base (4131347) to head (73d1f1b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lazy/expanded/compiler.rs	76.92%	5 Missing and 13 partials ⚠️
src/lazy/binary/raw/v1_1/immutable_buffer.rs	66.66%	0 Missing and 1 partial ⚠️
src/lazy/encoder/text/v1_1/writer.rs	0.00%	0 Missing and 1 partial ⚠️
src/lazy/expanded/macro_evaluator.rs	75.00%	0 Missing and 1 partial ⚠️
src/lazy/expanded/macro_table.rs	99.29%	1 Missing ⚠️
src/lazy/expanded/mod.rs	87.50%	1 Missing ⚠️
src/lazy/reader.rs	80.00%	0 Missing and 1 partial ⚠️
src/lazy/system_reader.rs	50.00%	0 Missing and 1 partial ⚠️
src/lazy/text/raw/v1_1/reader.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #872      +/-   ##
==========================================
- Coverage   77.91%   77.62%   -0.30%     
==========================================
  Files         136      136              
  Lines       34401    34092     -309     
  Branches    34401    34092     -309     
==========================================
- Hits        26805    26465     -340     
- Misses       5630     5659      +29     
- Partials     1966     1968       +2

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

zslayton

🗺️ PR Tour 🧭

zslayton · 2024-12-06T11:15:01Z

benches/read_many_structs.rs

-        let compiled_macro = TemplateCompiler::compile_from_text(
+        let compiled_macro = TemplateCompiler::compile_from_source(


🪧 The compile_from_text method previously required Ion 1.1. It now works with any Ion encoding; when used during bootstrapping we only pass it Ion 1.0.

zslayton · 2024-12-06T11:18:40Z

src/lazy/binary/raw/v1_1/immutable_buffer.rs

@@ -1317,8 +1317,9 @@ mod tests {
        encode_macro_fn: impl FnOnce(MacroAddress) -> Vec<u8>,
        test_fn: impl FnOnce(BinaryEExpArgsIterator_1_1<'_>) -> IonResult<()>,
    ) -> IonResult<()> {
-        let mut context = EncodingContext::empty();
-        let template_macro = TemplateCompiler::compile_from_text(context.get_ref(), macro_source)?;
+        let mut context = EncodingContext::for_ion_version(IonVersion::v1_1);


🪧 At first, Ion 1.0 and 1.1 readers would use the same EncodingContext. Later, EncodingContext was modified to be able to take an Ion version, allowing it to initialize the symbol table differently to accommodate Ion 1.1's new symtab. This PR continues that differentiation, causing 1.0 encoding contexts to be initialized with an empty macro table.

zslayton · 2024-12-06T11:19:14Z

src/lazy/expanded/compiler.rs

@@ -50,6 +54,17 @@ impl ExpansionAnalysis {
        }
    }

+    /// Returns an expansion analysis for a macro that will produce a single value that may or may
+    /// not be a system value.
+    pub const fn possible_system_value() -> Self {


🪧 I added a few more named ExpansionAnalysis constructors so their intent can be expressed at a high level.

zslayton · 2024-12-06T11:20:36Z

src/lazy/expanded/compiler.rs

@@ -146,7 +174,7 @@ pub struct TemplateCompiler {}

 impl TemplateCompiler {
    /// Takes a TDL expression in the form:
-    /// ```ion_1_1
+    /// ```ion


The rustdoc renderer doesn't differentiate between these two identifiers, but now that the template compiler is Ion-version-agnostic, I wanted this fence to be too.

zslayton · 2024-12-06T11:21:22Z

src/lazy/expanded/compiler.rs

-            "$ion" => context
-                .system_module
-                .macro_table()
-                .clone_macro_with_id(macro_id),
+            "$ion" => ION_1_1_SYSTEM_MACROS.clone_macro_with_id(macro_id),


🪧 System macro resolution now happens in the singleton instance.

zslayton · 2024-12-06T11:30:03Z

src/lazy/expanded/macro_table.rs

            Arc::new(Macro::named(
+                name,
+                TemplateCompiler::compile_signature(context, signature).unwrap(),


🪧 Using the previously defined helper method, we can now compile the signature from source instead of manually instantiating a Vec of Parameters.

zslayton · 2024-12-06T11:32:01Z

src/lazy/expanded/macro_table.rs

+            template(
+                r#"
+                        (macro set_symbols (symbols*)
+                             $ion_encoding::(
+                               // Set a new symbol table
+                               (symbol_table [(%symbols)])
+                               // Include the active encoding module macros
+                               (macro_table $ion_encoding)
+                             )
+                       )
+                    "#,
+            ),


🪧 My next task is to switch over to the default module syntax, which means redefining these system template macros. That job will now be much easier--I can just revise this Ion 1.0 text.

Wonderful, this is such a nice change.

zslayton · 2024-12-06T11:32:41Z

src/lazy/expanded/macro_table.rs

@@ -827,4 +522,10 @@ impl MacroTable {
        }
        Ok(())
    }
+
+    pub(crate) fn reset_to_system_macros(&mut self) {


🪧 Used (for now) after encountering an Ion 1.1 IVM.

zslayton · 2024-12-06T11:33:07Z

src/lazy/expanded/mod.rs

@@ -182,22 +176,14 @@ impl<'top> EncodingContextRef<'top> {
        &self.context.macro_table
    }

-    pub fn system_symbol_table(&self) -> &'top SymbolTable {


🪧 These are no longer needed; we have the singleton.

zslayton · 2024-12-06T11:35:01Z

src/lazy/reader.rs

@@ -287,20 +286,18 @@ mod tests {
    fn expand_macro_test(
        macro_source: &str,
        encode_macro_fn: impl FnOnce(MacroAddress) -> Vec<u8>,
-        test_fn: impl FnOnce(Reader<AnyEncoding, &[u8]>) -> IonResult<()>,
+        test_fn: impl FnOnce(Reader<v1_1::Binary, &[u8]>) -> IonResult<()>,


🪧 This test was written before encoding directives were implemented. It manually registers a macro definition with the reader instead. Now that 1.0 readers don't have macros, it needs to be limited to working with 1.1 data.

zslayton commented Dec 6, 2024

View reviewed changes

zslayton marked this pull request as ready for review December 6, 2024 11:35

zslayton requested review from jobarr-amzn, popematt and tgregg December 6, 2024 11:36

zslayton mentioned this pull request Dec 6, 2024

Switches from "encoding module" to "default module" #873

Merged

popematt approved these changes Dec 6, 2024

View reviewed changes

zslayton merged commit c103ca3 into main Dec 6, 2024
35 checks passed

zslayton deleted the bootstrap-macros branch December 6, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhauls system macro table bootstrapping #872

Overhauls system macro table bootstrapping #872

zslayton commented Dec 6, 2024

codecov bot commented Dec 6, 2024 •

edited

Loading

zslayton left a comment

zslayton Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

jobarr-amzn Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

zslayton Dec 6, 2024

		let compiled_macro = TemplateCompiler::compile_from_text(
		let compiled_macro = TemplateCompiler::compile_from_source(

Overhauls system macro table bootstrapping #872

Overhauls system macro table bootstrapping #872

Conversation

zslayton commented Dec 6, 2024

codecov bot commented Dec 6, 2024 • 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

codecov bot commented Dec 6, 2024 •

edited

Loading