Skip to content

"Avoid out-or-range shifts" is wrong in multiple ways #309

@felix91gr

Description

@felix91gr

The guideline in question:

https://coding-guidelines.arewesafetycriticalyet.org/coding-guidelines/expressions/index.html#gui_LvmzGKdsAgI5

How is it wrong

Semantic Errors

In the second and third paragraphs of the Amplification:

    If the types of both operands are integer types,
    the shift left expression ``lhs << rhs`` evaluates to the value of the left operand ``lhs`` whose bits are 
    shifted left by the number of positions specified by the right operand ``rhs``.
    Vacated bits are filled with zeros. 
    The expression ``lhs << rhs`` evaluates to :math:`\mathrm{lhs} \times 2^{\mathrm{rhs}}`, 
    cast to the type of the left operand.
    If the value of the right operand is negative or greater than or equal to the width of the left operand,
    then the operation results in an out-of-range shift.

    If the types of both operands are integer types,
    the shift right expression ``lhs >> rhs`` evaluates to the value of the left operand ``lhs`` 
    whose bits are shifted right by the number of positions specified by the right operand ``rhs``.
    If the type of the left operand is any signed integer type and is negative,
    the vacated bits are filled with ones.
    Otherwise, vacated bits are filled with zeros.
    The expression ``lhs >> rhs`` evaluates to :math:`\mathrm{lhs} / 2^{\mathrm{rhs}}`,
    cast to the type of the left operand.
    If the value of the right operand is negative,
    greater than or equal to the width of the left operand,
    then the operation results in an out-of-range shift.

That explanation is wrong:

  1. The behavior of lhs << rhs and lhs >> rhs when rhs : uN is to do rhs = rhs & (2.pow(N) - 1) first, to "truncate" the value of rhs, and then perform the shift.
  2. The mathematical division in the third paragraph, lhs / 2^rhs, isn't exactly right either. The real result is the floor function applied to that division.

Location Errors

FLS

The guideline calls out the FLS in the first paragraph:

    The Rust FLS incorrectly describes this behavior as <`arithmetic overflow <https://github.com/rust-lang/fls/issues/632>`_.

Which is not the place to do so. The FLS has its own repo and Zulip channel, and if they messed up their terminology, we can call them out there.

Amplification

The first section of the Rule is the Amplification. That part is normative. Whatever is in the Amplification should be considered a rule, line by line.

(And again, not the place to call out the FLS, but regardless of that...)

In this section, the guideline explains how shifts work under overflow - this is not the place for doing so. That should go in the Rationale.

Typos

  • Title: Avoid out-*or*-range shifts, or should be of.
  • First paragraph: <arithmetic overflow, that < shouldn't be there.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions