From 03b51a07dbd1fec5b8ca96228e1385cb5c0060ac Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 17 Mar 2022 10:44:32 -0700 Subject: [PATCH 01/10] Refactor converters to numeric types for `aws_smithy_types::Number` Currently, conversions from `aws_smithy_types::Number` into numeric Rust types (`{i,u}{8, 16, 32, 64}` and `f{32, 64}`) are always lossy, because they use the `as` Rust keyword to cast into the target type. This means that clients and servers are accepting lossy data: for example, if an operation is modeled to take in a 32-bit integer as input, and a client incorrectly sends an integer number that does not fit in 32 bits, the server will silently accept the truncated input. There are malformed request protocol tests that verify that servers must reject these requests. This commit removes the lossy `to_*` methods on `Number` and instead implements `TryFrom<$typ> for Number` for the target numeric type `$typ`. These converters will attempt their best to perform the conversion safely, and fail if it is lossy. The code-generated JSON parsers will now fail with `aws_smithy_json::deserialize::ErrorReason::InvalidNumber` if the number in the JSON document cannot be converted into the modeled integer type without losing precision. For floating point target types, lossy conversions are still performed, via `Number::to_f32_lossy` and `Number::to_f64_lossy`. --- .../protocol/ServerProtocolTestGenerator.kt | 26 - .../protocols/parse/JsonParserGenerator.kt | 19 +- .../aws-smithy-json/src/deserialize.rs | 4 +- .../aws-smithy-json/src/deserialize/error.rs | 9 + .../aws-smithy-json/src/deserialize/token.rs | 2 +- rust-runtime/aws-smithy-types/src/lib.rs | 460 +++++++++++++++++- 6 files changed, 462 insertions(+), 58 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt index 9437ff2f0a..b00db031be 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt @@ -665,40 +665,14 @@ class ServerProtocolTestGenerator( FailingTest(RestJson, "RestJsonWithPayloadExpectsModeledAccept", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonBodyMalformedBlobInvalidBase64_case1", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonBodyMalformedBlobInvalidBase64_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case6", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case8", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case10", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case0", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case1", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case3", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonWithBodyExpectsApplicationJsonContentType", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonWithPayloadExpectsImpliedContentType", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonWithPayloadExpectsModeledContentType", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonWithoutBodyExpectsEmptyContentType", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case6", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case8", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case10", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyIntegerUnderflowOverflow_case0", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyIntegerUnderflowOverflow_case1", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonBodyMalformedListNullItem", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonBodyMalformedMapNullValue", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case6", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case8", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case10", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonMalformedSetDuplicateItems", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonMalformedSetNullItem", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case6", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case8", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case10", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case0", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case1", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case2", TestType.MalformedRequest), - FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case3", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonHeaderMalformedStringInvalidBase64MediaType_case1", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonBodyTimestampDateTimeRejectsUTCOffsets_case0", TestType.MalformedRequest), FailingTest(RestJson, "RestJsonBodyTimestampDefaultRejectsMalformedEpochSeconds_case5", TestType.MalformedRequest), diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt index 739f0a93e6..0d729c6882 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt @@ -251,8 +251,23 @@ class JsonParserGenerator( } private fun RustWriter.deserializeNumber(target: NumberShape) { - val symbol = symbolProvider.toSymbol(target) - rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_#{T}())", "T" to symbol, *codegenScope) + if (target.isFloatShape) { + rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_f32_lossy())", *codegenScope) + } else if (target.isDoubleShape) { + rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_f64_lossy())", *codegenScope) + } else { + rustTemplate( + """ + #{expect_number_or_null}(tokens.next())? + .map(|v| { + use std::convert::TryInto; + v.try_into() + }) + .transpose()? + """, + *codegenScope + ) + } } private fun RustWriter.deserializeTimestamp(member: MemberShape) { diff --git a/rust-runtime/aws-smithy-json/src/deserialize.rs b/rust-runtime/aws-smithy-json/src/deserialize.rs index 247d8962bf..f2419e0f42 100644 --- a/rust-runtime/aws-smithy-json/src/deserialize.rs +++ b/rust-runtime/aws-smithy-json/src/deserialize.rs @@ -282,7 +282,7 @@ impl<'a> JsonTokenIterator<'a> { /// returns `(start_index, end_index, negative, floating)`, with `start_index` /// and `end_index` representing the slice of the stream that is the number, /// `negative` whether or not it is a negative number, and `floating` whether or not - /// it is a floating point number. + /// the number contains a decimal point and/or an exponent. fn scan_number(&mut self) -> (usize, usize, bool, bool) { let start_index = self.index; let negative = if self.peek_byte() == Some(b'-') { @@ -338,7 +338,7 @@ impl<'a> JsonTokenIterator<'a> { if negative > 0 { Number::Float(-(positive as f64)) } else { - Number::NegInt(negative as i64) + Number::NegInt(negative) } } else { Number::PosInt( diff --git a/rust-runtime/aws-smithy-json/src/deserialize/error.rs b/rust-runtime/aws-smithy-json/src/deserialize/error.rs index 2bf1c50264..2be7780a45 100644 --- a/rust-runtime/aws-smithy-json/src/deserialize/error.rs +++ b/rust-runtime/aws-smithy-json/src/deserialize/error.rs @@ -82,3 +82,12 @@ impl From for Error { } } } + +impl From for Error { + fn from(_: aws_smithy_types::TryFromNumberError) -> Self { + Error { + reason: ErrorReason::InvalidNumber, + offset: None, + } + } +} diff --git a/rust-runtime/aws-smithy-json/src/deserialize/token.rs b/rust-runtime/aws-smithy-json/src/deserialize/token.rs index 7ab718ddc4..84fb8d3752 100644 --- a/rust-runtime/aws-smithy-json/src/deserialize/token.rs +++ b/rust-runtime/aws-smithy-json/src/deserialize/token.rs @@ -217,7 +217,7 @@ pub fn expect_timestamp_or_null( ) -> Result, Error> { Ok(match timestamp_format { Format::EpochSeconds => { - expect_number_or_null(token)?.map(|v| DateTime::from_secs_f64(v.to_f64())) + expect_number_or_null(token)?.map(|v| DateTime::from_secs_f64(v.to_f64_lossy())) } Format::DateTime | Format::HttpDate => expect_string_or_null(token)? .map(|v| DateTime::from_str(v.as_escaped_str(), timestamp_format)) diff --git a/rust-runtime/aws-smithy-types/src/lib.rs b/rust-runtime/aws-smithy-types/src/lib.rs index 8ce19f0f2d..0cea5f23cc 100644 --- a/rust-runtime/aws-smithy-types/src/lib.rs +++ b/rust-runtime/aws-smithy-types/src/lib.rs @@ -13,7 +13,7 @@ unreachable_pub )] -use std::collections::HashMap; +use std::{collections::HashMap, convert::TryFrom}; pub mod base64; pub mod date_time; @@ -80,46 +80,452 @@ pub enum Document { /// #[derive(Debug, Clone, Copy, PartialEq)] pub enum Number { - /// Unsigned 64-bit integer value + /// Unsigned 64-bit integer value. PosInt(u64), - /// Signed 64-bit integer value + /// Signed 64-bit integer value. The wrapped value is _always_ negative. NegInt(i64), - /// 64-bit floating-point value + /// 64-bit floating-point value. Float(f64), } -macro_rules! to_num_fn { - ($name:ident, $typ:ident, $styp:expr) => { +impl Number { + /// Converts to an `f64` lossily. + /// Use `Number::try_from` to make the conversion only if it is not lossy. + pub fn to_f64_lossy(self: Self) -> f64 { + match self { + Number::PosInt(v) => v as f64, + Number::NegInt(v) => v as f64, + Number::Float(v) => v as f64, + } + } + + /// Converts to an `f32` lossily. + /// Use `Number::try_from` to make the conversion only if it is not lossy. + pub fn to_f32_lossy(self: Self) -> f32 { + match self { + Number::PosInt(v) => v as f32, + Number::NegInt(v) => v as f32, + Number::Float(v) => v as f32, + } + } +} + +/// The error type returned when conversion into an integer type or floating point type is lossy. +#[derive(Debug)] +pub enum TryFromNumberError { + /// Used when the conversion from an integer type into a smaller integer type would be lossy. + OutsideIntegerRange(std::num::TryFromIntError), + /// Used when the conversion from an `u64` into a floating point type would be lossy. + U64ToFloatLossyConversion(u64), + /// Used when the conversion from an `i64` into a floating point type would be lossy. + I64ToFloatLossyConversion(i64), + /// Used when attempting to convert an `f64` into an `f32`. + F64ToF32LossyConversion(f64), + /// Used when attempting to convert a decimal, infinite, or `NaN` floating point type into an + /// integer type. + UnexpectedFloat(f64), + /// Used when attempting to convert a negative [`Number`] into an unsigned integer type. + UnexpectedNegativeInteger(i64), +} + +impl std::fmt::Display for TryFromNumberError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + TryFromNumberError::OutsideIntegerRange(err) => write!(f, "integer too large: {}", err), + TryFromNumberError::UnexpectedFloat(v) => write!( + f, + "cannot convert {} floating point number into an integer", + v + ), + TryFromNumberError::UnexpectedNegativeInteger(v) => write!( + f, + "cannot convert {} negative integer into an unsigned integer type", + v + ), + TryFromNumberError::U64ToFloatLossyConversion(v) => { + write!( + f, + "cannot convert {}u64 into a floating point type without precision loss", + v + ) + } + TryFromNumberError::I64ToFloatLossyConversion(v) => { + write!( + f, + "cannot convert {}i64 into a floating point type without precision loss", + v + ) + } + TryFromNumberError::F64ToF32LossyConversion(v) => { + write!(f, "will not attempt to convert {}f64 into a f32", v) + } + } + } +} + +impl std::error::Error for TryFromNumberError {} + +impl From for TryFromNumberError { + fn from(value: std::num::TryFromIntError) -> Self { + Self::OutsideIntegerRange(value) + } +} + +macro_rules! to_unsigned_integer_converter { + ($typ:ident, $styp:expr) => { #[doc = "Converts to a `"] #[doc = $styp] - #[doc = "`. This conversion may be lossy."] - pub fn $name(self) -> $typ { - match self { - Number::PosInt(val) => val as $typ, - Number::NegInt(val) => val as $typ, - Number::Float(val) => val as $typ, + #[doc = "`. This conversion can only fail if it is lossy."] + impl TryFrom for $typ { + type Error = TryFromNumberError; + + fn try_from(value: Number) -> Result { + match value { + Number::PosInt(v) => Ok(Self::try_from(v)?), + Number::NegInt(v) => Err(Self::Error::UnexpectedNegativeInteger(v)), + Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + } } } }; - ($name:ident, $typ:ident) => { - to_num_fn!($name, $typ, stringify!($typ)); + ($typ:ident) => { + to_unsigned_integer_converter!($typ, stringify!($typ)); }; } -impl Number { - to_num_fn!(to_f32, f32); - to_num_fn!(to_f64, f64); - - to_num_fn!(to_i8, i8); - to_num_fn!(to_i16, i16); - to_num_fn!(to_i32, i32); - to_num_fn!(to_i64, i64); - - to_num_fn!(to_u8, u8); - to_num_fn!(to_u16, u16); - to_num_fn!(to_u32, u32); - to_num_fn!(to_u64, u64); +macro_rules! to_signed_integer_converter { + ($typ:ident, $styp:expr) => { + #[doc = "Converts to a `"] + #[doc = $styp] + #[doc = "`. This conversion can only fail if it is lossy."] + impl TryFrom for $typ { + type Error = TryFromNumberError; + + fn try_from(value: Number) -> Result { + match value { + Number::PosInt(v) => Ok(Self::try_from(v)?), + Number::NegInt(v) => Ok(Self::try_from(v)?), + Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + } + } + } + }; + + ($typ:ident) => { + to_signed_integer_converter!($typ, stringify!($typ)); + }; +} + +/// Converts to a `u64`. The conversion can only fail if it is lossy. +impl TryFrom for u64 { + type Error = TryFromNumberError; + + fn try_from(value: Number) -> Result { + match value { + Number::PosInt(v) => Ok(v), + Number::NegInt(v) => Err(Self::Error::UnexpectedNegativeInteger(v)), + Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + } + } +} +to_unsigned_integer_converter!(u32); +to_unsigned_integer_converter!(u16); +to_unsigned_integer_converter!(u8); + +impl TryFrom for i64 { + type Error = TryFromNumberError; + + fn try_from(value: Number) -> Result { + match value { + Number::PosInt(v) => Ok(Self::try_from(v)?), + Number::NegInt(v) => Ok(v), + Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + } + } +} +to_signed_integer_converter!(i32); +to_signed_integer_converter!(i16); +to_signed_integer_converter!(i8); + +/// Converts to an `f64`. The conversion can only fail if it is lossy. +impl TryFrom for f64 { + type Error = TryFromNumberError; + + fn try_from(value: Number) -> Result { + match value { + // Integers can only be represented with full precision in a float if they fit in the + // significand, which is 24 bits in `f32` and 53 bits in `f64`. + // https://github.com/rust-lang/rust/blob/58f11791af4f97572e7afd83f11cffe04bbbd12f/library/core/src/convert/num.rs#L151-L153 + Number::PosInt(v) => { + if v <= (1 << 53) { + Ok(v as Self) + } else { + Err(Self::Error::U64ToFloatLossyConversion(v)) + } + } + Number::NegInt(v) => { + if -(1 << 53) <= v && v <= (1 << 53) { + Ok(v as Self) + } else { + Err(Self::Error::I64ToFloatLossyConversion(v)) + } + } + Number::Float(v) => Ok(v), + } + } +} + +/// Converts to an `f64`. The conversion can only fail if it is lossy. +impl TryFrom for f32 { + type Error = TryFromNumberError; + + fn try_from(value: Number) -> Result { + match value { + Number::PosInt(v) => { + if v <= (1 << 24) { + Ok(v as Self) + } else { + Err(Self::Error::U64ToFloatLossyConversion(v)) + } + } + Number::NegInt(v) => { + if -(1 << 24) <= v && v <= (1 << 24) { + Ok(v as Self) + } else { + Err(Self::Error::I64ToFloatLossyConversion(v)) + } + } + Number::Float(v) => Err(Self::Error::F64ToF32LossyConversion(v)), + } + } +} + +#[cfg(test)] +mod number { + use super::*; + + macro_rules! to_unsigned_converter_tests { + ($typ:ident) => { + assert_eq!($typ::try_from(Number::PosInt(69u64)).unwrap(), 69); + + assert!(matches!( + $typ::try_from(Number::PosInt(($typ::MAX as u64) + 1u64)).unwrap_err(), + TryFromNumberError::OutsideIntegerRange(..) + )); + + assert!(matches!( + $typ::try_from(Number::NegInt(-1i64)).unwrap_err(), + TryFromNumberError::UnexpectedNegativeInteger(..) + )); + + for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert!(matches!( + $typ::try_from(Number::Float(val)).unwrap_err(), + TryFromNumberError::UnexpectedFloat(..) + )); + } + }; + } + + #[test] + fn to_u64() { + assert_eq!(u64::try_from(Number::PosInt(69u64)).unwrap(), 69u64); + + assert!(matches!( + u64::try_from(Number::NegInt(-1i64)).unwrap_err(), + TryFromNumberError::UnexpectedNegativeInteger(..) + )); + + for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert!(matches!( + u64::try_from(Number::Float(val)).unwrap_err(), + TryFromNumberError::UnexpectedFloat(..) + )); + } + } + + #[test] + fn to_u32() { + to_unsigned_converter_tests!(u32); + } + + #[test] + fn to_u16() { + to_unsigned_converter_tests!(u16); + } + + #[test] + fn to_u8() { + to_unsigned_converter_tests!(u8); + } + + macro_rules! to_signed_converter_tests { + ($typ:ident) => { + assert_eq!($typ::try_from(Number::PosInt(69u64)).unwrap(), 69); + assert_eq!($typ::try_from(Number::NegInt(-69i64)).unwrap(), -69); + + assert!(matches!( + $typ::try_from(Number::PosInt(($typ::MAX as u64) + 1u64)).unwrap_err(), + TryFromNumberError::OutsideIntegerRange(..) + )); + + assert!(matches!( + $typ::try_from(Number::NegInt(($typ::MIN as i64) - 1i64)).unwrap_err(), + TryFromNumberError::OutsideIntegerRange(..) + )); + + for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert!(matches!( + u64::try_from(Number::Float(val)).unwrap_err(), + TryFromNumberError::UnexpectedFloat(..) + )); + } + }; + } + + #[test] + fn to_i64() { + assert_eq!(i64::try_from(Number::PosInt(69u64)).unwrap(), 69); + assert_eq!(i64::try_from(Number::NegInt(-69i64)).unwrap(), -69); + + for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert!(matches!( + u64::try_from(Number::Float(val)).unwrap_err(), + TryFromNumberError::UnexpectedFloat(..) + )); + } + } + + #[test] + fn to_i32() { + to_signed_converter_tests!(i32); + } + + #[test] + fn to_i16() { + to_signed_converter_tests!(i16); + } + + #[test] + fn to_i8() { + to_signed_converter_tests!(i8); + } + + #[test] + fn to_f64() { + assert_eq!(f64::try_from(Number::PosInt(69u64)).unwrap(), 69f64); + assert_eq!(f64::try_from(Number::NegInt(-69i64)).unwrap(), -69f64); + assert_eq!(f64::try_from(Number::Float(-69f64)).unwrap(), -69f64); + assert!(f64::try_from(Number::Float(f64::NAN)).unwrap().is_nan()); + assert_eq!( + f64::try_from(Number::Float(f64::INFINITY)).unwrap(), + f64::INFINITY + ); + assert_eq!( + f64::try_from(Number::Float(f64::NEG_INFINITY)).unwrap(), + f64::NEG_INFINITY + ); + + let significand_max_u64: u64 = 1 << 53; + let significand_max_i64: i64 = 1 << 53; + + assert_eq!( + f64::try_from(Number::PosInt(significand_max_u64)).unwrap(), + 9007199254740992f64 + ); + + assert_eq!( + f64::try_from(Number::NegInt(significand_max_i64)).unwrap(), + 9007199254740992f64 + ); + assert_eq!( + f64::try_from(Number::NegInt(-significand_max_i64)).unwrap(), + -9007199254740992f64 + ); + + assert!(matches!( + f64::try_from(Number::PosInt(significand_max_u64 + 1)).unwrap_err(), + TryFromNumberError::U64ToFloatLossyConversion(..) + )); + + assert!(matches!( + f64::try_from(Number::NegInt(significand_max_i64 + 1)).unwrap_err(), + TryFromNumberError::I64ToFloatLossyConversion(..) + )); + assert!(matches!( + f64::try_from(Number::NegInt(-significand_max_i64 - 1)).unwrap_err(), + TryFromNumberError::I64ToFloatLossyConversion(..) + )); + } + + #[test] + fn to_f32() { + assert_eq!(f32::try_from(Number::PosInt(69u64)).unwrap(), 69f32); + assert_eq!(f32::try_from(Number::NegInt(-69i64)).unwrap(), -69f32); + + let significand_max_u64: u64 = 1 << 24; + let significand_max_i64: i64 = 1 << 24; + + assert_eq!( + f32::try_from(Number::PosInt(significand_max_u64)).unwrap(), + 16777216f32 + ); + + assert_eq!( + f32::try_from(Number::NegInt(significand_max_i64)).unwrap(), + 16777216f32 + ); + assert_eq!( + f32::try_from(Number::NegInt(-significand_max_i64)).unwrap(), + -16777216f32 + ); + + assert!(matches!( + f32::try_from(Number::PosInt(significand_max_u64 + 1)).unwrap_err(), + TryFromNumberError::U64ToFloatLossyConversion(..) + )); + + assert!(matches!( + f32::try_from(Number::NegInt(significand_max_i64 + 1)).unwrap_err(), + TryFromNumberError::I64ToFloatLossyConversion(..) + )); + assert!(matches!( + f32::try_from(Number::NegInt(-significand_max_i64 - 1)).unwrap_err(), + TryFromNumberError::I64ToFloatLossyConversion(..) + )); + + for val in [69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert!(matches!( + f32::try_from(Number::Float(val)).unwrap_err(), + TryFromNumberError::F64ToF32LossyConversion(..) + )); + } + } + + #[test] + fn to_f64_lossy() { + assert_eq!(Number::PosInt(69u64).to_f64_lossy(), 69f64); + assert_eq!( + Number::PosInt((1 << 53) + 1).to_f64_lossy(), + 9007199254740992f64 + ); + assert_eq!( + Number::NegInt(-(1 << 53) - 1).to_f64_lossy(), + -9007199254740992f64 + ); + } + + #[test] + fn to_f32_lossy() { + assert_eq!(Number::PosInt(69u64).to_f32_lossy(), 69f32); + assert_eq!(Number::PosInt((1 << 24) + 1).to_f32_lossy(), 16777216f32); + assert_eq!(Number::NegInt(-(1 << 24) - 1).to_f32_lossy(), -16777216f32); + assert_eq!( + Number::Float(1452089033.7674935).to_f32_lossy(), + 1452089100f32 + ); + } } /* ANCHOR_END: document */ From 7cace8a5dd9c78d43a23fd2fef7969c13ef78f51 Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 23 Mar 2022 16:49:28 +0100 Subject: [PATCH 02/10] Update changelog --- CHANGELOG.next.toml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 73077ebc92..a4022bd12b 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -34,3 +34,9 @@ message = "Update all SDK and runtime crates to [edition 2021](https://blog.rust references = ["aws-sdk-rust#490"] meta = { "breaking" = true, "tada" = false, "bug" = false } author = "Velfi" + +[[smithy-rs]] +message = "Refactor converters to numeric types for `aws_smithy_types::Number`" +references = ["smithy-rs#1274"] +meta = { "breaking" = true, "tada" = false, "bug" = true } +author = "david-perez" From ab6f306aef2a2fa0ec1b0620dce91b2f4d464154 Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 23 Mar 2022 20:23:14 +0100 Subject: [PATCH 03/10] Kick off CI From 6130fcb9bd14e385dcd9be7cc6832771804c487a Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 23 Mar 2022 22:37:24 +0100 Subject: [PATCH 04/10] fix needless_arbitrary_self_type --- rust-runtime/aws-smithy-types/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust-runtime/aws-smithy-types/src/lib.rs b/rust-runtime/aws-smithy-types/src/lib.rs index 0cea5f23cc..d9b3df3fff 100644 --- a/rust-runtime/aws-smithy-types/src/lib.rs +++ b/rust-runtime/aws-smithy-types/src/lib.rs @@ -91,7 +91,7 @@ pub enum Number { impl Number { /// Converts to an `f64` lossily. /// Use `Number::try_from` to make the conversion only if it is not lossy. - pub fn to_f64_lossy(self: Self) -> f64 { + pub fn to_f64_lossy(self) -> f64 { match self { Number::PosInt(v) => v as f64, Number::NegInt(v) => v as f64, @@ -101,7 +101,7 @@ impl Number { /// Converts to an `f32` lossily. /// Use `Number::try_from` to make the conversion only if it is not lossy. - pub fn to_f32_lossy(self: Self) -> f32 { + pub fn to_f32_lossy(self) -> f32 { match self { Number::PosInt(v) => v as f32, Number::NegInt(v) => v as f32, From fc23fad8d3fb6a56871649130c614e4355d898c9 Mon Sep 17 00:00:00 2001 From: david-perez Date: Mon, 11 Apr 2022 18:50:50 +0200 Subject: [PATCH 05/10] Suggestions from PR --- rust-runtime/aws-smithy-types/src/lib.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/rust-runtime/aws-smithy-types/src/lib.rs b/rust-runtime/aws-smithy-types/src/lib.rs index d9b3df3fff..53e1f22d3b 100644 --- a/rust-runtime/aws-smithy-types/src/lib.rs +++ b/rust-runtime/aws-smithy-types/src/lib.rs @@ -88,6 +88,8 @@ pub enum Number { Float(f64), } +/* ANCHOR_END: document */ + impl Number { /// Converts to an `f64` lossily. /// Use `Number::try_from` to make the conversion only if it is not lossy. @@ -134,12 +136,12 @@ impl std::fmt::Display for TryFromNumberError { TryFromNumberError::OutsideIntegerRange(err) => write!(f, "integer too large: {}", err), TryFromNumberError::UnexpectedFloat(v) => write!( f, - "cannot convert {} floating point number into an integer", + "cannot convert floating point number {} into an integer", v ), TryFromNumberError::UnexpectedNegativeInteger(v) => write!( f, - "cannot convert {} negative integer into an unsigned integer type", + "cannot convert negative integer {} into an unsigned integer type", v ), TryFromNumberError::U64ToFloatLossyConversion(v) => { @@ -175,7 +177,7 @@ macro_rules! to_unsigned_integer_converter { ($typ:ident, $styp:expr) => { #[doc = "Converts to a `"] #[doc = $styp] - #[doc = "`. This conversion can only fail if it is lossy."] + #[doc = "`. This conversion fails if it is lossy."] impl TryFrom for $typ { type Error = TryFromNumberError; @@ -198,7 +200,7 @@ macro_rules! to_signed_integer_converter { ($typ:ident, $styp:expr) => { #[doc = "Converts to a `"] #[doc = $styp] - #[doc = "`. This conversion can only fail if it is lossy."] + #[doc = "`. This conversion fails if it is lossy."] impl TryFrom for $typ { type Error = TryFromNumberError; @@ -217,7 +219,7 @@ macro_rules! to_signed_integer_converter { }; } -/// Converts to a `u64`. The conversion can only fail if it is lossy. +/// Converts to a `u64`. The conversion fails if it is lossy. impl TryFrom for u64 { type Error = TryFromNumberError; @@ -248,7 +250,7 @@ to_signed_integer_converter!(i32); to_signed_integer_converter!(i16); to_signed_integer_converter!(i8); -/// Converts to an `f64`. The conversion can only fail if it is lossy. +/// Converts to an `f64`. The conversion fails if it is lossy. impl TryFrom for f64 { type Error = TryFromNumberError; @@ -276,7 +278,7 @@ impl TryFrom for f64 { } } -/// Converts to an `f64`. The conversion can only fail if it is lossy. +/// Converts to an `f64`. The conversion fails if it is lossy. impl TryFrom for f32 { type Error = TryFromNumberError; @@ -528,8 +530,6 @@ mod number { } } -/* ANCHOR_END: document */ - pub use error::Error; /// Generic errors for Smithy codegen From c21572dab734d43892e35cb5a17d46cab7fd9ca4 Mon Sep 17 00:00:00 2001 From: david-perez Date: Mon, 11 Apr 2022 19:07:51 +0200 Subject: [PATCH 06/10] Expand on breaking change in changelog --- CHANGELOG.next.toml | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 03e3be3157..ebf49c1c65 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -60,7 +60,39 @@ meta = { "breaking" = false, "tada" = false, "bug" = false } author = "benesch" [[smithy-rs]] -message = "Refactor converters to numeric types for `aws_smithy_types::Number`" +message = """ +Lossy converters into integer types for `aws_smithy_types::Number` have been +removed. Lossy converters into floating point types for +`aws_smithy_types::Number` have been suffixed with `_lossy`. If you were +directly using the integer lossy converters, we recommend you use the safe +converters. + +_Before:_ + +```rust +fn f1(n: aws_smithy_types::Number) { + let foo: f32 = n.to_f32(); // Lossy conversion! + + let bar: u32 = n.to_u32(); // Lossy conversion! +} +``` + +_After:_ + +```rust +fn f1(n: aws_smithy_types::Number) { + use std::convert::TryInto; // Unnecessary import if you're using Rust 2021 edition. + + let foo: f32 = n.try_into().expect("lossy conversion detected"); // Or handle the error instead of panicking. + + // You can still do lossy conversions, but only into floating point types. + let foo: f32 = n.to_f32_lossy(); + + // To lossily convert into integer types, use an `as` cast directly. + let bar: u32 = n as u32; // Lossy conversion! +} +``` +""" references = ["smithy-rs#1274"] meta = { "breaking" = true, "tada" = false, "bug" = true } author = "david-perez" From e17da696d2b01e44dd7031df00150bad6e87335b Mon Sep 17 00:00:00 2001 From: david-perez Date: Fri, 12 Aug 2022 14:49:55 +0200 Subject: [PATCH 07/10] Rename some TryFromNumberError variants --- .../protocols/parse/JsonParserGenerator.kt | 5 +-- rust-runtime/aws-smithy-types/src/lib.rs | 32 +++++++++---------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt index 3b772fd252..7495dd1069 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt @@ -270,10 +270,7 @@ class JsonParserGenerator( rustTemplate( """ #{expect_number_or_null}(tokens.next())? - .map(|v| { - use std::convert::TryInto; - v.try_into() - }) + .map(|v| v.try_into()) .transpose()? """, *codegenScope diff --git a/rust-runtime/aws-smithy-types/src/lib.rs b/rust-runtime/aws-smithy-types/src/lib.rs index 1bb90d0f95..6420ebaa82 100644 --- a/rust-runtime/aws-smithy-types/src/lib.rs +++ b/rust-runtime/aws-smithy-types/src/lib.rs @@ -125,21 +125,21 @@ pub enum TryFromNumberError { F64ToF32LossyConversion(f64), /// Used when attempting to convert a decimal, infinite, or `NaN` floating point type into an /// integer type. - UnexpectedFloat(f64), + FloatToIntegerLossyConversion(f64), /// Used when attempting to convert a negative [`Number`] into an unsigned integer type. - UnexpectedNegativeInteger(i64), + NegativeToUnsignedLossyConversion(i64), } impl std::fmt::Display for TryFromNumberError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { TryFromNumberError::OutsideIntegerRange(err) => write!(f, "integer too large: {}", err), - TryFromNumberError::UnexpectedFloat(v) => write!( + TryFromNumberError::FloatToIntegerLossyConversion(v) => write!( f, "cannot convert floating point number {} into an integer", v ), - TryFromNumberError::UnexpectedNegativeInteger(v) => write!( + TryFromNumberError::NegativeToUnsignedLossyConversion(v) => write!( f, "cannot convert negative integer {} into an unsigned integer type", v @@ -184,8 +184,8 @@ macro_rules! to_unsigned_integer_converter { fn try_from(value: Number) -> Result { match value { Number::PosInt(v) => Ok(Self::try_from(v)?), - Number::NegInt(v) => Err(Self::Error::UnexpectedNegativeInteger(v)), - Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + Number::NegInt(v) => Err(Self::Error::NegativeToUnsignedLossyConversion(v)), + Number::Float(v) => Err(Self::Error::FloatToIntegerLossyConversion(v)), } } } @@ -208,7 +208,7 @@ macro_rules! to_signed_integer_converter { match value { Number::PosInt(v) => Ok(Self::try_from(v)?), Number::NegInt(v) => Ok(Self::try_from(v)?), - Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + Number::Float(v) => Err(Self::Error::FloatToIntegerLossyConversion(v)), } } } @@ -226,8 +226,8 @@ impl TryFrom for u64 { fn try_from(value: Number) -> Result { match value { Number::PosInt(v) => Ok(v), - Number::NegInt(v) => Err(Self::Error::UnexpectedNegativeInteger(v)), - Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + Number::NegInt(v) => Err(Self::Error::NegativeToUnsignedLossyConversion(v)), + Number::Float(v) => Err(Self::Error::FloatToIntegerLossyConversion(v)), } } } @@ -242,7 +242,7 @@ impl TryFrom for i64 { match value { Number::PosInt(v) => Ok(Self::try_from(v)?), Number::NegInt(v) => Ok(v), - Number::Float(v) => Err(Self::Error::UnexpectedFloat(v)), + Number::Float(v) => Err(Self::Error::FloatToIntegerLossyConversion(v)), } } } @@ -318,13 +318,13 @@ mod number { assert!(matches!( $typ::try_from(Number::NegInt(-1i64)).unwrap_err(), - TryFromNumberError::UnexpectedNegativeInteger(..) + TryFromNumberError::NegativeToUnsignedLossyConversion(..) )); for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { assert!(matches!( $typ::try_from(Number::Float(val)).unwrap_err(), - TryFromNumberError::UnexpectedFloat(..) + TryFromNumberError::FloatToIntegerLossyConversion(..) )); } }; @@ -336,13 +336,13 @@ mod number { assert!(matches!( u64::try_from(Number::NegInt(-1i64)).unwrap_err(), - TryFromNumberError::UnexpectedNegativeInteger(..) + TryFromNumberError::NegativeToUnsignedLossyConversion(..) )); for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { assert!(matches!( u64::try_from(Number::Float(val)).unwrap_err(), - TryFromNumberError::UnexpectedFloat(..) + TryFromNumberError::FloatToIntegerLossyConversion(..) )); } } @@ -380,7 +380,7 @@ mod number { for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { assert!(matches!( u64::try_from(Number::Float(val)).unwrap_err(), - TryFromNumberError::UnexpectedFloat(..) + TryFromNumberError::FloatToIntegerLossyConversion(..) )); } }; @@ -394,7 +394,7 @@ mod number { for val in [69.69f64, f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { assert!(matches!( u64::try_from(Number::Float(val)).unwrap_err(), - TryFromNumberError::UnexpectedFloat(..) + TryFromNumberError::FloatToIntegerLossyConversion(..) )); } } From afffb9c275bd8a56792c3d7f3e681cbf156e9016 Mon Sep 17 00:00:00 2001 From: david-perez Date: Fri, 12 Aug 2022 15:19:04 +0200 Subject: [PATCH 08/10] Unnecessary TryFrom import in 2021 edition --- rust-runtime/aws-smithy-types/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-runtime/aws-smithy-types/src/lib.rs b/rust-runtime/aws-smithy-types/src/lib.rs index 6420ebaa82..570e4e78fd 100644 --- a/rust-runtime/aws-smithy-types/src/lib.rs +++ b/rust-runtime/aws-smithy-types/src/lib.rs @@ -13,7 +13,7 @@ unreachable_pub )] -use std::{collections::HashMap, convert::TryFrom}; +use std::collections::HashMap; pub mod base64; pub mod date_time; From 329a929aa5c2b548d8ef38e4e3194d5d0669b4dc Mon Sep 17 00:00:00 2001 From: david-perez Date: Tue, 16 Aug 2022 14:35:27 +0200 Subject: [PATCH 09/10] appease ktlint --- .../rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt index 73ca95f58f..aa7f082e04 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt @@ -289,7 +289,7 @@ class JsonParserGenerator( .map(|v| v.try_into()) .transpose()? """, - *codegenScope + *codegenScope, ) } } From c0400f67ac05eb1ce5f81995dc4d71e6d23cc5d9 Mon Sep 17 00:00:00 2001 From: david-perez Date: Tue, 16 Aug 2022 17:23:14 +0200 Subject: [PATCH 10/10] refactor aws-config CredentialProcess JSON parsing --- aws/rust-runtime/aws-config/src/credential_process.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/aws/rust-runtime/aws-config/src/credential_process.rs b/aws/rust-runtime/aws-config/src/credential_process.rs index fc3409b401..ab4727b309 100644 --- a/aws/rust-runtime/aws-config/src/credential_process.rs +++ b/aws/rust-runtime/aws-config/src/credential_process.rs @@ -194,7 +194,12 @@ pub(crate) fn parse_credential_process_json_credentials( "Expiration": "2022-05-02T18:36:00+00:00" */ (key, Token::ValueNumber { value, .. }) if key.eq_ignore_ascii_case("Version") => { - version = Some(value.to_i32()) + version = Some(i32::try_from(*value).map_err(|err| { + InvalidJsonCredentials::InvalidField { + field: "Version", + err: err.into(), + } + })?); } (key, Token::ValueString { value, .. }) if key.eq_ignore_ascii_case("AccessKeyId") => { access_key_id = Some(value.to_unescaped()?)