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;

[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.

[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.

[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.Abdelhak]

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?

[perost]

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.

[casella]

See ​PR #2443 of the Modelica Specification

[perost]

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

[perost]

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

[casella]

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

[perost]

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.

[casella]

Thank you!