New rule: Array iteration should be in range

[fourls]

Prerequisites

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

Suggested rule title

Array iteration should be in range

Rule description

This rule would pick up on cases where a loop iterator may become out of range of the underlying array, by determining that the upper bound of iteration is less than or equal to the length of the array - 1. For example:

for I := 0 to Length(MyArr) do begin
  // ...
end;

This could also be extended to lists:

for I := 0 to MyList.Count do begin
  // ...
end;

Rationale

It's a very common problem to write for I := 0 to Length(MyArr), as it looks correct at first glance. This rule would catch those cases before causing a range check error.

[cirras]

Agreed. We should definitely include common collection types in this.

[Skarvion]

While it's a bad practice in the first place, should we cater for these types as well?

var
  Foo: array[1..5] of String;

So ensure that for I := ... starts at the first index?

[fourls]

I did actually consider this when initially making the proposal, which is why the name of the rule is so vague 😄 I decided not to include it because of some vague idea about it being difficult to generalise to custom list types, although now I think about it I think it would be best to just only support RTL types - TList, TStringList, string, array of x, etc.

I'm not opposed to this, although I wonder if it might be best split into two rules?

[Skarvion]

After thinking about it some more, there can be cases where certain algorithm may ignore the some of the first elements. It's definitely smelly to do that, but hard to capture all possible scenarios where those might be appropriate.

So yes, we should split to another rule. I have raised #117 , my comment earlier is still smelly regardless, no good reason to start from non-zero.

[zaneduffield]

After thinking about it some more, there can be cases where certain algorithm may ignore the some of the first elements. It's definitely smelly to do that, but hard to capture all possible scenarios where those might be appropriate.

So yes, we should split to another rule. I have raised #117 , my comment earlier is still smelly regardless, no good reason to start from non-zero.

I'd argue that the author could make their intentions clear by doing

for var I := Low(Typ) + 1 to High(Typ) do

[fourls]

After thinking about it some more, there can be cases where certain algorithm may ignore the some of the first elements. It's definitely smelly to do that, but hard to capture all possible scenarios where those might be appropriate.

I don't necessarily agree that it's a code smell to start past the first index - for example, it's the most elegant way to write the following code:

function HasDuplicates(Arr: TArray<string>): Boolean;
var
  I: Integer;
begin
  Result := False;

  for I := 1 to Length(Arr) - 1 do begin
    if Arr[I] = Arr[I - 1] then begin // Starting at I=1 means I-1 is always valid
      Result := True;
      Exit;
    end;
  end;
end;

I'd argue that the author could make their intentions clear by doing

for var I := Low(Typ) + 1 to High(Typ) do

I agree, but I don't think it's necessary to penalise developers for not doing so. I suspect most existing code that uses this pattern would not use Low and High.

In summary - I agree that it's probably best to keep this rule as described in the initial description, since it's much less opinionated. We can discuss more in #117.