zeek / spicy

C++ parser generator for dissecting protocols & files.
https://docs.zeek.org/projects/spicy
Other
249 stars 37 forks source link

Problems with non-unit types. #1845

Closed sethhall closed 2 months ago

sethhall commented 2 months ago

I seem to be having some trouble with defining any types that aren't units.

module Test;
public type X = bytes &size=3;
❯ printf '123' | spicy-dump -p Test::X test4.spicy
[error] spicy-dump: no parsers available

Problems still persists if my public type is a unit but I'm using one of these non-unit types within it.

module Test;

public type Y = unit {
    x: X;
};
public type X = bytes &size=3;
❯ printf '123' | spicy-dump -p Test::Y test4.spicy
[error] test4.spicy:4:5-4:9: bytes field requires one of &eod, &parse_at, &parse_from, &size, &until, &until-including
[error] spicy-dump: aborting after errors

If I provide a length where the X type is getting used then it works, but that doesn't seem like desired behavior.

module Test;

public type Y = unit {
    x: X &size=3;
};
public type X = bytes &size=3;
❯ printf '123' | spicy-dump -p Test::Y test4.spicy
Test::Y {
  x: 123
}
rsmmr commented 2 months ago

I think there're a couple of misunderstandings going on here:

spicy-dump: no parsers available

Only units can be top-level types for parsing, so the error message is expected here. The public isn't saying "use it for parsing", but "this type is visible externally". For units, "visible externally" implies "we can use it for parsing"; but for other types it doesn't.

public type X = bytes &size=3;

Parsing attributes are associated with fields, not types. If you put them into a type definition, they'll simply be ignored currently (which is a diagnostics bug: we should really have a validator giving you an error message rejecting such attributes on types). This is similar to #1812 in that regard.

sethhall commented 2 months ago

Sorry, I tried to condense my observation so much that I may have gone too far. 😄

The structure I'm really wondering about is if I can do this...

module Test;
public type X = vector<uint16>;

I initially tried that as uint16[] (gives a syntax error) but the spicy parser didn't like that. Using the vector syntax lets it compile but then it complains about no parsers being available which I think gets back to the points you made in your comment.

Is it reasonable for me to want to write parsers with non-unit data structures? It feels to me that there are enough data structures out in the world that are effectively vectors with no outer structure that it would be supported. If it's a huge architectural problem at the moment though I'd rather just close this ticket and table it for the future. 😄

Also, I guess I have one other question, should I be dropping example scripts somewhere whenever I run across little syntax things that I think should kick out an error or kick out an error that is incorrect or misleading? Perhaps a running ticket with a checklist or something?

rsmmr commented 2 months ago

Is it reasonable for me to want to write parsers with non-unit data structures?

Short answer: not going to happen. The unit is the basic entry point for parsing, and all the machinery has been structured around that. It also doesn't seem to be a limitation to put whatever type you want to parse into a top-level unit, so I think that's fine as is.

I be dropping example scripts somewhere whenever I run across little syntax things that I think should kick out an error or kick out an error that is incorrect or misleading?

Just create individual tickets, it's the easiest to work through. You can mark them as "diagnostics" and "low priority" if you want (otherwise we'll probably do that :-))

sethhall commented 2 months ago

Is it reasonable for me to want to write parsers with non-unit data structures?

Short answer: not going to happen.

sounds good

Just create individual tickets, it's the easiest to work through. You can mark them as "diagnostics" and "low priority" if you want (otherwise we'll probably do that :-))

Will do! Thanks.

sethhall commented 2 months ago

Whoops.

sethhall commented 2 months ago

Dammit.