Adds high-level Writer API for defining macros #938

zslayton · 2025-03-13T12:53:07Z

PR tour forthcoming, please stand by until this is no longer in draft mode.

Related to #920. cc @barries

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

…ames to be used for writing E-exps.

codecov · 2025-03-13T12:54:57Z

Codecov Report

Attention: Patch coverage is 61.96013% with 229 lines in your changes missing coverage. Please review.

Project coverage is 78.66%. Comparing base (bbcf8cb) to head (7a2f304).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lazy/expanded/macro_table.rs	68.92%	48 Missing and 30 partials ⚠️
src/lazy/text/raw/v1_1/reader.rs	21.68%	64 Missing and 1 partial ⚠️
src/lazy/encoder/writer.rs	67.67%	4 Missing and 28 partials ⚠️
src/lazy/expanded/compiler.rs	73.77%	5 Missing and 11 partials ⚠️
src/lazy/encoder/binary/v1_1/container_writers.rs	0.00%	8 Missing ⚠️
src/lazy/binary/raw/v1_1/struct.rs	20.00%	0 Missing and 4 partials ⚠️
src/lazy/encoder/text/v1_1/value_writer.rs	89.74%	3 Missing and 1 partial ⚠️
src/lazy/expanded/template.rs	78.94%	4 Missing ⚠️
src/lazy/never.rs	0.00%	4 Missing ⚠️
src/lazy/binary/raw/v1_1/binary_buffer.rs	0.00%	0 Missing and 3 partials ⚠️
... and 6 more

