Skip to content

Conversation

nirosys
Copy link
Contributor

@nirosys nirosys commented Aug 6, 2025

Note: This PR includes #991 & #992.

Issue #, if available: #943

Description of changes:
Prior to this PR (as of #991) supported tagless encoding was limited to uint8. This PR extends the unsigned int support to the remaining uint types by implementing uint16, uint32 and uint64.

CLI Testing

Below is an inspect of an encoding and usage of the following macro:

(macro foo (uint8::a uint16::b uint32::c uint64::d)
   (.sum (.sum (%a) (%b)) (.sum (%c) (%d)))
)
$ ./target/release/ion inspect ~/Code/sandbox/tagless_output/output.10n
┌──────────────┬──────────────┬─────────────────────────┬──────────────────────┐
│    Offset    │    Length    │       Binary Ion        │       Text Ion       │
├──────────────┼──────────────┼─────────────────────────┼──────────────────────┘
│            0 │            4 │ e0 01 01 ea             │ $ion_1_1 // Version marker
├──────────────┼──────────────┼─────────────────────────┤
│            4 │            6 │ e7 f9 24 69 6f 6e       │ $ion:: // <text>
│           10 │          114 │ fc e1                   │ (
│           12 │            2 │ ee 10                   │ · module
│           14 │            2 │ a1 5f                   │ · _
│           16 │            5 │ c4                      │ · (
│           17 │            2 │ ee 0f                   │ · · symbol_table
│           19 │            2 │ a1 5f                   │ · · _
│              │              │                         │ · )
│           21 │          103 │ fc cb                   │ · (
│           23 │            2 │ ee 0e                   │ · · macro_table
│           25 │            2 │ a1 5f                   │ · · _
│           27 │           97 │ fc bf                   │ · · (
│           29 │            6 │ a5 6d 61 63 72 6f       │ · · · macro
│           35 │            4 │ a3 66 6f 6f             │ · · · foo
│           39 │           41 │ fc 4f                   │ · · · (
│           41 │            7 │ e7 f7 75 69 6e 74 38    │ · · · · uint8:: // <text>
│           48 │            2 │ a1 61                   │ · · · · a
│           50 │            8 │ e7 f5 75 69 6e 74 31 36 │ · · · · uint16:: // <text>
│           58 │            2 │ a1 62                   │ · · · · b
│           60 │            8 │ e7 f5 75 69 6e 74 33 32 │ · · · · uint32:: // <text>
│           68 │            2 │ a1 63                   │ · · · · c
│           70 │            8 │ e7 f5 75 69 6e 74 36 34 │ · · · · uint64:: // <text>
│           78 │            2 │ a1 64                   │ · · · · d
│              │              │                         │ · · · )
│           80 │           44 │ fc 55                   │ · · · (
│           82 │            2 │ a1 2e                   │ · · · · '.'
│           84 │            4 │ a3 73 75 6d             │ · · · · sum
│           88 │           18 │ fc 21                   │ · · · · (
│           90 │            2 │ a1 2e                   │ · · · · · '.'
│           92 │            4 │ a3 73 75 6d             │ · · · · · sum
│           96 │            5 │ c4                      │ · · · · · (
│           97 │            2 │ a1 25                   │ · · · · · · '%'
│           99 │            2 │ a1 61                   │ · · · · · · a
│              │              │                         │ · · · · · )
│          101 │            5 │ c4                      │ · · · · · (
│          102 │            2 │ a1 25                   │ · · · · · · '%'
│          104 │            2 │ a1 62                   │ · · · · · · b
│              │              │                         │ · · · · · )
│              │              │                         │ · · · · )
│          106 │           18 │ fc 21                   │ · · · · (
│          108 │            2 │ a1 2e                   │ · · · · · '.'
│          110 │            4 │ a3 73 75 6d             │ · · · · · sum
│          114 │            5 │ c4                      │ · · · · · (
│          115 │            2 │ a1 25                   │ · · · · · · '%'
│          117 │            2 │ a1 63                   │ · · · · · · c
│              │              │                         │ · · · · · )
│          119 │            5 │ c4                      │ · · · · · (
│          120 │            2 │ a1 25                   │ · · · · · · '%'
│          122 │            2 │ a1 64                   │ · · · · · · d
│              │              │                         │ · · · · · )
│              │              │                         │ · · · · )
│              │              │                         │ · · · )
│              │              │                         │ · · )
│              │              │                         │ · )
│              │              │                         │ )
├──────────────┼──────────────┼─────────────────────────┤
│          124 │           16 │ 18                      │ (:foo
│          125 │            1 │ 01                      │ · 1 // a
│          126 │            2 │ 02 00                   │ · 2 // b
│          128 │            4 │ 03 00 00 00             │ · 3 // c
│          132 │            8 │ 04 00 00 00 00 00 00 00 │ · 4 // d
│              │              │                         │ )
│              │              │                         │ 10
├──────────────┼──────────────┼─────────────────────────┤
│          140 │              │                         │  // End of stream
└──────────────┴──────────────┴─────────────────────────┘

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

Copy link

codecov bot commented Aug 6, 2025

Codecov Report

❌ Patch coverage is 74.64789% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.28%. Comparing base (62a7af8) to head (a1cd76a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/lazy/expanded/macro_table.rs 33.33% 0 Missing and 6 partials ⚠️
src/lazy/expanded/template.rs 53.84% 6 Missing ⚠️
src/lazy/binary/raw/v1_1/binary_buffer.rs 77.77% 2 Missing ⚠️
src/lazy/encoder/binary/v1_1/fixed_uint.rs 75.00% 0 Missing and 2 partials ⚠️
src/lazy/binary/raw/v1_1/e_expression.rs 83.33% 0 Missing and 1 partial ⚠️
src/lazy/expanded/macro_evaluator.rs 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (74.64%) 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     #993      +/-   ##
==========================================
- Coverage   78.28%   78.28%   -0.01%     
==========================================
  Files         138      138              
  Lines       34036    34093      +57     
  Branches    34036    34093      +57     
==========================================
+ Hits        26644    26688      +44     
- Misses       5329     5334       +5     
- Partials     2063     2071       +8     

☔ 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.

@nirosys
Copy link
Contributor Author

nirosys commented Aug 8, 2025

Build & Test fails due to clippy.. looks like a new rust release may have adjusted clippy lints.

@nirosys nirosys marked this pull request as ready for review August 8, 2025 21:32
Comment on lines 345 to 358
ParameterEncoding::UInt16 => {
let (fixed_uint_lazy_value, remaining) = try_or_some_err! {
self.remaining_args_buffer.read_fixed_uint_as_lazy_value(2)
};
let value_ref = &*self
.remaining_args_buffer
.context()
.allocator()
.alloc_with(|| fixed_uint_lazy_value);
(
EExpArg::new(parameter, EExpArgExpr::ValueLiteral(value_ref)),
remaining,
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of duplication here that could be cleaned up with something like this, if you change the argument type for read_fixed_uint_as_lazy_value and inverted the match statement in that function.

                ParameterEncoding::UInt8,
                ParameterEncoding::UInt16,
                ParameterEncoding::UInt32,
                ParameterEncoding::UInt64, => {
                    let (fixed_uint_lazy_value, remaining) = try_or_some_err! {
                        self.remaining_args_buffer.read_fixed_uint_as_lazy_value(parameter.encoding())
                    };
                    let value_ref = &*self
                        .remaining_args_buffer
                        .context()
                        .allocator()
                        .alloc_with(|| fixed_uint_lazy_value);
                    (
                        EExpArg::new(parameter, EExpArgExpr::ValueLiteral(value_ref)),
                        remaining,
                    )
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I implemented it slightly different though. I didn't want to bring the ParamaterEncoding into the raw binary reader (where read_fixed_uint_as_lazy_value lives) since it would leak a higher level template type into the low level reader, so I added a TryFrom impl to convert ParameterEncoding into BinaryValueEncoding, which is a nearly mirrored enum (minus macro shape). The rest is just as your suggestion.

@nirosys
Copy link
Contributor Author

nirosys commented Sep 4, 2025

Added a new commit to address the clippy warnings, not sure if that's why the merge button got disabled or not.

@nirosys nirosys merged commit fcc7eb1 into amazon-ion:main Sep 8, 2025
28 of 29 checks passed
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.

3 participants