Add guideline for issue #156

Co-authored-by: felix91gr <[email protected]>

Author: 2 people

src/coding-guidelines/expressions.rst

@@ -399,3 +399,162 @@ Expressions
           /* ... */
         }
 
+
+.. guideline:: Integer shift shall only be performed through `checked_` APIs
+    :id: gui_DRmTvyOtvdZh 
+    :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`` <https://doc.rust-lang.org/core/index.html?search=%22checked_shl%22>`_ function and right shifts via the `\ ``checked_shr`` <https://doc.rust-lang.org/core/index.html?search=%22checked_shr%22>`_ function. Both of these functions exist in `\ ``core`` <https://doc.rust-lang.org/core/index.html>`_.
+
+    This rule applies to the following primitive types:
+
+
+    * ``i8``
+    * ``i16``
+    * ``i32``
+    * ``i64``
+    * ``i128``
+    * ``u8``
+    * ``u16``
+    * ``u32``
+    * ``u64``
+    * ``u128``
+    * ``usize``
+    * ``isize``
+
+    .. rationale:: 
+        :id: rat_ElqKhoEBAPHl 
+        :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 <https://wiki.sei.cmu.edu/confluence/x/ItcxBQ>`_.
+
+        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:
+
+        .. list-table::
+           :header-rows: 1
+
+           * - **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``.
+
+        ``<T>::checked_shl(M)`` returns a value of type ``Option<T>``\ , 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_xTceuJc7RoTb 
+        :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_dn4iJW2Qf7kQ 
+        :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<u32> {
+                  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));
+              }