❌ Your patch status has failed because the patch coverage (61.96%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #938      +/-   ##
==========================================
- Coverage   78.76%   78.66%   -0.11%     
==========================================
  Files         137      137              
  Lines       34739    35203     +464     
  Branches    34739    35203     +464     
==========================================
+ Hits        27363    27691     +328     
- Misses       5378     5454      +76     
- Partials     1998     2058      +60

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zslayton

🗺️ PR Tour 🧭

zslayton · 2025-03-13T12:58:10Z

examples/write_log_events.rs

 //! This program demonstrates implementing WriteAsIon using Ion 1.1's e-expressions for a more
-//! compact encoding. It uses raw-level writer APIs that end users are unlikely to leverage.
-//! Ion 1.1 is not yet finalized; the encoding produced by this example and the APIs it uses
-//! are very likely to change.
-
+//! compact encoding.


🪧 Happily, this example that ended up at the top of the diff showcases the new functionality.

zslayton · 2025-03-13T13:00:53Z

examples/write_log_events.rs

 use ion_rs::*;

 fn main() -> IonResult<()> {
    #[cfg(not(feature = "experimental"))]
-    panic!("This example requires the 'experimental' feature to work.");
+    {
+        eprintln!("This example requires the 'experimental' feature to work. Rebuild it with the flag `--features experimental`.");


🪧 This was originally a panic! to make it clear that the example had failed to run. However, the boilerplate output text from the panic! obscured the message explaining what had gone wrong. Now it just prints a (more detailed) explanation to STDERR.

zslayton · 2025-03-13T13:43:47Z

examples/write_log_events.rs

-    // IDs and Ion 1.1 refers to them as part of a macro. In both cases, however, the encoding
-    // context is not written out in the resulting Ion stream.
-    // TODO: Include the symbol/macro table definitions in the resulting output stream.
-    #[allow(dead_code)]


🪧 This example no longer uses the raw writer to encode symbol IDs directly. The text literals are now provided to the managed Writer as would happen in most applications.

zslayton · 2025-03-13T13:45:41Z

src/constants.rs

@@ -124,6 +124,7 @@ pub(crate) mod v1_1 {

        pub const ION: SystemSymbol_1_1 = SystemSymbol_1_1::new_unchecked(1);
        pub const ENCODING: SystemSymbol_1_1 = SystemSymbol_1_1::new_unchecked(10);
+        pub const MACRO_TABLE: SystemSymbol_1_1 = SystemSymbol_1_1::new_unchecked(14);


🪧 The Writer can now emit a directive to append macros to the macro table. This system symbol is used in that directive.

zslayton · 2025-03-13T13:49:58Z

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

@@ -1311,7 +1311,7 @@ mod tests {
    ) -> IonResult<()> {
        let mut context = EncodingContext::for_ion_version(IonVersion::v1_1);
        let template_macro =
-            TemplateCompiler::compile_from_source(context.get_ref(), macro_source)?;
+            TemplateCompiler::compile_from_source(context.macro_table(), macro_source)?;


🪧 Previously, the TemplateCompiler would pass around an EncodingContextRef that was used to resolve macros being invoked in the provided TDL source. However, the EncodingContextRef is specific to the Reader. (The Writer has an analogous construct that is optimized for different operations.) This PR modifies the TemplateCompiler to pass around a &MacroTable instead of an EncodingContext. Both the Reader and Writer can easily provide a &MacroTable, and that was the only field in the EncodingContextRef that was being used anyway.

zslayton · 2025-03-13T14:10:57Z

src/lazy/encoder/writer.rs

@@ -92,6 +96,46 @@ impl<E: Encoding, Output: Write> Writer<E, Output> {
        Ok(writer)
    }

+    pub fn compile_macro(&mut self, source: impl IonInput) -> IonResult<Macro> {
+        let mut reader = Reader::new(AnyEncoding, source)?;


🪧 Eventually I'd like to optimize this by either accepting bulk macro definitions, re-using the Reader, or just offering a programmatic API for constructing a macro.

zslayton · 2025-03-13T14:12:01Z

src/lazy/encoder/writer.rs

+        // TODO: Once expression group serialization is complete, this can be replaced by a call
+        //       to the `add_macros` system macro.


🪧 Binary expr group writing needs to be functional before we can do this.

zslayton · 2025-03-13T14:13:57Z

src/lazy/expanded/compiler.rs

@@ -216,14 +216,14 @@ impl TemplateCompiler {
    /// to the template without interpretation. `(literal ...)` does not appear in the compiled
    /// template as there is nothing more for it to do at expansion time.
    pub fn compile_from_source(
-        context: EncodingContextRef<'_>,
+        active_macros: &MacroTable,


🪧 All changes in this file are either:

Replacing the previously-passed-around EncodingContextRef with a &MacroTable.
OR

Mechanical renaming of Macro (which is now a user-facing handle) to MacroDef (which continues to not be user-visible).

zslayton · 2025-03-13T14:16:27Z

src/lazy/text/raw/v1_1/reader.rs

@@ -203,6 +327,33 @@ impl From<SystemMacroAddress> for MacroIdRef<'_> {
    }
 }

+#[derive(Clone, Debug, PartialEq)]
+pub enum MacroId {


🪧 An owned variant of MacroIdRef.

zslayton · 2025-03-13T14:17:55Z

src/lazy/text/raw/v1_1/reader.rs

+    fn resolve(self, _macro_table: &'a MacroTable) -> IonResult<ResolvedId<'a>> {
+        // ResolvedId is already resolved
+        Ok(self)
+    }


🪧 This is the important bit. If you pass an unresolved identifier to a raw eexp writer, it will try to resolve it. If you pass a resolved identifier to a raw eexp writer (say, from the managed writer that already resolved it), it won't need to resolve it again.

popematt

I've only quickly scanned the PR. I'll take a closer look later. I had a few drive-by thoughts though.

popematt · 2025-03-14T16:02:16Z

src/lazy/text/raw/v1_1/reader.rs

+    LocalName(CompactString),
+    LocalAddress(usize),
+    SystemAddress(SystemMacroAddress),
+    // TODO: Qualified names and addresses


What about system name? (Or is that part of qualified name?)

Because the current impl doesn't have multi-module support, System is a special case. Later, it will be supplanted by QualifiedAddress and (as you mention) QualifiedName.

popematt · 2025-03-14T17:59:49Z

examples/write_log_events.rs

+        let mut ion_writer = Writer::new(v1_1::Binary, BufWriter::new(ion_1_1_file.as_file()))?;
+
+        // ...then we define some macros that we intend to use to encode our log data...
+        let macros = LogEventMacros {


I think this example would be clearer without having a custom struct here. I was initially confused by this.

I can do that, but it means passing two &Macro references later on instead of one "all-the-macros" reference. I don't feel strongly either way.

popematt · 2025-03-27T23:28:36Z

src/lazy/encoder/writer.rs

        Ok(ApplicationEExpWriter::new(
            self.encoding,
            self.value_writer_config,
-            self.raw_value_writer.eexp_writer(macro_id)?,
+            self.raw_value_writer.eexp_writer(macro_id)?, // HERE


What is HERE—leftover comment?

* Adds `lazzy-soucre-location` feature flag * Adds `row` and `prev_newline_offset` for `IonSlice` and `IonStream` * Adds unit tests for `IonSlice` and `IonStream`

zslayton added 7 commits March 4, 2025 13:08

Rename Macro -> MacroDef

4f426a0

Stubs out ExprGroupWriter, impl WriteAsIon for TemplateMacroRef

82a6263

Enables Writer to compile macros and add them to the default module

58508d9

Introduces MacroIdLike trait that allows &Macro, addresses, and n…

97123af

…ames to be used for writing E-exps.

Updates write_log_events to use managed writer

2f24735

Improves documentation for the write_log_events example

323c0e7

cleanup, clippy suggestions

d5241f3

zslayton added 4 commits March 13, 2025 09:42

Example cleanup

b2b29d7

Disable binary expr group writing for now

f5c848d

Adds doc comment

12ad98c

Adds doc comment to ResolvedId

a941b5d

zslayton commented Mar 13, 2025

View reviewed changes

zslayton marked this pull request as ready for review March 13, 2025 14:18

zslayton requested review from popematt and jobarr-amzn March 13, 2025 14:18

zslayton mentioned this pull request Mar 13, 2025

Macros ready for experimental use? #920

Open

popematt reviewed Mar 14, 2025

View reviewed changes

Merge branch 'main' into register-macros

30d504c

zslayton requested a review from popematt March 18, 2025 15:42

popematt approved these changes Mar 27, 2025

View reviewed changes

zslayton and others added 6 commits March 31, 2025 10:15

Removes old comment

b36f05f

Adds location metadata to LazyValue and unit tests (#941)

6ac726a

* Adds `lazzy-soucre-location` feature flag * Adds `row` and `prev_newline_offset` for `IonSlice` and `IonStream` * Adds unit tests for `IonSlice` and `IonStream`

Implement AsRef<Element> for Element

0535440

Cargo fmt fixes

8615e63

clippy suggestion

8d1573c

Merge remote-tracking branch 'origin/main' into register-macros

7a2f304

zslayton merged commit 7298b5b into main Mar 31, 2025
34 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds high-level Writer API for defining macros #938

Adds high-level Writer API for defining macros #938

zslayton commented Mar 13, 2025

codecov bot commented Mar 13, 2025 •

edited

Loading

zslayton left a comment

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

zslayton Mar 13, 2025

popematt left a comment

popematt Mar 14, 2025

zslayton Mar 17, 2025

popematt Mar 14, 2025

zslayton Mar 17, 2025

popematt Mar 27, 2025

		// TODO: Once expression group serialization is complete, this can be replaced by a call
		// to the `add_macros` system macro.

Adds high-level Writer API for defining macros #938

Adds high-level Writer API for defining macros #938

Conversation

zslayton commented Mar 13, 2025

codecov bot commented Mar 13, 2025 • 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

popematt 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

codecov bot commented Mar 13, 2025 •

edited

Loading