__OpenModelica_tearingSelect annotation uses undeclared identifiers

[perost]

The __OpenModelica_tearingSelect annotation currently uses simple identifiers as the values, e.g. __OpenModelica_tearingSelect=always. In the context of Modelica this makes little sense since e.g. always isn't declared anywhere, and if it where (which probably would be a really bad idea) it wouldn't be a value in itself but a variable with some binding equation.

This can be problematic since annotations are expected to follow the normal Modelica rules, and breaking that expectation makes it harder to deal with in the compiler. For example, in #11948 I had to add an exception specifically for __OpenModelica_tearingSelect because of this.

As I see it there are two alternatives, using strings ("always") or an enumeration (e.g. TearingSelect.always). I would lean towards strings since that doesn't require adding anything to the top-level scope.

We should obviously also keep the current semantics for a while since people use it, but add a deprecation warning for it.

[perost]

Ping @casella @kabdelhak @phannebohm for input.

[casella]

I would recommend __OpenModelica_tearingSelect =TearingSelect.always, to mimick StateSelect.always, since tearingSelect is similar in scope and spirit to stateSelect, and my goal is to eventually have it as an attribute in the Modelica language.

[phannebohm]

I agree we shouldn't leave it like it is. TearingSelect.always looks nicer to me, especially when we want to have it as an actual attribute some day.

On the other hand the modeler is allowed to have a class called TearingSelect as of the current language spec, which could lead to name conflicts. I guess that's what you meant with adding to the top-level scope, @perost.
So maybe we could name it __OpenModelica_TearingSelect.always? It's quite bulky though.

[perost]

On the other hand the modeler is allowed to have a class called TearingSelect as of the current language spec, which could lead to name conflicts. I guess that's what you meant with adding to the top-level scope, @perost. So maybe we could name it __OpenModelica_TearingSelect.always? It's quite bulky though.

Yes, that was my concern. But thinking a bit more about it we shouldn't add it to the top-level scope but to the annotation scope, which is checked last and only in annotations. That way there won't be any name conflicts and a userdefined TearingSelect class will instead override the builtin one (which will break the annotation though).

I tried looking up what other vendors are doing but I couldn't find any examples of using enumerations in vendor annotations, only strings and magic numbers instead.

[casella]

Yes, that was my concern. But thinking a bit more about it we shouldn't add it to the top-level scope but to the annotation scope, which is checked last and only in annotations. That way there won't be any name conflicts and a userdefined TearingSelect class will instead override the builtin one (which will break the annotation though).

Absolutely.

The point of __OpenModelica annotations is that we are free to define whatever annotations we want, whereas variable attributes must be defined by the Modelica Language Specification; if we write

Real x(TearingSelect = tearingSelect.always);

this would actually be illegal code and it could be rejected by other Modelica tools.

[perost]

Yes, that was my concern. But thinking a bit more about it we shouldn't add it to the top-level scope but to the annotation scope, which is checked last and only in annotations. That way there won't be any name conflicts and a userdefined TearingSelect class will instead override the builtin one (which will break the annotation though).

Absolutely.

The point of __OpenModelica annotations is that we are free to define whatever annotations we want, whereas variable attributes must be defined by the Modelica Language Specification; if we write

Real x(TearingSelect = tearingSelect.always);

this would actually be illegal code and it could be rejected by other Modelica tools.

That's not really relevant here, the issue is that if we define it as __OpenModelica_TearingSelect = TearingSelect.always, then we've added a nonstandard TearingSelect enumeration type. In other words, there's no standard way to add a vendor specific enumeration type for use in a vendor specific annotation.

[casella]

That's not really relevant here, the issue is that if we define it as __OpenModelica_TearingSelect = TearingSelect.always, then we've added a nonstandard TearingSelect enumeration type. In other words, there's no standard way to add a vendor specific enumeration type for use in a vendor specific annotation.

I get your point, but as a matter of fact, all tools other than OMC can just skip __OpenModelica annotations entirely, without even the need of actually parsing them further. So why bother. Do I miss something?

[perost]

That's not really relevant here, the issue is that if we define it as __OpenModelica_TearingSelect = TearingSelect.always, then we've added a nonstandard TearingSelect enumeration type. In other words, there's no standard way to add a vendor specific enumeration type for use in a vendor specific annotation.

I get your point, but as a matter of fact, all tools other than OMC can just skip __OpenModelica annotations entirely, without even the need of actually parsing them further. So why bother. Do I miss something?

I'm not really concerned about other tools, as long as the annotation uses standard syntax it shouldn't cause any issues. It's more that if we want to add enumeration types that can be instantiated by the compiler (which we do for e.g. getModelInstance) then there's no way to do that without possibly causing issues with user models if they happen to declare elements with the same name.

The likelyhood that adding a TearingSelect enumeration will actually cause issues is of course very small since users don't have much reason to use that name in their own models, but polluting the global namespace is still something we shouldn't do lightly.

[casella]

I agree that we shouldn't pollute the namespace, but it is also true that there the chance of this being a problem is much lower than the chance that omc segfaults because of some other issue. I would tentatively add it, and possibly remove it or change it if somebody complains.