Implement system macro make_decimal #986

nirosys · 2025-06-26T23:09:18Z

Issue #, if available: #942

Description of changes:
This PR implements the make_decimal system macro.

Running tests: ion-tests/conformance/system_macros/make_decimal.ion
  make_decimal can be invoked                              ...  [Ok]
  the first argument must be a single, non-null integer    ...  [Ok]
  the second argument must be a single, non-null integer   ...  [Ok]
  in binary both arguments are encoded as tagged values    ...  [Ok]
  make_decimal creates a single unannotated decimal        ...  [Ok]

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

nirosys · 2025-06-26T23:11:19Z

src/lazy/expanded/macro_evaluator.rs

+
+    /// Given a [`ValueExpr`], this function will expand it into its underlying value; An
+    /// error is return if the value does not expand to exactly one Int.
+    fn get_integer(&self, env: Environment<'top, D>, value: ValueExpr<'top, D>) -> IonResult<Int> {


The need for this type of function has popped up a couple times now. Thinking about pulling it out into a standalone function, or something similarly reusable.

codecov · 2025-06-26T23:13:05Z

Codecov Report

Attention: Patch coverage is 81.44330% with 18 lines in your changes missing coverage. Please review.

Project coverage is 78.47%. Comparing base (8f03009) to head (3e068d0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lazy/expanded/macro_evaluator.rs	81.91%	1 Missing and 16 partials ⚠️
src/lazy/expanded/template.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #986   +/-   ##
=======================================
  Coverage   78.46%   78.47%           
=======================================
  Files         138      138           
  Lines       33290    33385   +95     
  Branches    33290    33385   +95     
=======================================
+ Hits        26121    26198   +77     
- Misses       5177     5179    +2     
- Partials     1992     2008   +16

☔ 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 · 2025-06-26T23:34:49Z

New rust just dropped today.. clippy lints are going wild with the enabling of uninlined format args.

nirosys · 2025-06-26T23:46:59Z

Will follow up with a PR for the clippy warnings. It's a bit much to include here, especially since they all seem unrelated to this PR.

popematt · 2025-06-30T17:21:00Z

src/lazy/expanded/macro_evaluator.rs

+        let expo_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; missing second argument"))?;
+        let exponent = self.get_integer(environment, expo_expr?)?;
+
+        let decimal = Decimal::new(coefficient, exponent.as_i64().ok_or_else(|| IonError::decoding_error("Unable to convert coefficient Int to i64"))?);


Might be worth updating this error message to say something like "coefficient is out of the range supported by this implementation" so that it's clear that this is an intentional limitation in the library.

popematt · 2025-06-30T17:21:28Z

src/lazy/expanded/macro_evaluator.rs

+    ) -> IonResult<MacroExpansionStep<'top, D>> {
+        // Arguments should be: (coefficient exponent)
+        //   Both coefficient and exponent should evaluate to a single integer value.
+        let coeff_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments"))?;


Small change to be explicit about the problem that was encountered.

Suggested change

let coeff_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments"))?;

let coeff_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; found 0 arguments"))?;

popematt · 2025-06-30T17:21:45Z

src/lazy/expanded/macro_evaluator.rs

+        let coeff_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments"))?;
+        let coefficient = self.get_integer(environment, coeff_expr?)?;
+
+        let expo_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; missing second argument"))?;


(very optional) Minor suggestion to make the message consistent with my suggested change for the previous error message.

Suggested change

let expo_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; missing second argument"))?;

let expo_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; found only 1 argument"))?;

popematt · 2025-06-30T17:25:57Z

src/lazy/expanded/macro_evaluator.rs

+                let mut evaluator = MacroEvaluator::new_with_environment(env);
+                evaluator.push(invocation.expand()?);
+                let int_arg = match evaluator.next()? {
+                    None => IonResult::decoding_error("`make_decimal` takes two integers as arguments; empty value found"),


It would be a slight improvement to indicate which parameter was problematic, and it also prepares this for being generalized as a stand-alone helper function.

format!("`make_decimal` parameter {i} must be an integer, but argument was absent")

(And similarly for the "multiple values" case.)

popematt · 2025-06-30T17:30:18Z

src/lazy/expanded/macro_evaluator.rs

@@ -3134,6 +3202,72 @@ mod tests {
        )
    }

+    #[test]
+    fn make_decimal_eexp() -> IonResult<()> {
+        stream_eq(


Minor suggestion—make each expression in the test input produce a different value so that it's obvious where something may have gone wrong.

popematt · 2025-06-30T17:31:11Z

src/lazy/expanded/macro_evaluator.rs

+    }
+
+    #[test]
+    fn make_decimal_arg_errors() -> IonResult<()> {


Do we need to test for a case like (:make_decimal 1 2 3), or is checking the number of arguments handled elsewhere?

The number of provided arguments is checked in the text parser itself, yea. I need to look though to see if the same is happening in the binary reader.

nirosys · 2025-06-30T22:50:55Z

src/lazy/expanded/macro_evaluator.rs

+
+// A simple wrapper for make_decimal's known arguments. Provides context for error reporting, and
+// functionality to expand e-exp and validate integer types.
+struct MakeDecimalArgument<'top, D: Decoder>(&'static str, ValueExpr<'top, D>);


Making this a little closer to reusable. I'll probably make it general use with the make_timestamp implementation.

What will you call it then? NamedIntegerArgument or something maybe? You'd have to add another field to name the context (macro) in the error messaging, and at that point you might as well name the fields 🤔...

jobarr-amzn · 2025-07-02T17:54:16Z

src/lazy/expanded/macro_evaluator.rs

+
+// A simple wrapper for make_decimal's known arguments. Provides context for error reporting, and
+// functionality to expand e-exp and validate integer types.
+struct MakeDecimalArgument<'top, D: Decoder>(&'static str, ValueExpr<'top, D>);


What will you call it then? NamedIntegerArgument or something maybe? You'd have to add another field to name the context (macro) in the error messaging, and at that point you might as well name the fields 🤔...

…impl

Implement system macro make_decimal

24044b3

nirosys commented Jun 26, 2025

View reviewed changes

nirosys marked this pull request as ready for review June 26, 2025 23:47

popematt reviewed Jun 30, 2025

View reviewed changes

Address PR feedback

9420be2

nirosys commented Jun 30, 2025

View reviewed changes

jobarr-amzn approved these changes Jul 2, 2025

View reviewed changes

nirosys added 2 commits July 2, 2025 13:28

Merge branch 'main' into rgiliam/make_decimal_impl

7c2f774

Merge remote-tracking branch 'origin/main' into rgiliam/make_decimal_…

3e068d0

…impl

nirosys merged commit 79d6e05 into amazon-ion:main Jul 2, 2025
29 checks passed

nirosys deleted the rgiliam/make_decimal_impl branch July 2, 2025 21:11

nirosys mentioned this pull request Jul 7, 2025

Conformance tests failing due to unimplemented system macros #942

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement system macro make_decimal #986

Implement system macro make_decimal #986

Uh oh!

nirosys commented Jun 26, 2025

Uh oh!

nirosys Jun 26, 2025

Uh oh!

codecov bot commented Jun 26, 2025 •

edited

Loading

Uh oh!

nirosys commented Jun 26, 2025

Uh oh!

nirosys commented Jun 26, 2025

Uh oh!

popematt Jun 30, 2025

Uh oh!

popematt Jun 30, 2025

Uh oh!

popematt Jun 30, 2025

Uh oh!

popematt Jun 30, 2025

Uh oh!

popematt Jun 30, 2025

Uh oh!

popematt Jun 30, 2025

Uh oh!

nirosys Jun 30, 2025

Uh oh!

nirosys Jun 30, 2025

Uh oh!

jobarr-amzn Jul 2, 2025

Uh oh!

jobarr-amzn Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

	let coeff_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments"))?;
	let coeff_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; found 0 arguments"))?;

	let expo_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; missing second argument"))?;
	let expo_expr = self.arguments.next().ok_or(IonError::decoding_error("`make_decimal` takes 2 integer arguments; found only 1 argument"))?;

Implement system macro make_decimal #986

Implement system macro make_decimal #986

Uh oh!

Conversation

nirosys commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nirosys commented Jun 26, 2025

Uh oh!

nirosys commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 26, 2025 •

edited

Loading