Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification about encapsulated short class definition #3270

Closed
qlambert-pro opened this issue Oct 31, 2022 · 9 comments · Fixed by #3282
Closed

Clarification about encapsulated short class definition #3270

qlambert-pro opened this issue Oct 31, 2022 · 9 comments · Fixed by #3282
Assignees

Comments

@qlambert-pro
Copy link
Collaborator

Section 4.5.1 says:

An exception to the above is that if the short class definition is declared as encapsulated, then the modifiers follow the rules for encapsulated classes and cannot be looked up in the enclosing scope.

Which only provides a constraints for the modifiers.

Does that mean that the following is valid:

package P
  model A
    Real x = 1;
  end A;

  encapsulated model B = A;
end P;
@HansOlsson
Copy link
Collaborator

It shouldn't be allowed.
The reason is two-fold:

  • It breaks the idea with encapsulation.
  • The problem with modifiers is that it can make a class depending on local variables (when allowing using encapsulated models outside of the enclosing class), and you can indirectly do the same with classes.

It was added to resolve #2743 (where the class names used .P.A).

With indirect I mean that the specification currently contains the following to illustrate the rule:

model A
 parameter Real R;
...
 encapsulated model Load2=.Resistor(R=2);
end A;

But you could also do:

model A
 parameter Real R;
...
 model Load3=.Resistor(R=2);
 encapsulated model Load4=Load3;
end A;

Where Load4 had the same issue with modifiers as Load2.

@qlambert-pro
Copy link
Collaborator Author

Right then we need to rephrase the specs because the suggestion that it only applies to modifiers is ambiguous.

@qlambert-pro
Copy link
Collaborator Author

qlambert-pro commented Oct 31, 2022

You seem to mean R=R for the modifiers. Wouldn't the following be allowed and equivalent by your interpretation?

package P
  model A
    Real x = 1;
  end A;

  model Main
    parameter Real p = 2;
    model B = A(x = p);
    encapsulated model C = .P.Main.B;
    C c;
  end Main;
end P;

@HansOlsson
Copy link
Collaborator

You seem to mean R=R for the modifiers. Wouldn't the following be allowed and equivalent by your interpretation?

package P
  model A
    Real x = 1;
  end A;

  model Main
    parameter Real p = 2;
    model B = A(x = p);
    encapsulated model C = .P.Main.B;
    C c;
  end Main;
end P;

Local classes can be a problem in general, but P.Main isn't a package and B isn't encapsulated so you are not allowed to use it.

@qlambert-pro
Copy link
Collaborator Author

qlambert-pro commented Oct 31, 2022

That's not what the specs currently say:

If the identifier denotes a class, [...] If the class does not satisfy the requirements for a package, the
lookup is restricted to encapsulated elements only.

The way it is written only applies to the first part of the name being a package.

Also, I am not sure what the following mean:

that class is temporarily flattened ([...]) and using the enclosing classes of the denoted class

I guess we should remove and?

@HansOlsson
Copy link
Collaborator

That's not what the specs currently say:

If the identifier denotes a class, [...] If the class does not satisfy the requirements for a package, the
lookup is restricted to encapsulated elements only.

The way it is written only applies to the first part of the name being a package.

Oh, that restriction must be present for all parts (in a normal recursive way). Similarly for the partial restriction.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Oct 31, 2022

Also, I am not sure what the following mean:

that class is temporarily flattened ([...]) and using the enclosing classes of the denoted class

I guess we should remove and?

Hm... I believe we should move the 'and ...' into the parenthesis, giving:

If the identifier denotes a class, that class is temporarily flattened (as if instantiating a component without modifiers of this class, see section 7.2.2 and using the enclosing classes of the denoted class).

@qlambert-pro
Copy link
Collaborator Author

qlambert-pro commented Oct 31, 2022

If the identifier denotes a class, that class is temporarily flattened (as if instantiating a component without modifiers of this class, see section 7.2.2 and using the enclosing classes of the denoted class).
ok

For the original issue, as well as specifying that the restriction also applies to the base class itself, we could add a non-normative text pointing out that for an encapsulated short-class definition the base class reference must be fully qualified.

@gwr69
Copy link
Contributor

gwr69 commented Nov 1, 2022

If the class does not satisfy the requirements for a package, the
lookup is restricted to encapsulated elements only.

It may be helpful to point out that this references section 5.3.2 (Composite Name Lookup).

@HansOlsson HansOlsson self-assigned this Nov 20, 2022
HansOlsson added a commit to HansOlsson/ModelicaSpecification that referenced this issue Nov 21, 2022
Change grammar for two reasons:
* It is type-specifier not IDENT
* For encapsulated we really need the leading "." which is part of type-specifier.
Closes modelica#3270
HansOlsson added a commit that referenced this issue Feb 9, 2023
* Clarify encapsulated.
Change grammar for two reasons:
* It is type-specifier not IDENT
* For encapsulated we really need the leading "." which is part of type-specifier.
Closes #3270
* Clarify composite name lookup (related to encapsulated).
* Clearer on global name lookup.
* Consistent use of terminology.
* Update chapters/classes.tex
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants