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

Unknown values for enums that are used in switch-on cases leads to NPE in Java #300

Closed
SeriyBg opened this issue Dec 27, 2017 · 14 comments
Closed
Milestone

Comments

@SeriyBg
Copy link

SeriyBg commented Dec 27, 2017

I have a field that is defined as an enum and the second field that in uses this enum in switch-on case:

- id: type
  type: u1
  enum: type_code

- id: body:
  type:
    swith-on: type
    case: 
      'type_code::type1': type1
      'type_code::type2': type2
      _: generic_type

enums:
  type_code:
    1: type1
    2: type2

For these case, I have to define all the possible values for enum, even if I care only about two values, otherwise I will get NPE in Java as the field type is null and this field is used in Java switch statement.
The expected behavior for me is to get generic_type if the type field will be of type3.

@GreyCat
Copy link
Member

GreyCat commented Dec 27, 2017

Good catch, thanks! We totally need a test for that.

@GreyCat
Copy link
Member

GreyCat commented Dec 27, 2017

Probably we really need to do something to unify behavior of enums in various languages. This example shows why it is important.

@SeriyBg
Copy link
Author

SeriyBg commented Dec 27, 2017

Yeah, that's a java specific stuff.
One more suggestion from me: maybe have some UKNOWN value in the enum and all the unknown values map to it. But really not sure about this one, as it looks more like a workaround.

@GreyCat
Copy link
Member

GreyCat commented Dec 27, 2017

Actually, the situation is even more dire with Python: trying to access an unknown enum value results in runtime exception there.

@KOLANICH
Copy link

So for python we need the compiler to use a custom enum class catching such exceptions.

@Aladarion
Copy link

I re-up this topic. Today's only solution is to handle the execption viaa try: execpt: solution on the .py generated file.

@brunchboy
Copy link

I re-up it as well: I have files where I need to parse the structures I know, but it is impossible for me to know in advance all possible fourcc values that I might encounter in the field (there is no documentation for the reverse-engineered file format), which leads to the parse crashing with a NullPointerException in Java even when the data I care about can be found later in the file. This is forcing me to give up on the readability offered by enums.

@GreyCat GreyCat added this to the v0.9 milestone Nov 21, 2018
brunchboy added a commit to Deep-Symmetry/crate-digger that referenced this issue Nov 21, 2018
brunchboy pushed a commit to brunchboy/kaitai_struct_formats that referenced this issue Nov 21, 2018
Trying to use an enum causes unavoidable parse errors in Java and
Python when new/unknown FourCC values are encountered. See
kaitai-io/kaitai_struct#300
@webbnh
Copy link

webbnh commented Nov 21, 2018

@Aladarion and @brunchboy: would it help if you added a default case (_) to your switch-on attribute? That's what we've done in our project. (Our default type returns the data as a single hex-encoded string.)

@brunchboy
Copy link

No, Java can’t handle null as input to a switch statement, which is what happens in this case, whether or not there is a default: case, so the generated code will crash. Evidently the crash happens in the enum itself in Python. The change needs to be made to the enum definition within the KSC specification and compiler so that there is a default match at that level.

@GreyCat
Copy link
Member

GreyCat commented Apr 15, 2019

Two tests created to demonstrate the problem:

Ruby spec for it passes, Java spec demonstrates NPE as of now. Next stop: delivering Java fix.

@GreyCat
Copy link
Member

GreyCat commented Apr 18, 2019

Crude, but working fix is done. Actually, this opened a big can of worms:

  1. Our "true switch" vs "switch emulated with if-then-else" was pretty clunky, I ended up doing huge refactoring to make things more straightforward.
  2. Java can potentially fail on any null value there, and we can't blindly do a if (foo != null) check because it won't compile too. Understanding Java nullability is relatively hard and is currently impossible to do solely relying on mechanisms that DataType provides. This fix provides special check for enum, but in future, technically, it's possible to hit the same problem with other nullable values (like Integer or String), which happened to be null due to if or type switching without the default.

@GreyCat
Copy link
Member

GreyCat commented Apr 18, 2019

I've created #568 as a reminder that the deeper problem still exists. Given that:

  • it's pretty artificial,
  • there's tons of more pressing issues,
  • it will require radical reworking of some of the ksc parts

... it's unlikely that it will be fixed soon.

@brunchboy
Copy link

Is this fix available in a released version of KSC?

@brunchboy
Copy link

I should have been more specific in my question, I meant released as a Maven artifact, and it appears the answer is “no,” the latest version out there is 0.8 released well before this change. Is there any hope of getting this released, and incorporated into the KSC maven plugin? It would make my mapping files for interacting with Pioneer DJ equipment so much nicer.

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

6 participants