GreedyRange's exception swallowing behaviour is problematic (for some usecases)

[phire]

I know GreedyRange is designed to abort when it encounters an exception.
But for some usecases, this behavour is problematic.

For example, I'm parsing a file format that has multiple concatinated "substreams", each filled with an unknown number of variable length records. This seems like a perfect usecase for FixedSize and GreedyRange

Simplified example:

header = Struct(
  "SectionASize" / Int32ul,
  "SectionBSize" / Int32ul,
  "SectionCSize" / Int32ul,
)

file = Struct(
  "Header" / header,
  "SectionA" / FixedSize(this.Header.SectionASize, GreedyRange(ARecord)), # Note: Records are variable length
  "SectionB" / FixedSize(this.Header.SectionBSize, GreedyRange(BRecord)),
  "SectionC" / FixedSize(this.Header.SectionCSize, GreedyRange(CRecord)),
)

And this works perfectly fine. But only if the file is valid and the record types have been implemented correctly.
When the file is invalid, or I'm still the the process of reverse engineering the file format types and one of the records throws and exception, GreedyRange will just silently truncate all remaining items. That's a huge pain, I might not even notice there was an error until way later, and it can be hard to debug.

I tried to find an alternative solution. The best I found was a hacking way of using RepeatUntil

"SectionA" / FixedSize(this.Header.SectionASize, RepeatUntil(
    lambda x, lst, ctx: x._io.tell() == ctx.Header.SectionASize, 
    ARecord
 ),

The fact that I had to read the source code and access a non-public variable is highly indicative of a deficiency in construct's API. Maybe there is a better way? But I couldn't find it.

Users might also encounter this when using GreedyRange with OffsettedEnd or otherwise just expecting GreedyRange until the end of a file to actually consume the whole file.

Other usecases?

The other time I've run into this is when parsing a variable length list with termination conditions. The documentation suggests that StopIf can be used to signal a termination condition. But because GreedyRange always terminates on any exceptions it silently swallows any exceptions before your StopIf condition.

For this usecase, it's possible to use an alternative implementation with RepeatUntil and expressing the termination condition in the lambda. But that moves where the termination condition is expressed (and sometimes you want it to be with the subcon), and kind of means StopIf is useless for GreedyRange.

Possible solutions

I've brainstormed a few potential solutions to this problem, but I'm not sure the best direction. Maybe someone has a better idea?

New Repeater

Add a new repeater that is just GreedyRange without exception swallowing.

Perhaps named something like RepeatUntilEnd or FillRange.

Add an alternative mode to GreedyRange

An optional parameter could be added to GreedyRange that controls the swallowing behavour, allowing the user to do custom exception filters.

• When not supplied, GreedyRange keeps the current behaviour
• When something Falsifiable (ie None, [], False) is supplied, all exceptions (except StopFieldError) are propagated up
• When a list of exceptions is supplied, all other are propagated
• When a lambda is supplied, it's called with the exception allowing for custom filtering code (or other custom behaviour on exceptions)

Change the GreedyRange API

Maybe GreedyRange shouldn't be doing this swallowing behaviour by default?
It's somewhat counterintuitive, and maybe users should be explictly opting into this stopping on exceptions (a StopIfException wrapper?)

But this would be a breaking API change, which I guess that would be a hard sell.

A ConsumeAll wrapper

Perhaps the user should be explictly stating that they expect a subcon to fill all remaining space in the stream. For example:

"SectionA" / FixedSize(this.Header.SectionASize, ConsumeAll(
    # Expect entire FixedSized substream to be filled with ARecords
    GreedyRange(ARecord)
)),

This has wider applications beyond just GreedyRange as ConsumeAll would always throw an error when it's subcon doesn't completely fill the remaining space in the stream.

The major advantage here is you can do things like:

"SectionA" / FixedSize(this.Header.SectionASize, ConsumeAll(
    # Same as above, but allow the substream to be padded with nulls to a multiple of 16 bytes
    Aligned(16, GreedyRange(ARecord))
)),

It would be pretty hard to express this with any of the previous solutions.

However, it doesn't entirely fix the issue of GreedyRange swallowing exceptions. While we get an error from ConsumeAll when the stream had unused bytes, we won't know why GreedyRange aborted early.

So GreedyRange would need to modify it's behaviour when nested inside a ConsumeAll context to stop swallowing exceptions (or store them until later for ConsumeAll to retrieve them). Though the design gets complex, what if there are multiple Greedys inside a ConsumeAll? Or a complex structure with nesting and other variable length fields?

I'm not sure I like the idea of GreedyRange's behaviour changing based on it's wrapper, maybe this solution can be combined with one of the above solutions?

[arekbulski]

For a start, thank you for your elaborate report. I found it an interesting read. Please forgive me for not replying earlier. I used to reply within 5 minutes but now I have other responsibilities so, yeah. I totally agree with you that a problem exists. I also agree with most of what you wrote on how to approach it.

Changing GreedyRange semantics is out of the question. Extending its API with another keyword argument is unlikely to happen, but if we must, we can do it. Adding the ConsumeAll class is a very good idea. I would like to incorporate it into the core library.

To fit with the existing naming convention I would like the class to be named GreedyConsumeAll, or so. ConsumeAll is a good name for it, I would say. We can use it.

Please propose a PR? This includes new test cases, GreedyRange test cases, and all relevant documentation pages.

[phire]

Adding the ConsumeAll class is a very good idea.

With or without GreedyRange changing semantics when inside a ConsumeAll?

I do have several concerns about ConsumeAll changing semantics:

• It might not be obvious to users.
• Interacting with complex nested structures with multiple GreedyRanges could really complicate things
  (But if we go for the "storing exceptions until later" route, it should be fine, just report all greedies that failed early)
• It also doesn't fix other use cases where GreedyRange's size is unbounded, but the user still wants exceptions.

But I agree ConsumeAll is a good idea even if it doesn't change semantics. I'll look into making a PR. Though I was re-reading the documentation and discovered that Terminate is a thing, a basic implementation would just be FocusedSeq(0, GreedyRange(x), Terminate)

What are your thoughts about my suggestion creating a new Repeater for this usecase? (I suggested RepeatUntilEnd as a name, you might have a better idea). Of all the remaining solutions, it's probably the most sane, and a better idea than adding a keyword arg to GreedyRange.

[arekbulski]

Oh sorry for not making this clear: I would like ConsumeAll to be an alternative to GreedyRange. Basically it would do same thing except [1] not catch exceptions and [2] fail if not finished parsing at EOF.

[phire]

Ah, that makes more sense.
I'm not sure about the name, I was suggesting ConsumeAll for something else.

I want the alternative to cover two usecases

1. Repeating until the stream encounters an EOF
2. Repeating until the subcon emits a StopIf exception (for example, parsing structs until one has a final flag set)

ConsumeAll is an ok name for the first usecase, as it is consuming the entire stream, but seems wrong for the second usecase. It's no longer consuming the entire stream (though it is consuming an entire range of subcons defined by some rules)

I think it's wise to avoid the "Greedy" naming convention, as the semantics are clearly different from the other Greedys. I was thinking about trying to align it with RepeatUntil's naming convention with RepeatSomething (RepeatForever, RepeatUntilEnd, or even just Repeat). It can also be considered to be RepeatUntil without the Until lambda.

[arekbulski]

Is there a PR coming? Not that there is a rush.