Adds initial guard rails to the ApplicationEExpWriter
#965
Conversation
Because new symbols can be added to the symbol table at any depth, each value writer needs to hold a mutable reference to the symbol table. Because the ApplicationEExpWriter needs to hold information about the macro being invoked, it needs to hold a shared reference to a macro in the macro table. These two requirements conflict if the symbol and macro tables both reside in the same structure. This commit splits the unified `WriterContext` into separate references.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
==========================================
+ Coverage 78.71% 78.90% +0.19%
==========================================
Files 138 138
Lines 35195 35277 +82
Branches 35195 35277 +82
==========================================
+ Hits 27703 27835 +132
+ Misses 5423 5368 -55
- Partials 2069 2074 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| fn eexp_writer<'a>(self, macro_id: impl MacroIdLike<'a>) -> IonResult<Self::EExpWriter> | ||
| where | ||
| Self: 'a; |
There was a problem hiding this comment.
🪧 I had to tweak the signature of this method to communicate that the returned value will outlive the provided identifier.
| }; | ||
|
|
||
| pub(crate) struct WriterContext { |
There was a problem hiding this comment.
🪧 The symbol table can be modified by the user at any depth by writing a new symbol, annotation, or field name. This required value writer types to pass around a &mut WriterContext to make updates as needed.
In contrast, the macro table can only be modified at the top level, between values. When the ApplicationEExpWriter goes to store a shared reference to the invoked macro, it causes a conflict. There can't simultaneously be a &mut WriterContext and a &MacroDef that points inside the WriterContext. Therefore, I've split the WriterContext into a WriterSymbolTable and WriterMacroTable that can be managed separately. Each new type is a thin wrapper around SymbolTable/MacroTable and does some minor bookkeeping each time the underlying table is modified.
| let address = self | ||
| .context | ||
| .macro_table | ||
| .add_template_macro(template_macro)?; | ||
| self.context.num_pending_macros += 1; |
There was a problem hiding this comment.
🪧 The new Writer_____Table types make the code a bit less verbose.
| symbols: &'a mut WriterSymbolTable, | ||
| macros: &'a WriterMacroTable, |
There was a problem hiding this comment.
🪧 Notice that one of these is &mut and the other is simply &.
| let resolved_id = macro_id.resolve(self.macros)?; | ||
| let macro_ref = self | ||
| .macros | ||
| .macro_at_address(resolved_id.address()) | ||
| .expect("just resolved"); |
There was a problem hiding this comment.
🪧 This method now resolves the provided macro ID up front.
| // TODO: these are now available but not yet used. | ||
| _invoked_macro: &'value MacroDef, | ||
| _param_index: usize, |
There was a problem hiding this comment.
🪧 The next PR will add validation for each argument-writing method call that happens next.
| @@ -843,7 +843,7 @@ impl TemplateCompiler { | |||
|
|
|||
| /// Adds a `lazy_sexp` that has been determined to represent a macro invocation to the | |||
| /// TemplateBody. | |||
| fn compile_macro<'top, D: Decoder>( | |||
| fn compile_macro_invocation<'top, D: Decoder>( | |||
There was a problem hiding this comment.
🪧 I renamed this method for clarity. The entire module is dedicated to compiling macros. This particular method is compiling a TDL macro invocation within a template.
| let fragments_str = String::from_utf8(bytes).expect("Invalid input string generated"); | ||
| assert_eq!( | ||
| fragments_str, | ||
| "$ion_1_1 $ion::(module _ (macro_table (macro m (v '!' ) ('%' v ) ) ) ) (:m 1)" | ||
| .to_string(), | ||
| ); |
There was a problem hiding this comment.
🪧 This test was doing a string-equality check that started failing because of the workaround I added later. I changed the test to use Ion stream equality instead.
| buffer = writer.close()?; | ||
| Ok(buffer) | ||
| } | ||
|
|
||
| fn write<E: ion_rs::Encoding, O: std::io::Write>( | ||
| fn write<E: Encoding, O: std::io::Write>( |
There was a problem hiding this comment.
🪧 The comment below explains the problem I encountered and the workaround that's now in place.
zslayton
requested review from
jobarr-amzn and
popematt
and removed request for
jobarr-amzn
| ) | ||
| .expect("valid Ion"); | ||
| let actual_sequence = Element::read_all(bytes).expect("Writer must generate valid Ion."); | ||
| assert!(IonData::from(expected_sequence).eq(&IonData::from(actual_sequence))) |
There was a problem hiding this comment.
FYI—IonData has an associated function eq that can be used to compare anything that implements IonEq (which is also the requirement for Into<IonData>).
assert!(IonData::eq(expected_sequence, actual_sequence))Or, you could do
| assert!(IonData::from(expected_sequence).eq(&IonData::from(actual_sequence))) | |
| assert_eq!(IonData::from(expected_sequence), IonData::from(actual_sequence)) |
The
ApplicationEExpWriteris intended to be the common validation layer for user interactions with the underlyingEncoding::RawEExpWritertype. It resolves the provided ID to a macro, then verifies that each argument aligns with that macro's signature. This PR begins the process of implementing that.Specifically, it implements the initial macro ID resolution, raising an error if/when the user attempts to write an e-expression with an unrecognized ID. This caused several conformance tests to break because the
Writeris asked to emit macro definitions that it does not have in its own encoding context. I have added a workaround for this that I'll highlight in the PR tour. We can implement a more robust fix down the road.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.