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

Allow marking fields/instances as private #525

Open
dgelessus opened this issue Feb 28, 2019 · 7 comments
Open

Allow marking fields/instances as private #525

dgelessus opened this issue Feb 28, 2019 · 7 comments

Comments

@dgelessus
Copy link
Contributor

A private instance/field would be usable elsewhere in the spec, but not by user code that uses the compiled spec. This would be useful in a couple of cases:

  • Fields that are parsed in multiple "steps": the raw data is first parsed using a simple type, then translated to a nicer representation using an instance (the field used for the first parsing step could be marked private)
  • Value instances that are used to simplify some expressions in the spec, but aren't useful to spec users
@KOLANICH
Copy link

KOLANICH commented Mar 2, 2019

we are all consenting adults here

(cannot find the original message, the quote is usually attribbuted to Guido van Rossum and used as an argument on why python doesn't have private properties)

IMHO private and protected modifiers are unnecessary and only complicate the things. Sometimes I have to do

#define private public
#include <...>
#undef private

, which violates the standard, but the lack of burden of forking and maintaining an own fork or patching in build process outweights this.

IMHO it is deeply wrong to introduce such artificial restrictions.

@GreyCat
Copy link
Member

GreyCat commented Mar 2, 2019

@dgelessus I'd like to outline several things to think about.

KSY element

Would something like this, added for all seq and instances:

access: private|public # public being default

will work? Any better ideas?

Implementation

How exactly would that be mapped to code generated for every particular language? Most languages indeed have private, protected, public with at least some degree of similarity of semantics. Moreover, we typically already do "private members + public getters" scenario, and even our own generated invocations use public getters unconditionally for sake of compatiblity, ensuring laziness, etc.

For example, implementing this for Java will most likely result in:

  • omitting generation of a public getter (easy)
  • changing internally generated references to this attribute foo() to use internal member where getter is specifically not available due to attribute being private — foo (moderate)

For Python, we could probably change attribute name to something like self._m_foo, akin to what is happening with instances, and, again, we'll have to introduce extra complexity generating internal names to know whether it is self.foo or self._m_foo.

JavaScript will probably also follow Python's suit here — also this._m_foo instead of a property.

Cross-type access

Cross-type access would be also constrained by this implementation, i.e. you won't be able to access like that:

seq:
  - id: foo
    type: one
instances:
  value:
    foo_secret: foo.very_secret # <= this will fail most likely anyway due to lack of public getter
types:
  one:
    seq:
      - id: very_secret
        type: u1
        access: private

That would require extra validation check in precompile phase.

@dgelessus
Copy link
Contributor Author

Perhaps I should have worded the description better 😃 My suggestion is less about outright preventing spec users from accessing certain parts, and more about being able to mark parts of the spec as "not part of the API, don't rely on this". Basically I want to be able to say "this field/instance is only here because of the particular way I wrote the spec, it's not part of the file format and might change or go away when I update the spec".

In many compiled languages this is done with access modifiers, which is why I called my suggested modifier "private", but perhaps that's not the right term. In particular, I don't think the modifier should affect usage inside the spec itself - the field/instance in question should still be accessible from anywhere in the spec, even in other types. So the Kaitai compiler wouldn't perform any access checks of its own, the modifier would only affect how the field/instance appears in the generated code.

What exactly this means for the generated code depends on the target language of course, but in general I would try to follow the language conventions. That means in languages like Python or JavaScript you would put a prefix before the name to indicate that it's not for external use (without actually restricting access, because consenting adults), and in languages like Java or C++ you would change the access modifier (but maybe not literally to private, that might be too restrictive for what we want).

@GreyCat Regarding the Java codegen, I don't think removing the getter is necessary - it should be enough to change the getter's access modifier. That way all other generated code can continue to use the getter, regardless of whether it's public or not.

@GreyCat
Copy link
Member

GreyCat commented Mar 2, 2019

In particular, I don't think the modifier should affect usage inside the spec itself - the field/instance in question should still be accessible from anywhere in the spec, even in other types. So the Kaitai compiler wouldn't perform any access checks of its own, the modifier would only affect how the field/instance appears in the generated code.

I can't think of an easy way to achieve that. KS-generated classes are still classes, and if there would be two different classes, normal public/private restrictions will apply, so the situation I've demonstrated with "Cross-type access" would still fail to compile — either with a foo.verySecret() being deemed "unknown method" (because the getter wasn't even generated) or "unable to access private method".

Scoping access to exactly KS-generated code would be probably relatively hard. There are different features for that in some languages (like internal in C# or default package scope in Java), but this is probably not exactly what we want and we still can't rely on it to exist anyway.

@dgelessus
Copy link
Contributor Author

here are different features for that in some languages (like internal in C# or default package scope in Java), but this is probably not exactly what we want and we still can't rely on it to exist anyway.

Hm, to be honest I wasn't aware that the "semi-private" access modifiers vary so much between languages - C#'s internal seems to have a much wider scope than Java's package-private access, and C++ doesn't have this kind of modifier at all. You're probably right, it would be difficult to generate code for all langauges that consistently has the access levels that we want.

I still think that it would be too restrictive if "private" members could only be accessed from the same type. From my experience so far, Kaitai type declarations sometimes have to be very fine-grained, because either the file format structure or Kaitai requires it. In such cases you might have fields that are useful/necessary for other types, but are not meant for the end user.

Since it's not possible to implement this (consistently) using access modifiers, we could leave everything public and only change the identifiers. At that point it might not even be necessary to add a new language feature for this, you could just use a specific naming convention in this case. I'd be okay with either option, as long as I can somehow indicate "this member is not for external use".

@Mingun
Copy link

Mingun commented Jan 10, 2020

@GreyCat @dgelessus, I like you suggestions, but I think, it is better to name attribute hidden, to avoid unnecessary associations with access modifiers in languages that support them:

doc: Pascal string
seq:
  - id: len
    doc: Not part of API, just implementation detail
    type: u1
    hidden: true
  - id: content
    doc: Use `content().length()` to get string length
    size: len
    type: str
    encoding: ASCII

Hidden fields just doesn't part of API. In languages, like Java, getters can be omitted for their or their access modifiers can be changed from public to something else. The only drawback with modifiers is that such fields maybe can't be accessed from another ksy file, because, for example for Java, it can live in different packages.

Also, we can add configuration parameter for targets, that supports different modifiers. So, if discussed drawbacks not the case for particular user, it can generate classes with nice API.

@KOLANICH
Copy link

Basically I want to be able to say "this field/instance is only here because of the particular way I wrote the spec, it's not part of the file format and might change or go away when I update the spec".

#225 ? Also #88 may be related (allows the compiler to discard the temporary stuff freeing the memory and to provide a nice interface).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants