From 9d35d71438a182c92ffb6b12afcc87f118acc202 Mon Sep 17 00:00:00 2001 From: felix91gr <11747623+felix91gr@users.noreply.github.com> Date: Tue, 16 Sep 2025 20:05:19 +0000 Subject: [PATCH] Add guideline for issue #156 Co-authored-by: felix91gr Note: tables were written by hand due to limitations in our automation --- src/coding-guidelines/expressions.rst | 151 ++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/src/coding-guidelines/expressions.rst b/src/coding-guidelines/expressions.rst index 5f33a4a..04c256d 100644 --- a/src/coding-guidelines/expressions.rst +++ b/src/coding-guidelines/expressions.rst @@ -463,3 +463,154 @@ Expressions /* ... */ } + +.. guideline:: Integer shift shall only be performed through `checked_` APIs + :id: gui_RHvQj8BHlz9b + :category: required + :status: draft + :release: 1.7.0-latest + :fls: fls_sru4wi5jomoe + :decidability: decidable + :scope: module + :tags: numerics, reduce-human-error, maintainability, portability, surprising-behavior, subset + + In particular, the user should only perform left shifts via the `\ ``checked_shl`` `_ function and right shifts via the `\ ``checked_shr`` `_ function. Both of these functions exist in `\ ``core`` `_. + + This rule applies to the following primitive types: + + + * ``i8`` + * ``i16`` + * ``i32`` + * ``i64`` + * ``i128`` + * ``u8`` + * ``u16`` + * ``u32`` + * ``u64`` + * ``u128`` + * ``usize`` + * ``isize`` + + .. rationale:: + :id: rat_3MpR8QfHodGT + :status: draft + + This is a Subset rule, directly inspired by `INT34-C. Do not shift an expression by a negative number of bits or by greater than or equal to the number of bits that exist in the operand `_. + + In Rust these out-of-range shifts don't give rise to Undefined Behavior; however, they are still problematic in Safety Critical contexts for two reasons. + + + * + **Reason 1: inconsistent behavior** + + The behavior of shift operations depends on the compilation mode. Say for example, that we have a number ``x`` of type ``uN``\ , and we perform the operation + + ``x << M`` + + Then, it will behave like this: + + +------------------+-----------------+-----------------------+-----------------------+ + | Compilation Mode | ``0 <= M < N`` | ``M < 0`` | ``N <= M`` | + +==================+=================+=======================+=======================+ + | Debug | Shifts normally | Panics | Panics | + +------------------+-----------------+-----------------------+-----------------------+ + | Release | Shifts normally | Shifts by ``M mod N`` | Shifts by ``M mod N`` | + +------------------+-----------------+-----------------------+-----------------------+ + + .. + + Note: the behavior is exactly the same for the ``>>`` operator. + + + Panicking in ``Debug`` is an issue by itself, however, a perhaps larger issue there is that its behavior is different from that of ``Release``. Such inconsistencies aren't acceptable in Safety Critical scenarios. + + Therefore, a consistently-behaved operation should be required for performing shifts. + + * + **Reason 2: programmer intent** + + There is no scenario in which it makes sense to perform a shift of negative length, or of more than ``N - 1`` bits. The operation itself becomes meaningless. + + Therefore, an API that restricts the length of the shift to the range ``[0, N - 1]`` should be used instead of the ``<<`` and ``>>`` operators. + + * + **The Solution** + + The ideal solution for this exists in ``core``\ : ``checked_shl`` and ``checked_shr``. + + ``::checked_shl(M)`` returns a value of type ``Option``\ , in the following way: + + + * If ``M < 0``\ , the output is ``None`` + * If ``0 <= M < N`` for ``T`` of ``N`` bits, then the output is ``Some(T)`` + * If ``N <= M``\ , the output is ``None`` + + This API has consistent behavior across ``Debug`` and ``Release``\ , and makes the programmer intent explicit, which effectively solves this issue. + + .. non_compliant_example:: + :id: non_compl_ex_O9FZuazu3Lcn + :status: draft + + As seen in the example below: + + + * A ``Debug`` build **panics**\ , + * + Whereas a ``Release`` build prints the values: + + .. code-block:: + + 61 << -1 = 2147483648 + 61 << 4 = 976 + 61 << 40 = 15616 + + This shows **Reason 1** prominently. + + **Reason 2** is not seen in the code, because it is a reason of programmer intent: shifts by less than 0 or by more than ``N - 1`` (\ ``N`` being the bit-length of the value being shifted) are both meaningless. + + .. code-block:: rust + + fn bad_shl(bits: u32, shift: i32) -> u32 { + bits << shift + } + + let bits : u32 = 61; + let shifts = vec![-1, 4, 40]; + + for sh in shifts { + println!("{bits} << {sh} = {}", bad_shl(bits, sh)); + } + + .. compliant_example:: + :id: compl_ex_xpPQqYeEPGIo + :status: draft + + As seen in the example below: + + + * Both ``Debug`` and ``Release`` give the same exact output, which addresses **Reason 1**. + * Shifting by negative values is impossible due to the fact that ``checked_shl`` only accepts unsigned integers as shift lengths. + * Shifting by more than ``N - 1`` (\ ``N`` being the bit-length of the value being shifted) returns a ``None`` value: + .. code-block:: + + 61 << 4 = Some(976) + 61 << 40 = None + + The last 2 observations show how this addresses **Reason 2**. + + .. code-block:: rust + + fn good_shl(bits: u32, shift: u32) -> Option { + bits.checked_shl(shift) + } + + let bits : u32 = 61; + // let shifts = vec![-1, 4, 40]; + // ^--- Would not typecheck, as checked_shl + // only accepts positive shift amounts + let shifts = vec![4, 40]; + + for sh in shifts { + println!("{bits} << {sh} = {:?}", good_shl(bits, sh)); + }