Duplicated index name undetected in nested loops

[philip.hannebohm@…]

When defining nested for-loops, the index variable is not checked for duplication.

From what I can see, the specification is silent on this matter.

The following model shows this, it succeeds when model checked, but shouldn't, right?

package Nested
  model Example
    "Example model with nested for loops"
    final parameter Integer M = 3 "problem height";
    final parameter Integer N = 4 "problem width";
    Real[M,N] A "matrix";
    Real[M] x "vector";
    Real[N] y "vector";
  equation
    x = {sin(2*3.1415926*time*i) for i in 1:M};
    y = {cos(2*3.1415926*time*i) for i in 1:N};
    for i in 1:M loop
      for i in 1:N loop
        A[i,i] = x[i]*y[i];
      end for;
    end for;
  end Example;
end Nested;

[Per Östlund]

From a language point of view there's no issue with this, the inner iterator will simply shadow the outer and any references to i in the inner for-loop will refer to the innermost iterator. Most programming languages allow variables to be shadowed, so there's nothing odd about that.

One could argue that this behaviour does more harm than good though, since it's almost always an error to have nested for-loops with the same iterators. However, it is possible to write nested for-loops that use the same iterator names and which are still correct, even though most would probably consider that bad style.

I'm leaning towards adding a warning for this, even though it's technically allowed.

[Francesco Casella]

If it's not forbidden by the spec, I'm also in favour of a warning.

[philip.hannebohm@…]

Replying to perost:

From a language point of view there's no issue with this, the inner iterator will simply shadow the outer and any references to i in the inner for-loop will refer to the innermost iterator. Most programming languages allow variables to be shadowed, so there's nothing odd about that.

One could argue that this behaviour does more harm than good though, since it's almost always an error to have nested for-loops with the same iterators. However, it is possible to write nested for-loops that use the same iterator names and which are still correct, even though most would probably consider that bad style.

I'm leaning towards adding a warning for this, even though it's technically allowed.

Of course you're right when it comes to algorithm blocks.

If, however, for an equation the outer iterator is shadowed, you get the inner equation several times, which looks like very bad form to me because the system (except for things like i in 1:1) has either too many equations or is structurally singular, or both. I can't see how that doesn't necessarily lead to an error.

On the other hand, probably no one in their right mind would construct such an equation. So maybe a warning is sufficient.

[Francesco Casella]

I fully agree comment:3.

Shadowing in for-equations really makes no sense. We should probably issue a warning for the time being, and open an issue on the Modelica Specification tracker to explicitly prevent this situation, which is definitely lousy modelling style.

For-statements in algorithms are a different story.

[Karim Adbdelhak]

Replying to casella:

I fully agree comment:3.

Shadowing in for-equations really makes no sense. We should probably issue a warning for the time being, and open an issue on the Modelica Specification tracker to explicitly prevent this situation, which is definitely lousy modelling style.

For-statements in algorithms are a different story.

I agree, i don't think that there is any way where duplicate index names would make sense for for-equations, could you open an issue for the specification tracker?

[Per Östlund]

The consensus seems to be that iterator shadowing in for-equations doesn't make any sense, so I've opened ​PR #517 that adds a warning for that case.

[Francesco Casella]

See ​PR #2443 of the Modelica Specification

[Per Östlund]

Fixed in cfa5e031. We might revisit this topic later depending on the resolution of the ticket casella opened for the specification.

[Per Östlund]

My apologies, it seems I somehow failed to merge my PR without realising it. I've merged it properly now.

[Francesco Casella]

@perost, can you please backport this to 1.15.0 and 1.14.0 as well?

[Per Östlund]

Replying to casella:

@perost, can you please backport this to 1.15.0 and 1.14.0 as well?

It was already in 1.15, but I've backported it to 1.14 too now.

[Francesco Casella]

Thank you!