From d5b2f9435545334cfa8ed3510988ee470347c702 Mon Sep 17 00:00:00 2001 From: rooooooooob Date: Thu, 13 Apr 2023 19:11:52 -0700 Subject: [PATCH 1/4] Direct ranges in fields e.g. `foo: 0..255` Previously they were only supported at the top-level. Incorrect parsing lead these to be treated as a constant of the lower bound. Fixes #172 --- src/parsing.rs | 14 ++++++++++---- tests/core/input.cddl | 10 +++++----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/parsing.rs b/src/parsing.rs index d56e53e..ca13884 100644 --- a/src/parsing.rs +++ b/src/parsing.rs @@ -1121,7 +1121,7 @@ fn rust_type_from_type1( )); ty.as_bytes() } - Some(ControlOperator::Range(min_max)) => match &type1.type2 { + Some(ControlOperator::Range((low, high))) => match &type1.type2 { Type2::Typename { ident, .. } if ident.to_string() == "uint" || ident.to_string() == "int" @@ -1130,15 +1130,21 @@ fn rust_type_from_type1( || ident.to_string() == "float32" || ident.to_string() == "float64" => { - match range_to_primitive(min_max.0, min_max.1) { + match range_to_primitive(low, high) { Some(t) => t.into(), None => panic!( - "unsupported range for {:?}: {:?}", + "unsupported range for {:?}: {:?}. Issue: https://github.com/dcSpark/cddl-codegen/issues/173", ident.to_string().as_str(), control ), } - } + }, + // the base value will be a constant due to incomplete parsing earlier for explicit ranges + // e.g. foo = 0..255 + Type2::IntValue { .. } | + Type2::UintValue { .. } => range_to_primitive(low, high) + .map(Into::into) + .expect("unsupported ranges. Only integer ranges mapping to primitives supported. Issue: https://github.com/dcSpark/cddl-codegen/issues/173"), _ => base_type, }, Some(ControlOperator::Default(default_value)) => base_type.default(default_value), diff --git a/tests/core/input.cddl b/tests/core/input.cddl index 72a326d..476b07f 100644 --- a/tests/core/input.cddl +++ b/tests/core/input.cddl @@ -59,21 +59,21 @@ i64 = int .size 8 ; 8 bytes signed_ints = [ u_8: uint .size 1, - u_16: uint .size 2, - u_32: uint .size 4, + u_16: uint .le 65535, + u_32: 0..4294967295, u_64: uint .size 8, - i_8: int .size 1, + i_8: -128..127, i_16: int .size 2, i_32: int .size 4, i_64: int .size 8, - n_64: nint + n_64: nint, u64_max: 18446744073709551615, ; this test assumes i64::BITS == isize::BITS (i.e. 64-bit programs) or else the cddl parsing lib would mess up ; if this test fails on your platform we might need to either remove this part ; or make a fix for the cddl library. ; The fix would be ideal as even though the true min in CBOR would be -u64::MAX ; we can't test that since isize::BITS is never > 64 in any normal system and likely never will be - i64_min: -9223372036854775808 + i64_min: -9223372036854775808, ] default_uint = uint .default 1337 From c2a2d32f1166f6f44e884c78ce13015b9ba9dd1b Mon Sep 17 00:00:00 2001 From: rooooooooob Date: Mon, 17 Apr 2023 16:36:39 -0700 Subject: [PATCH 2/4] integer range test coverage + fix for >isize::MAX ranges better coverage between top-level vs signed_ints member for various ranges that map to primitives fix for when >isize::MAX limits are used as we used `isize` when `i128` would be better for covering all possibilities (`i64`, `u64`, `f64`) float ranges now panic when they have a decimal part as we were silently ignoring this. Issue created: #178 --- src/parsing.rs | 25 +++++++++++++++---------- tests/core/input.cddl | 14 +++++++------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/parsing.rs b/src/parsing.rs index ca13884..10b2346 100644 --- a/src/parsing.rs +++ b/src/parsing.rs @@ -176,11 +176,16 @@ fn parse_type_choices( } } -fn type2_to_number_literal(type2: &Type2) -> isize { +fn type2_to_number_literal(type2: &Type2) -> i128 { match type2 { - Type2::UintValue { value, .. } => *value as isize, - Type2::IntValue { value, .. } => *value, - Type2::FloatValue { value, .. } => *value as isize, + Type2::UintValue { value, .. } => *value as i128, + Type2::IntValue { value, .. } => *value as i128, + Type2::FloatValue { value, .. } => { + // FloatToInt trait still experimental so just directly check + let as_int = *value as i128; + assert_eq!(as_int as f64, *value, "decimal not supported. Issue: https://github.com/dcSpark/cddl-codegen/issues/178"); + as_int + }, _ => panic!( "Value specified: {:?} must be a number literal to be used here", type2 @@ -216,15 +221,15 @@ fn parse_control_operator( match operator.operator { RangeCtlOp::RangeOp { is_inclusive, .. } => { let range_start = match type2 { - Type2::UintValue { value, .. } => *value as isize, - Type2::IntValue { value, .. } => *value, - Type2::FloatValue { value, .. } => *value as isize, + Type2::UintValue { value, .. } => *value as i128, + Type2::IntValue { value, .. } => *value as i128, + Type2::FloatValue { value, .. } => *value as i128, _ => panic!("Number expected as range start. Found {:?}", type2), }; let range_end = match operator.type2 { - Type2::UintValue { value, .. } => value as isize, - Type2::IntValue { value, .. } => value, - Type2::FloatValue { value, .. } => value as isize, + Type2::UintValue { value, .. } => value as i128, + Type2::IntValue { value, .. } => value as i128, + Type2::FloatValue { value, .. } => value as i128, _ => unimplemented!("unsupported type in range control operator: {:?}", operator), }; ControlOperator::Range(( diff --git a/tests/core/input.cddl b/tests/core/input.cddl index 476b07f..75a5f8d 100644 --- a/tests/core/input.cddl +++ b/tests/core/input.cddl @@ -54,17 +54,17 @@ u8 = uint .size 1 u16 = uint .le 65535 u32 = 0..4294967295 u64 = uint .size 8 ; 8 bytes -i8 = -128..127 +i8 = -128 .. 127 i64 = int .size 8 ; 8 bytes signed_ints = [ - u_8: uint .size 1, - u_16: uint .le 65535, - u_32: 0..4294967295, - u_64: uint .size 8, - i_8: -128..127, + u_8: 0 .. 255, + u_16: uint .size 2, + u_32: uint .size 4, + u_64: uint .le 18446744073709551615, + i_8: int .size 1, i_16: int .size 2, - i_32: int .size 4, + i_32: -2147483648 .. 2147483647, i_64: int .size 8, n_64: nint, u64_max: 18446744073709551615, From cee12b73d9261eedca3852469e64057029eab3aa Mon Sep 17 00:00:00 2001 From: rooooooooob Date: Mon, 17 Apr 2023 16:44:22 -0700 Subject: [PATCH 3/4] run cargo fmt --- src/parsing.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/parsing.rs b/src/parsing.rs index 10b2346..54fe959 100644 --- a/src/parsing.rs +++ b/src/parsing.rs @@ -183,9 +183,12 @@ fn type2_to_number_literal(type2: &Type2) -> i128 { Type2::FloatValue { value, .. } => { // FloatToInt trait still experimental so just directly check let as_int = *value as i128; - assert_eq!(as_int as f64, *value, "decimal not supported. Issue: https://github.com/dcSpark/cddl-codegen/issues/178"); + assert_eq!( + as_int as f64, *value, + "decimal not supported. Issue: https://github.com/dcSpark/cddl-codegen/issues/178" + ); as_int - }, + } _ => panic!( "Value specified: {:?} must be a number literal to be used here", type2 From e914c6f971b57b04732b0c78ef75da5d35f73634 Mon Sep 17 00:00:00 2001 From: rooooooooob Date: Mon, 17 Apr 2023 17:02:32 -0700 Subject: [PATCH 4/4] clippy fixes --- src/parsing.rs | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/parsing.rs b/src/parsing.rs index 54fe959..0ec8d64 100644 --- a/src/parsing.rs +++ b/src/parsing.rs @@ -236,11 +236,11 @@ fn parse_control_operator( _ => unimplemented!("unsupported type in range control operator: {:?}", operator), }; ControlOperator::Range(( - Some(range_start as i128), + Some(range_start), Some(if is_inclusive { - range_end as i128 + range_end } else { - (range_end + 1) as i128 + range_end + 1 }), )) } @@ -255,31 +255,29 @@ fn parse_control_operator( ControlOperator::CBOR(rust_type_from_type2(types, parent_visitor, &operator.type2)) } token::ControlOperator::EQ => ControlOperator::Range(( - Some(type2_to_number_literal(&operator.type2) as i128), - Some(type2_to_number_literal(&operator.type2) as i128), + Some(type2_to_number_literal(&operator.type2)), + Some(type2_to_number_literal(&operator.type2)), )), // TODO: this would be MUCH nicer (for error displaying, etc) to handle this in its own dedicated way // which might be necessary once we support other control operators anyway token::ControlOperator::NE => ControlOperator::Range(( - Some((type2_to_number_literal(&operator.type2) + 1) as i128), - Some((type2_to_number_literal(&operator.type2) - 1) as i128), + Some(type2_to_number_literal(&operator.type2) + 1), + Some(type2_to_number_literal(&operator.type2) - 1), )), token::ControlOperator::LE => ControlOperator::Range(( lower_bound, - Some(type2_to_number_literal(&operator.type2) as i128), + Some(type2_to_number_literal(&operator.type2)), )), token::ControlOperator::LT => ControlOperator::Range(( lower_bound, - Some((type2_to_number_literal(&operator.type2) - 1) as i128), - )), - token::ControlOperator::GE => ControlOperator::Range(( - Some(type2_to_number_literal(&operator.type2) as i128), - None, - )), - token::ControlOperator::GT => ControlOperator::Range(( - Some((type2_to_number_literal(&operator.type2) + 1) as i128), - None, + Some(type2_to_number_literal(&operator.type2) - 1), )), + token::ControlOperator::GE => { + ControlOperator::Range((Some(type2_to_number_literal(&operator.type2)), None)) + } + token::ControlOperator::GT => { + ControlOperator::Range((Some(type2_to_number_literal(&operator.type2) + 1), None)) + } token::ControlOperator::SIZE => { let base_range = match &operator.type2 { Type2::UintValue { value, .. } => {