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

Add repeat-until last element check #183

Conversation

generalmimon
Copy link
Member

@generalmimon generalmimon commented Nov 17, 2019

I need to access typeProvider property of LanguageCompiler from trait GenericChecks, so I made it public with val.

I reuse ConsistencyError for throwing if the last element of array doesn't satisfy repeat-until condition, but I'm not sure what should be present in the error message. Maybe it should throw a special type of Exception deriving from ConsistencyError, but I don't know.

I also add methods blockScopeHeader and blockScopeFooter, because in case there are multiple seq fields with repeat-until, I need to redeclare _it variable, every time with a different type. Java doesn't allow redeclaring variables in the same scope, so I wrap each check in its own.

For example, these are the generated repeat-until checks in the test format repeat_until_complex:

        {
            TypeU1 _it = first().get(first().size() - 1);
            if (!(_it.count() == 0))
                throw new ConsistencyError("first", "UserTypeInstream(List(type_u1),None,List())", "_it.count() == 0");
        }
        {
            TypeU2 _it = second().get(second().size() - 1);
            if (!(_it.count() == 0))
                throw new ConsistencyError("second", "UserTypeInstream(List(type_u2),None,List())", "_it.count() == 0");
        }
        {
            int _it = third().get(third().size() - 1);
            if (!(_it == 0))
                throw new ConsistencyError("third", "Int1Type(false)", "_it == 0");
        }

@generalmimon generalmimon added the serialization kaitai-io/kaitai_struct#27 label Feb 2, 2020
@generalmimon
Copy link
Member Author

generalmimon commented Feb 10, 2020

I just realized that it isn't enough to just initialize the _it variable to simulate the conditions while deserialization. A simple search in the formats repository shows how the repeat-until can be used too:

repeat-until: _io.eof or _.block_type == block_type::end_of_file
repeat-until: _io.pos == _io.size - 8
repeat-until: _io.pos + _.size > header.size.value or _.is_invalid
repeat-until: _.type == "IEND" or _io.eof

Accessing methods of the _io is a problem, because we suddenly can't just pull out one element from the array and do a simple check on it, because the _io would be in a completely different state. Or worse, it doesn't have to be initialized at all, if you create the KaitaiStruct from scratch (with parameter-less constructor), invoke some setters, then _check() method which is supposed to tell you if all the stuff will be read back properly (note that _io property is still null) and then call the _write(io), which sets the _io.

So we need to mock the _io object too. But how to get appropriate properties? It's quite unfortunate, because for determining _io.pos we need to know which fields precede, and for _io.size (plus _io.eof) which fields follow. That would make the current _check()'s method design wrong: it doesn't just check if its own state is consistent, the state of the other objects also matters.

This means that we can't just let _write() to immediately call _write() the underlying structs, because there is no place for calling _check() from the underlying code. Yeah, we could move the _check() call directly to the body of _write(), that would solve the problem of deriving pos. But for deriving _io.size, it would be probably anyway necessary to make a clone of the whole KaitaiStruct object (to not screw up everything) with some dummy KaitaiStream implementation (that only tracks the current position, but doesn't actually write anything) and simulate the writing till we'll be sure that the referenced _io is closed. And if it references _io that it's calculated (using some expressions like a == b ? foo._io : _parent._io), well, then "God help us".

When I think about the consequences of trying to manage checking such an repeat-until expression using _io, it probably isn't really worth it. Maybe the temporary workaround is to detect if the repeat-until contains any _io magic and then not generate the check.

Of course, we could give up repeat-until checks completely (that's what we're doing for repeat: eos, the same problems apply for it too because it does exactly the same as repeat-until: _io.eof), but I think most usages will deal only with the just-parsed element, so checking the last element of the array will be enough. And it's always a good idea to help the programmer to catch errors just before his program starts to produce invalid files, if it's possible.

@generalmimon generalmimon marked this pull request as draft September 19, 2020 19:31
@UnePierre
Copy link

UnePierre commented Mar 14, 2022

Why do you only check the last element? (For performance, of course.)
All predecessors must not fulfill the condition, unless there is some inconsistency there. That also needs checking, I assume?

I reuse ConsistencyError for throwing if the last element of array
doesn't satisfy repeat-until condition, but I'm not sure what should
be present in the error message. Maybe it should throw a special type
of Exception deriving from ConsistencyError, but I don't know.
@generalmimon generalmimon marked this pull request as ready for review September 12, 2022 14:05
@generalmimon
Copy link
Member Author

@UnePierre:

All predecessors must not fulfill the condition, unless there is some inconsistency there. That also needs checking, I assume?

Yes, thanks for pointing it out. I'll follow with that after I merge this PR.

@generalmimon generalmimon merged commit 426a38d into kaitai-io:serialization Sep 12, 2022
@generalmimon generalmimon deleted the serialization-repeat-until-check branch September 12, 2022 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
serialization kaitai-io/kaitai_struct#27
Projects
None yet
2 participants