Is assignment of a derived record instance to a base record instance allowed?

[modelica-trac-importer]

Reported by stefanv on 14 Aug 2013 16:48 UTC
Is the assignment in function F below allowed, such that only r.x and r.y are copied into r0:

record R0
    Real x, y;
end R0;

record R
    extends R0;
    Real z;
end R;

function F
    input Real r;
    output Real r0;
algorithm
    r0 := r;
end F;

---

Migrated-From: https://trac.modelica.org/Modelica/ticket/1245

[modelica-trac-importer]

Comment by jmattsson on 15 Aug 2013 09:24 UTC
I'd say that:

1. It doesn't matter that R extends R0, only what components they have.
2. It has previously been decided (I can't remember when) that record types are only compatible if they have exactly the same set of components. (I think they have to be in the same order, as well - this affects external C functions.)
3. Since r0 and r are not type compatible, the assignment is not allowed.

[modelica-trac-importer]

Comment by hansolsson on 4 Sep 2013 12:07 UTC
Replying to [comment:1 jmattsson]:

I'd say that:

1. It doesn't matter that R extends R0, only what components they have.
2. It has previously been decided (I can't remember when) that record types are only compatible if they have exactly the same set of components. (I think they have to be in the same order, as well - this affects external C functions.)
3. Since r0 and r are not type compatible, the assignment is not allowed.

I agree, but a minor clarification:
There are some languages that do slicing in these cases, but 10.6.1 state that operands in assignments should type equivalent.

[modelica-trac-importer]

Comment by volker.beuter on 13 Nov 2013 20:50 UTC
First I did not understand the point here and was wondering what was the relation between the records and the function defined here. I think, the way F was defined there is no. I guess F was indented to be defined like this:

function F
    input R r;
    output R0 r0;
algorithm
    r0 := r;
end F;

i.e. with record input and output, not {{{Real}} ones.

[modelica-trac-importer]

Comment by stefanv on 13 Nov 2013 20:53 UTC
Ah, yes, you're right Volker. Just a typo on my part.

[modelica-trac-importer]

Comment by sjoelund.se on 4 Apr 2014 08:02 UTC
I think this would be rather important to resolve or clarify. Currently the MSL does not seem to care about the order:
CurvedBend.Geometry and Bend.dp_curvedOverall_IN_con need to be type-compatible to translate some Fluid.Dissipation models, and the records are:

record EdgedBend "Input for bend"
  SI.Diameter d_hyd;
  SI.Angle delta;
  SI.Length K;
end EdgedBend;

record Bend "Input for bend"
  extends EdgedBend;
  SI.Radius R_0;
end Bend;

record dp_curvedOverall_IN_con
  extends Modelica.Fluid.Dissipation.Utilities.Records.PressureLoss.Bend;
  // d_hyd,delta,K,R_0
end dp_curvedOverall_IN_con;

record Geometry "Geometric data for a curved bend"
  SI.Diameter d_hyd;
  SI.Radius R_0;
  SI.Angle delta;
  Modelica.Fluid.Types.Roughness K;
end Geometry;

There are also some user libraries out there that do precisely the r0 := r assignment.

[modelica-trac-importer]

Comment by jmattsson on 4 Apr 2014 08:18 UTC
I agree, we need to have consensus on whether this is allowed or not.

Note that there is a very simple work-around for the user in the case Martin brought up:

r0.d_hyd := r.d_hyd;
r0.delta := r.delta;
r0.K := r.K;
r0.R_0 := r.R_0;

One possible solution would be to not require the elements to be in the same order, and in the external function case just use the order of the declared type in the C struct. I'm not sure what the spec says on that, but I don't see any problems with it.

[modelica-trac-importer]

Comment by sjoelund.se on 4 Apr 2014 08:33 UTC
It's not a good work-around. The types are cast in function calls so you would need a conversion function between the two records. Which is... Ugly. It also needs changes to libraries.

Spec says

Declaration order is only significant for:

• Functions with more than one input variable called with positional arguments, Section 12.4.1.
• Functions with more than one output variable, Section 12.4.3.
• Records that are used as arguments to external functions, Section 12.9.1.3
• Enumeration literal order within enumeration types, Section 4.8.5.

Nothing about type compatibility of records... And since you need to strip out arrays, etc from external structs anyway it is probably fine to allow a different declaration order and still be type compatible.

(I also seem to recall some Modelica libraries structuring their records to all be in lexical order for external functions and records to work correctly in all tools. I guess because the structs would be compatible and just a copy instead of a shuffle...)

[modelica-trac-importer]

Comment by jmattsson on 4 Apr 2014 09:59 UTC
Replying to [comment:7 sjoelund.se]:

It's not a good work-around. The types are cast in function calls so you would need a conversion function between the two records. Which is... Ugly. It also needs changes to libraries.

Sure - not saying we should use that workaround, jut that it was there and (conceptually) simple.

Nothing about type compatibility of records... And since you need to strip out arrays, etc from external structs anyway it is probably fine to allow a different declaration order and still be type compatible.

I wasn't sure what the spec required and didn't go dig, but that is what I would prefer anyway - so from my standpoint no fix is needed on that.

(I also seem to recall some Modelica libraries structuring their records to all be in lexical order for external functions and records to work correctly in all tools. I guess because the structs would be compatible and just a copy instead of a shuffle...)

I recall seeing a support case where a customer had trouble with a specific tool reordering the members of the structs in lexical order before generating C structs for them. It doesn't do that since quite a versions ago, and I haven't heard of any other tools doing that, so I suspect that is simply a legacy practice.

Please note that the discussion about same order or not comes from just the parenthesis in my first comment, that even starts with "I think". Well, it seems I thought wrong about that. It seems pretty clear now that they do not have to be in the same order, and that it is a tool issue to convert to the correct struct when needed.

Is there any opposition to the interpretation that the records involved in an assignment have to contain the same set of members (i.e. same names and compatible types)?

[modelica-trac-importer]

Comment by sjoelund.se on 4 Apr 2014 10:32 UTC
Oh. If you had a function that takes input R0 r0 here and passed it a record R r, I guess we would say it is type incompatible according to the comments above. But Fluid.Dissipation uses that pattern as well...

Modelica.Fluid.Dissipation.PressureLoss.StraightPipe.dp_overall_DP calls Modelica.Fluid.Dissipation.PressureLoss.StraightPipe.dp_laminar_DP with the same inputs it has. But the laminar records have a fewer number of members than the overall ones.

[modelica-trac-importer]

Comment by jmattsson on 4 Apr 2014 10:55 UTC
I would call that a bug in the Fluid library. We should get Francesco's opinion.

[modelica-trac-importer]

Comment by adrpo on 8 Apr 2014 05:47 UTC
Make this ticket a blocker as depending on the interpretation it affects MSL (Modelica.Fluid.Dissipation).

[modelica-trac-importer]

Comment by dietmarw on 8 Apr 2014 06:40 UTC
Adrian, you might wanna specify for what milestone this is supposed to be a blocker for.

[modelica-trac-importer]

Comment by adrpo on 8 Apr 2014 06:42 UTC
Is there any "right-now" milestone?

[modelica-trac-importer]

Comment by jmattsson on 15 Apr 2014 11:45 UTC
The same issue occurs in Modelica.Electrical.Spice3.Internal.MOS2. It contains calls to Spice3.Internal.Mos.mos2CalcInitEquations and Spice3.Internal.Mos.mos2CalcCalcTempDependencies with the third argument of type Spice3.Internal.Mos2.Mos2ModelLineVariables, while the functions both take Spice3.Internal.Mos.MosModelLineVariables (the former has 3 members that are not in the latter).

[modelica-trac-importer]

Comment by jmattsson on 20 May 2014 07:35 UTC
6ac199e
Fixed passing incompatible record to functions in Electrical.Spice3.

This fixes this issue for Spice3 - Fluid is still remaining. The problem in Spice3 looked like a simple copy-paste error, and the functions in question are only called in one place.

[modelica-trac-importer]

Comment by sjoelund.se on 17 Jun 2014 16:33 UTC
Any updates on the progress on the issues remaining with Fluid.Dissipation?

[modelica-trac-importer]

Comment by sjoelund.se on 19 Jun 2014 05:59 UTC
6ac199e merged into 3.2.1 maintenance in f2b1907.

[modelica-trac-importer]

Comment by wischhusen on 20 Jun 2014 11:44 UTC
083bb47 Introduced a record constructor similar to:

  kc_laminar_IN_con IN_con_lam(d_hyd=IN_con.d_hyd, L= IN_con.L, 
target=IN_con.target);

for functions Channel.kc_evenGapOverall_KC, StraightPipe.kc_overall_KC, StraightPipe.dp_overall_DP, StraightPipe.dp_overall_MFLOW.

I could not find an issue with edged and curved bends. Martin S. could you please revert on this issue if it still prevails?

[modelica-trac-importer]

Comment by sjoelund.se on 20 Jun 2014 20:12 UTC
It seems to have worked, so I merged it into 3.2.1 maintenance in 5ec2522. I think those are the last of my found issues (but I only test classes used by experiments in MSL and ModelicaTest, so there could be more). We should close this as fixed with some nice changelog message if no one finds further issues.

[modelica-trac-importer]

Modified by wischhusen on 23 Jun 2014 15:37 UTC

[modelica-trac-importer]

Changelog modified by wischhusen on 23 Jun 2014 15:37 UTC
Casting of records is not allowed. A record constructor was integrated in wrapper functions.

[modelica-trac-importer]

Comment by jmattsson on 14 Jul 2014 07:36 UTC
For the record: our regression tests does an error check of all models and blocks in MSL, and no longer detects this error in any of them. It could still be there in a function that is not used, but that seems unlikely.

[bilderbuchi]

Comment by sjoelund.se on 4 Apr 2014 08:02 UTC I think this would be rather important to resolve or clarify. Currently the MSL does not seem to care about the order: CurvedBend.Geometry and Bend.dp_curvedOverall_IN_con need to be type-compatible to translate some Fluid.Dissipation models, and the records are:

record EdgedBend "Input for bend"
  SI.Diameter d_hyd;
  SI.Angle delta;
  SI.Length K;
end EdgedBend;

record Bend "Input for bend"
  extends EdgedBend;
  SI.Radius R_0;
end Bend;

record dp_curvedOverall_IN_con
  extends Modelica.Fluid.Dissipation.Utilities.Records.PressureLoss.Bend;
  // d_hyd,delta,K,R_0
end dp_curvedOverall_IN_con;

record Geometry "Geometric data for a curved bend"
  SI.Diameter d_hyd;
  SI.Radius R_0;
  SI.Angle delta;
  Modelica.Fluid.Types.Roughness K;
end Geometry;

There are also some user libraries out there that do precisely the r0 := r assignment.

Is it correct that this issue with CurvedBend has never been fixed?
I'm getting an "error: passing 'Modelica_Fluid_Fittings_BaseClasses_Bends_CurvedBend_Geometry' to parameter of incompatible type 'Modelica_Fluid_Dissipation_PressureLoss_Bend_dp__curvedOverall__IN__con'" on compilation in OpenModelica, and from what I can tell the situation sketched above by @sjoelund is still present in MSL 4 (so I don't think it's an OpenModelica problem at this point).

[beutlich]

Is it correct that this issue with CurvedBend has never been fixed?

I guess you are right here.

The invalid assignments are:

ModelicaStandardLibrary/Modelica/Fluid/Fittings.mo

Line 17 in 64b1f57

geometry,

ModelicaStandardLibrary/Modelica/Fluid/Fittings.mo

Line 30 in 64b1f57

geometry,

ModelicaStandardLibrary/Modelica/Fluid/Fittings.mo

Line 2118 in 64b1f57

geometry,

ModelicaStandardLibrary/Modelica/Fluid/Fittings.mo

Line 2167 in 64b1f57

geometry,

@bilderbuchi Could you please open a new issue, referencing this old issue, such that we do not need to reopen this issue. Thanks.

[bilderbuchi]

Sure: #4102.