Skip to content

Conversation

@HansOlsson
Copy link
Collaborator

Closes #3049
Note that it is slightly different from the issue-text.
The key part is simply: don't use a class in such a way that it the class-reference can be seen if the corresponding class-contents cannot be shown as that doesn't make sense.

Copy link
Collaborator

@maltelenz maltelenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to little real world experience using the higher level of protections to give good feedback on this.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way or another, I think there should be less room for interpretation. My preference would be to have a concrete set of rules rather than something generic with examples.

@HansOlsson HansOlsson added this to the Phone 2022-4 milestone Sep 6, 2022
@HansOlsson HansOlsson requested a review from beutlich September 29, 2022 11:36
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the rule that tools are held responsible for not revealing information when protection usage restrictions have been violated. I know others have a different opinion here, so I'm just suggesting that an example is added to show the consequences of this rule more clearly.

@d-hedberg
Copy link

If a tool allows libraries to be produced where protection usage restrictions have been violated, these libraries must be considered corrupt.

The only sensible and robust way of dealing with this by other tools (that do have a mechanism in place to detect and prevent this violation) would be to not allow such a library to load at all. I fail to see why the responsibility to deal with such a corrupt library produced by one tool should fall on the other tools (more than to inform the user that the library violates the protection usage restrictions and cannot be permitted to load).

@HansOlsson
Copy link
Collaborator Author

If a tool allows libraries to be produced where protection usage restrictions have been violated, these libraries must be considered corrupt.

The only sensible and robust way of dealing with this by other tools (that do have a mechanism in place to detect and prevent this violation) would be to not allow such a library to load at all. I fail to see why the responsibility to deal with such a corrupt library produced by one tool should fall on the other tools (more than to inform the user that the library violates the protection usage restrictions and cannot be permitted to load).

I find that a bit harsh - but it is consistent with not exposing the wrong contents.

@henrikt-ma
Copy link
Collaborator

The only sensible and robust way of dealing with this by other tools (that do have a mechanism in place to detect and prevent this violation) would be to not allow such a library to load at all. I fail to see why the responsibility to deal with such a corrupt library produced by one tool should fall on the other tools (more than to inform the user that the library violates the protection usage restrictions and cannot be permitted to load).

I find that a bit harsh - but it is consistent with not exposing the wrong contents.

I like the simplicity of this approach, and I think it would serve well as an example in the specification of how a tool may interpret the requirement – better than the examples I've suggested above.

@henrikt-ma
Copy link
Collaborator

henrikt-ma commented Oct 6, 2022

Another weird situation for a tool to handle is when EncryptedLib.A has Access.hide, and the user enters the text of this top-level model:

model M
  EncryptedLib.A a;
end M;

Should the tool suddenly close the text view and just show the icon of M? As I see it, it is not reasonable to require that a tool takes action to not reveal the existence of EncryptedLib.A when this happens. Instead, I think the tool's responsibility should be to give an error when flattening M, and also refuse to save M to permanent storage. (This, in combination with refusing to load such a definition of M from permanent storage, as proposed by @d-hedberg.)

@HansOlsson HansOlsson requested a review from henrikt-ma October 7, 2022 11:03
@HansOlsson HansOlsson modified the milestones: Phone 2022-4, Phone 2022-5 Oct 10, 2022
HansOlsson and others added 4 commits October 11, 2022 11:33
Co-authored-by: Henrik Tidefelt <[email protected]>
Co-authored-by: Henrik Tidefelt <[email protected]>
Co-authored-by: Henrik Tidefelt <[email protected]>
Co-authored-by: Henrik Tidefelt <[email protected]>
@HansOlsson
Copy link
Collaborator Author

Language group:
System modeler (Henrik): should be possible to refuse to load if not consistently used in visible part of library (negotiate with library developer). Should be seen as lookup error.

Tools should support developing such libraries similarly as other libraries without any restrictions, restrictions only apply when it is used.

@beutlich beutlich removed their request for review October 14, 2022 19:30
@beutlich beutlich dismissed their stale review October 14, 2022 19:31

resolved

@henrikt-ma
Copy link
Collaborator

I'd be interested in going forward with the things agreed upon at the last phone meeting, but it would be good if currently open conversations could be resolved first to avoid GitHub suggestion chaos.

Co-authored-by: Henrik Tidefelt <[email protected]>
@HansOlsson
Copy link
Collaborator Author

Phone meeting:
Tools should ideally have some way of checking this before encrypting.

The discussion focuses on two issues:

  • How illegal is it to violate the rules? (Should vs must)
  • Accessing text that uses protected classes.

HansOlsson and others added 2 commits November 9, 2023 15:40
Co-authored-by: Henrik Tidefelt <[email protected]>
Having a requirement that prevents a tool from just showing the text may have a bad performance impact.
@HansOlsson HansOlsson requested a review from henrikt-ma November 9, 2023 15:04
@HansOlsson
Copy link
Collaborator Author

I'm not seeing any sensible solution for the issue in practice, except the currently proposed one.

The issue is:

  • A library has been encrypted - and for some reason some model has a diagram that uses a model that shouldn't be exposed.
  • Without requiring that tools instantiate all models in the library to ensure that they satisfy the requirements before showing anything.

Co-authored-by: Elena Shmoylova <[email protected]>
@HansOlsson HansOlsson modified the milestones: Phone 2022-5, Phone 2023-6 Nov 24, 2023
@HansOlsson
Copy link
Collaborator Author

Language group:
Favor 'should': Hans, Stephan, Elena
Against ('may not'):
Abstain: Dag, Quentin, Gerd, Markus

@HansOlsson
Copy link
Collaborator Author

I'd be interested in going forward with the things agreed upon at the last phone meeting, but it would be good if currently open conversations could be resolved first to avoid GitHub suggestion chaos.

Everything should now be resolved.

@henrikt-ma
Copy link
Collaborator

Language group: Favor 'should': Hans, Stephan, Elena Against ('may not'): Abstain: Dag, Quentin, Gerd, Markus

As this doesn't make any sense to us at Wolfram (Quentin may not have taken part of the discussions), I don't foresee System Modeler ever complying with the current formulation.

@henrikt-ma henrikt-ma dismissed their stale review December 6, 2023 16:22

Dismissing my review because I don't want to turn it into an approval

@HansOlsson HansOlsson merged commit 5b9562c into modelica:master Dec 11, 2023
@HansOlsson HansOlsson deleted the RestrictAccessUse branch December 11, 2023 07:44
@HansOlsson HansOlsson added the M37 For pull requests merged into Modelica 3.7 label Jan 17, 2024
@HansOlsson HansOlsson added the clarification Specification of feature is unclear, but not incorrect label Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clarification Specification of feature is unclear, but not incorrect M37 For pull requests merged into Modelica 3.7

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Has the Access.hide annotation a meaning for instances of the class?

6 participants