modelica / ModelicaSpecification

Specification of the Modelica Language
https://specification.modelica.org
Creative Commons Attribution Share Alike 4.0 International
101 stars 41 forks source link

Name specification in selective model extension #3312

Closed eshmoylova closed 1 year ago

eshmoylova commented 1 year ago

I would like a clarification or an agreement on when to enforce the following rule for Model Selection. The syntax for inheritance-modification states:

inheritance-modification :
    break ( connect-equation | IDENT )

Among the examples provided with the corresponding MCP there was one showcasing an invalid case:

model QualifiedNameDeselection
  "Invalid: Only top-level components can be deselected."
  extends A(break b.sign);
...

According to what it says in the specification right now, I would consider it a syntax error, and the model should fail on parsing. Is that the desired outcome?

The reason for asking is the hope to achieve consistency among tools. If the error is not considered as a syntax error, that model can be included in a library, and the library can be imported successfully (up to a point), and the error would only be detected only when that model is used, and the rest of the library is not affected by it. If, however, the error is considered as a syntax error, the parser looks at all files and that file could be potentially rejected. If there are any other models in the same file, they may also be rejected if the parser stops after encountering a syntax error.

It may be tempting to delay the check until after the parsing is done, only when that particular model is processed. But that may mean some errors may survive undetected and would affect a portability between the tools. MSL includes a parser run in the workflow through GitHub Actions. Assume a developer ran some tests in their tool of choice but the tests did not include the invalid model, or included it with TestCase(shouldPass=false) , and then tries to upload it to MSL. What should happen? (Asking as the person in charge of updating the currently used parser.)

@christoff-buerger, @beutlich?

HansOlsson commented 1 year ago

Yes, that should be a syntax error, as it seems better to detect it early.

christoff-buerger commented 1 year ago

@eshmoylova: I can not follow your general concern. How exactly you present errors is tool dependent and a question of tool quality. The specification only defines what is permitted and what not. It can use any means to do that: from natural language to more formal means like context-free grammars. In this case, the context-free grammar of the Modelica language specification was chosen because I considered it the most precise and concise means; but I could also have picked a more relaxed context-free grammar with qualified names and add the restriction to non-qualified as a natural language paragraph. From the viewpoint of validity and specification this makes no difference. But as said, I think to use the grammar is more concise.

This does by no means imply, that your tool must decide if something is a valid deselection name throughout parsing. It only means, that your tool must reject qualified names in deselections -- how it does so and how you present the issue to users is completely up to the tooling.

To give some ideas what I mean:

eshmoylova commented 1 year ago

@christoff-buerger, I am quite blatantly asking if tools are going to implement what the specification says. How is it currently implemented in Dymola? Will there be an actual error for that model or will the invalid deselection be simply ignored? I ran that model in the version of Dymola that I have, and there was no error: the invalid deselection was simply ignored. But I do not have the latest version of Dymola. So, I am asking here: Is that how Dymola still implements it? And if it is, is there a plan to keep it like that? And if Dymola will keep the implementation the way it was in the earlier version, are you as a the author of the proposal OK with that? And if this is a valid way to treat an invalid model then we should say so in the specification right now. Because see #3309. The specification clearly said one thing, and yet the tools were implementing it the other way. And then we end up with user models that work in one tool and not in the other. And years later we change the specification to match what (some of) the tools do because now we have lots of user models, and also it makes some sense, so why keep that restriction, right? So, I am asking if the prototype implementation has indeed implemented what is written in the specification. What was implemented in that version of Dymola (ignoring invalid deselection) can also be a valid way to deal with such models. And if the deselection really meant to remove something meaningful and didn't, we would get an error later for one reason or another.

HansOlsson commented 1 year ago

@christoff-buerger, I am quite blatantly asking if tools are going to implement what the specification says. How is it currently implemented in Dymola? Will there be an actual error for that model or will the invalid deselection be simply ignored?

Short answer: Just implement the grammar as written and it will be fine.

Longer answer: There are two parts:

Explanation:

The reason is that as Christoff notes there are two issues with parsing errors:

Obviously those two issues could create problems for users - and the solution we found is to also report parsing errors when using the class (I can't remember exactly in what cases).

Rambling extra: The problem with grammar vs. semantic problems is somewhat messy in the current Modelica specification as there are some constructs that really should be forbidden in the grammar, but since it is written as an LL(k)-grammar instead of a more general grammar (like LR or a more general context-free grammar) that isn't really possible.

E.g., https://specification.modelica.org/master/functions.html#output-formal-parameters-of-functions has the following that ideally should be a grammatical error: ( x +1 , 3.0 , z / y ) = f (1.0 , 2.0) ; If someone uses an LR-grammar I don't see why we should forbid them from detecting this during parsing. (Rewriting the grammar to drop the LL(k)-requirement is a different topic.)

christoff-buerger commented 1 year ago

How is it currently implemented in Dymola? Will there be an actual error for that model or will the invalid deselection be simply ignored?

Using qualified names is an error. The specification is clear. Tools must report an error; simulation is not possible; anything else is a tool implementation error.

I like to add another thought to what @HansOlsson said. In an interactive IDE, one of course likes to support editing of even broken models (i.e., we do not just abort with an error and leave the user to external editors to fix the qualified name). To that end, we very likely will process such selective model extensions with qualified names and any related semantics (like name analysis, type analysis for model selections etc). This does by no means mean that the model magically becomes valid; any use of it will be marked with respective error messages and simulation obviously is not possible.

Hence, we will process even qualified names and do two things:

  1. Give as good error messages as possible (hence, not just "parsing error, bad luck").
  2. Provide our ordinary interactive -- particularly graphical -- means to develop on such models (including the means to fix the error). This point requires semantic analyses.

But that is a pure tool quality issue. You can also just abort any processing or interaction with an error message.

HansOlsson commented 1 year ago

I believe it has now been clarified.