New rule: Set containment checks should be enclosed in parentheses

[fourls]

Prerequisites

• This rule has not already been suggested.
• This should be a new rule, not an improvement to an existing rule.
• This rule would be generally useful, not specific to my code or setup.

Suggested rule title

Set containment checks should be enclosed in parentheses

Rule description

In Delphi, the not unary operator is stronger than the in binary operator, meaning that not MyVar in MySet is parsed as (not MyVar) in MySet instead of the generally expected not (MyVar in MySet).

This can be particularly confusing for integers, for which not is a perfectly valid operator - the example below incorrectly prints MyByte is neither 3 or 252.

procedure RunTest;
var
  MyByte: Byte;
begin
  MyByte := 3;
  Assert(not MyByte = 252);

  if not MyByte in [3, 252] then begin
    Writeln('MyByte is neither 3 or 252');
  end
  else begin
    Writeln('MyByte is 3 or 252!');
  end;
end;

This rule would pick up these cases and suggest using parentheses to make the order of operations clear and intentional.

Rationale

This is a case in which the compiler's interpretation is almost never what the programmer intended. Even if the programmer did mean to check if not X was in the array, parentheses are needed to make this clear to other maintainers.

[Cirras]

Agreed that we should do this, as the operator precedence makes these not <integer> in <set> expressions very confusing and hard to understand.

Set containment checks should be enclosed in parentheses

Should all in checks be enclosed?

This could go a few ways:

• require all containment checks to be enclosed
• require a containment check that's part of a larger expression to be enclosed
• require a containment check to be enclosed only if the parent expression is a unary not

[fourls]

• require a containment check to be enclosed only if the parent expression is a unary not

I would probably go with this approach. Delphi's operator precedence is not intuitive and if we made a rule for every time it was a little unpredictable then we'd be here forever.

We definitely don't want to enclose all in checks - maybe clarifying the rule title could be good. "Negated set containment checks should be enclosed in parentheses"?

[fourls]

An alternate approach to this rule could be to require all bitwise not invocations to be contained in parentheses. They're confusing and take precedence over logical not, which is almost always not what you want.