search

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
471 stars 168 forks source link

ModelicaServices.ExternalReferences.loadResource violates purity rules #3655

Open henrikt-ma opened 3 years ago

henrikt-ma commented 3 years ago

The demo implementation of ModelicaServices.ExternalReferences.loadResource isn't marked impure,

https://github.com/modelica/ModelicaStandardLibrary/blob/81dd1b63a61075c85fbd26945eb333b202b9e4b9/ModelicaServices/package.mo#L164

Yet it calls the impure Modelica.Utilities.Files.fullPathName,

https://github.com/modelica/ModelicaStandardLibrary/blob/81dd1b63a61075c85fbd26945eb333b202b9e4b9/Modelica/Utilities/Files.mo#L568

Even though this really just concerns a dummy implementation of ModelicaServices, I'd interpret the purity of loadResource as dictating how it is also supposed to be handled in actual implementations of ModelicaServices. For example, marking it impure clearly shows that a call to loadResource isn't allowed in a constant's declaration equation.

henrikt-ma commented 3 years ago

@maltelenz just pointed out to me that not having loadResource marked impure makes the deprecated semantics kick in, where the impure function can actually be used to initialize a constant:

A deprecated semantics is that external functions (and functions defined in Modelica directly or indirectly calling them) without pure or impure keyword are assumed to be impure, but without any restriction on calling them. Except for the function Modelica.Utilities.Streams.print, diagnostics must be given if called in a simulation model.

This means that loadResource would need to be pure in order to be backwards compatible, suggesting that when called with a constant argument, the call could be evaluated already during translation. I'm not sure this counts as being backwards compatible in a good way…

HansOlsson commented 1 year ago

The purity rules are in general messy and might need further revisions, and loadResource is no exception.

In one sense loadResource has impure semantics, as it has a side-effect in terms of referencing the external resource (and tools do external changes based on that). In another sense the intent inside the model is that the resource is just there and not changed by loadResource. But then there's the fact that it is normally used together with reading the referenced external resource and that is less pure - so I'm not sure if marking it as impure would be that problematic, but doing it just before a minor revision is far from ideal.

As for fullPathName it is impure in the sense that it under Windows depends on the current directory (and possibly some other settings), and on under Linux additionally follows symbolic links.