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

Parent class type detector doesn't check instances (C#/Java) #19

Closed
LogicAndTrick opened this issue Aug 11, 2016 · 4 comments
Closed

Parent class type detector doesn't check instances (C#/Java) #19

LogicAndTrick opened this issue Aug 11, 2016 · 4 comments
Assignees
Milestone

Comments

@LogicAndTrick
Copy link
Collaborator

This one's pretty obscure, but there's definitely a bug here.

The compiler seems to have some logic to check if a type is used in multiple locations to determine whether the parent is a known type or whether it should just a a generic KaitaiStruct type. (ClassCompiler.scala@markupParentTypes)

This check doesn't check the usages of the type inside instances. If only one type uses it in the seq section, but other types use it in the instances section, the resulting output won't compile.

I tried to fix this one by adding to markupParentTypes, but I couldn't work it out (I'm not familiar enough with the type system yet).


KSY to replicate the bug:

meta:
  id: multiple_use
  endian: le
seq:
  - id: t1
    type: type_1
  - id: t2
    type: type_2
types:
  multi:
    seq:
      - id: value
        type: s4
  type_1:
    seq:
      - id: first_use
        type: multi  # parent type = type_1
  type_2:
    seq: []
    instances:
      second_use:
        pos: 0
        type: multi  # parent type = type_2

Compiled result (similar problems in both C# and Java):

// Error CS1503 - Argument 2: cannot convert from 'MultipleUse.Type2' to 'MultipleUse.Type1'
class MultipleUse : KaitaiStruct
{
    ...

    // Note here that `parent` is `Type1`. It should be `KaitaiStruct`.
    class Multi : KaitaiStruct
    {
        public Multi(KaitaiStream io, Type1 parent = null, MultipleUse root = null) : base(io) { ... }
    }

    class Type1 : KaitaiStruct
    {
        ...

        private void _parse()
        {
            _firstUse = new Multi(m_io, this, m_root);
        }
    }

    class Type2 : KaitaiStruct
    {
        public Multi SecondUse
        {
            get
            {
                ...
                _secondUse = new Multi(m_io, this, m_root); // <--- compile error here
                ...
            }
        }
    }
}

Temporary work around:

# Add this to `seq`:
  - id: unused
    type: multi
    if: false
@GreyCat GreyCat self-assigned this Aug 11, 2016
@GreyCat
Copy link
Member

GreyCat commented Aug 11, 2016

Fantastic job, as always, @LogicAndTrick, thanks :)
I'll try to fix that one.

@GreyCat
Copy link
Member

GreyCat commented Aug 11, 2016

By the way, you're welcome to check in these test .ksys directly into tests :)

GreyCat added a commit that referenced this issue Aug 12, 2016
@GreyCat
Copy link
Member

GreyCat commented Aug 12, 2016

Seems to be fixed. I've added test spec for Java, probably we'll have to add them now for the rest of the languages...

@GreyCat GreyCat added this to the v0.5 milestone Aug 23, 2016
@GreyCat
Copy link
Member

GreyCat commented Aug 23, 2016

Tests for everything beside C++ (not working now, need more namespacing tweaks) and PHP (under development anyway) are ready and passing - see http://kaitai.io/ci/ - I guess I'm closing this one as solved.

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

2 participants