Strange diagnostics with diamond-inheriting from a class with equations

[anatoly.trosinenko@…]

When inheriting through different paths from the same class containing both variables and equations, variables are merged and equations are not. Supposing this is correct, diagnostics looks quite strange: it suggests I have "not big enough" equation, but the list of unsolved variables is empty -- or does it contain -1 variable?.. :)

How to reproduce

Take the following package:

package TestInheritance

  model Base
    input Real x;
    output Real y;
  equation
    y = x;
  end Base;
  
  model A 
    extends Base;
  end A;
  
  model B
    extends Base;
  end B;
  
  model AB
    extends A;
    extends B;
  end AB;

end TestInheritance;

... and select the AB model.

Check it:

Check of TestInheritance.AB completed successfully.
Class TestInheritance.AB has 2 equation(s) and 1 variable(s).
2 of these are trivial equation(s).

Instantiate it:

class TestInheritance.AB
  input Real x;
  output Real y;
equation
  y = x;
  y = x;
end TestInheritance.AB;

Frankly speaking, I don't know how this should be handled according to the standard, but up to this point it looks consistent.

Now try to simulate it: you get the following error

[1] 11:53:00 Symbolic Error
Too many equations, over-determined system. The model has 2 equation(s) and 1 variable(s).

[2] 11:53:00 Symbolic Warning
[TestInheritance: 7:5-7:10]: Equation 2 (size: 1) y = x is not big enough to solve for enough variables.
  Remaining unsolved variables are: 
  Already solved: y
  Equations used to solve those variables:
    Equation 1 (size: 1): y = x

And this diagnostics looks quite strange.

What is expected

Ideally, the compiler should show a warning (or even error?) explicitly stating two paths of inheritance of Base. Can some model with diamond inheritance from a model with non-zero number of equations be simulatable at all?

[Francesco Casella]

Section 7.1 of the specification says:

Equations of the flattened base class that are syntactically equivalent to equations in the flattened enclosing class are discarded. This feature is deprecated, and it is recommend to give a warning when discarding them and for the future give a warning about all forms of equivalent equations due to inheritance. [Note: equations that are mathematically equivalent but not syntactically equivalent are not discarded, hence yield an overdetermined system of equations.]

In fact, only the new front-end (-d=newInst) produces the reported output, which I understand is correct, because removing the redundant equation would be a deprecated feature. As such, I'm not sure we really want to implement it.

The old front-end only keeps one equation, but it will be discontinued in 2019.

@perost, can you please comment on this?

@anatoly, for the record please note that any issue regarding instantiation concerns the front-end, not the backend.

[Per Östlund]

Replying to casella:

@perost, can you please comment on this?

We haven't implemented it in the new frontend since it's deprecated and expensive, and I haven't seen any model that needs it yet. If it turns out to be needed it would be fairly trivial to implement though.

Also, the old frontend doesn't implement it correctly, since it shouldn't actually remove any equations in this case according to the specification. Note that it only says that equations that are equivalent to equations in the flattened enclosing class (I assume this means the inheriting class) are removed, while in this case they're equivalent to other inherited equations. And if you add the equation to AB too:

model AB
  extends A;
  extends B;
equation
  y = x;
end AB;

you'll get two equations in the flat model, again not according to the specification.

[Francesco Casella]

Replying to perost:

We haven't implemented it in the new frontend since it's deprecated and expensive, and I haven't seen any model that needs it yet. If it turns out to be needed it would be fairly trivial to implement though.

@anatoly, if you are aware of models actually needing this, feel free to reopen the ticket. In that case, I'd suggest to only allow this when some flag is turned on.

[anatoly.trosinenko@…]

Technically, I have stumbled upon this issue with real models, but I don't propose to specifically implement this feature. After your explanation, it became clear to me that this is not something that have to be implemented. The best I'm asking for, is to implement some clearer warning to help users with porting their models from old to new frontend in case someone used this deprecated feature.

Not reopening this issue on my own, since I'm not completely sure it is worth spending time on implementing it.