Enables dependent macros #822
Conversation
| /// to a [`MacroTable`](crate::lazy::expanded::macro_table::MacroTable). | ||
| /// to a [`MacroTable`]. |
There was a problem hiding this comment.
🗺️ This explicit path was made redundant because this module now imports MacroTable.
| // TODO: This is a rudimentary implementation that panics instead of performing thorough | ||
| // validation. Where it does surface errors, the messages are too terse. | ||
| // TODO: This is a rudimentary implementation that surfaces terse errors. |
There was a problem hiding this comment.
🗺️ Updated this comment as the impl hasn't used panics in a while.
| fn compile_value<'top, D: Decoder>( | ||
| context: EncodingContextRef<'top>, | ||
| signature: &MacroSignature, | ||
| fn compile_value<D: Decoder>( | ||
| tdl_context: TdlContext, |
There was a problem hiding this comment.
🗺️ Rather than passing a context, signature, and (now) pending_macros to the compiler for each subexpression, we now pass a single TdlContext (defined below on line 840). We can extend this TdlContext as needed in the future to avoid similar plumb-the-argument-through PRs.
| let address = | ||
| context.macro_table.address_for_name(name).ok_or_else(|| { | ||
| IonError::decoding_error(format!("unrecognized macro name: {name}")) | ||
| })?; | ||
| Ok((Some(name.to_string()), address)) | ||
| MacroIdRef::LocalName(name) | ||
| } else { | ||
| IonResult::decoding_error("macro names must be an identifier") | ||
| return IonResult::decoding_error("macro names must be an identifier"); |
There was a problem hiding this comment.
🗺️ Rather than performing the actual lookup of the ID here, this method now saves the ID in a variable (macro_id on line 635 above) and then does the lookup on line 665 below.
I believe that we're going to want to eventually support depending on macros in the active macro table; when we go to add that, we'll just add another .or_else(|| Self::resolve_macro_id_in(...)) call after line 665.
| @@ -5,19 +5,25 @@ use std::ops::{Deref, Range}; | |||
| use bumpalo::collections::Vec as BumpVec; | |||
| use rustc_hash::FxHashMap; | |||
|
|
|||
| use crate::{Bytes, Decimal, Int, IonResult, IonType, LazyExpandedFieldName, Str, Symbol, SymbolRef, Timestamp, try_or_some_err, Value}; | |||
There was a problem hiding this comment.
🗺️
Important
>> Everything from here on is just a formatting change done by rustfmt. <<
There are no logic changes.
zslayton
force-pushed
the
dependent-macros
branch
from
76aa86e
to
0e4c460
Compare
| // TODO: Should it be possible for a macro to reference (and therefore "close over") a macro | ||
| // in the current context? The referenced macro would not necessarily be accessible | ||
| // in the new encoding context, but would still need to be accessible internally. |
There was a problem hiding this comment.
This would be consistent with how the symbol table works, right?
There was a problem hiding this comment.
Yes, in that the active symbol table remains available while reading a new symbol table. However, symbols don't depend on other symbol definitions (you can't put $10 in the symbols list of an LST), so this is new territory in that respect.
This builds on PR #821. It also borrows from and supersedes the changes in @jobarr-amzn's PR #818.
Modifies the
TemplateCompilerto resolve macro IDs in the table of 'pending' macros (defined in the current encoding directive) rather than the active macro table. This allows macros to depend on previously defined macros.Note
cargo fmtdid some reflows that accounts for ~half of the diff. Fortunately, all of the formatting changes occur in a contiguous block at the end of the PR. I've added a PR tour comment showing where that begins.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.