Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Container tooling #885

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Container tooling #885

wants to merge 10 commits into from

Conversation

zslayton
Copy link
Contributor

@zslayton zslayton commented Dec 19, 2024

This PR adds lots of new tooling methods and bookkeeping:

  • Tags each value with the variable that produced it, if any.
  • Renames UnexpandedField to FieldExpr, which is much clearer.
  • Adds container iterators that yield both macro invocations and their expansions, allowing tooling to see both and work with whatever they need.
  • Adds Span accessor for the delimited END byte.

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

@zslayton zslayton marked this pull request as ready for review December 19, 2024 22:24
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 57.64706% with 180 lines in your changes missing coverage. Please review.

Project coverage is 77.62%. Comparing base (4e0d272) to head (b738f70).

Files with missing lines Patch % Lines
src/lazy/expanded/struct.rs 61.46% 51 Missing and 28 partials ⚠️
src/lazy/expanded/sequence.rs 70.37% 2 Missing and 30 partials ⚠️
src/lazy/expanded/macro_evaluator.rs 61.90% 15 Missing and 1 partial ⚠️
src/lazy/expanded/template.rs 25.00% 15 Missing ⚠️
src/lazy/binary/raw/value.rs 0.00% 13 Missing ⚠️
src/lazy/decoder.rs 35.29% 11 Missing ⚠️
src/lazy/any_encoding.rs 0.00% 7 Missing ⚠️
src/lazy/binary/raw/v1_1/value.rs 0.00% 3 Missing ⚠️
src/lazy/text/value.rs 0.00% 3 Missing ⚠️
src/lazy/expanded/mod.rs 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #885      +/-   ##
==========================================
- Coverage   77.85%   77.62%   -0.24%     
==========================================
  Files         136      136              
  Lines       34670    35038     +368     
  Branches    34670    35038     +368     
==========================================
+ Hits        26994    27199     +205     
- Misses       5680     5789     +109     
- Partials     1996     2050      +54     

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

Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ PR Tour 🧭

@@ -1047,6 +1048,15 @@ impl<'top> LazyRawValue<'top, AnyEncoding> for LazyRawAnyValue<'top> {
}
}

fn is_delimited(&self) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 Each encoded value kind can now report whether it is delimited. For text 1.0 and binary 1.0, this method just returns false.

Comment on lines +229 to +240
fn delimited_end_span(&self) -> Span<'top> {
let bytes = self.span().bytes();
let end = bytes.len();
let range = if !self.is_delimited() {
end..end
} else {
debug_assert!(bytes[end - 1] == 0xF0);
end - 1..end
};
let end_bytes = bytes.get(range).unwrap();
Span::with_offset(self.range().end - end_bytes.len(), end_bytes)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 For scalars and length-prefixed containers, this returns an empty slice.

EExp(eexp) => FieldExpr::EExp(eexp.resolve(context)?),
};
Ok(field)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 A convenience method pattern used widely throughout the crate; resolve turns a raw value (or in this case, field) into its higher-level, context-ful equivalent.

Comment on lines -398 to +417
/// Creates an iterator that converts each raw struct field into an `UnexpandedField`, a
/// Creates an iterator that converts each raw struct field into an `FieldExpr`, a
/// common representation for both raw fields and template fields that is used in the
/// expansion process.
fn unexpanded_fields(
fn field_exprs(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 UnexpandedField was a clunky name, hopefully this is better. The new relationship between LazyExpandedField and FieldExpr mirrors the existing relationship between LazyExpandedValue and ValueExpr.

if input.len() < flex_int_len {
if input.len() <= flex_int_len {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This bug surfaced while I was working on an example file from @jobarr-amzn. This fixed it.

variable: Option<TemplateVariableReference<'top>>,
pub(crate) variable: Option<TemplateVariableReference<'top>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 Macros now remember the variable (if any) that produced them, allowing them in turn to tag the values they produce.

Comment on lines +210 to +213
/// Like an [`ExpandedListIterator`], but will yield macro invocations before also yielding
/// that macro's expansion.
#[derive(Debug)]
pub struct ListValueExprIterator<'top, D: Decoder> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This section introduces iterators that return both the expressions and the values they expand to.

Comment on lines +729 to +732
impl<'top, D: Decoder> Iterator for FieldExprIterator<'top, D> {
type Item = IonResult<FieldExpr<'top, D>>;

fn next(&mut self) -> Option<Self::Item> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This is pretty close to the regular struct iterator, expect that any time we would change states and keep reading, we now return the expression that caused us to change states. The following call to next() will resume reading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant