alavrik / piqi

Piqi – universal schema language: JSON, XML, Protocol Buffers data validation and conversion
http://piqi.org
Apache License 2.0
246 stars 36 forks source link

piqi loops forever on self-recursive type definitions #39

Closed yfyf closed 10 years ago

yfyf commented 10 years ago

This is certainly some super-corner-case unhappy-path behaviour, but if you have a spec like:

[~/sandbox]λ cat foo.piqi 
.alias [
    .name foo
    .type foo
]

then piqi convert foo.piqi will loop forever. Ideally it would throw up with an error.

yfyf commented 10 years ago

Also, no level of --debug seems to produce output either.

alavrik commented 10 years ago

Thanks for reminding about it. This is actually a TODO. It triggers a tight infinite loop that doesn't have any debug logging in it. We also need to check for infinite types with records and variants. I was planning to cover all of it at once one day.

yfyf commented 10 years ago

I realised that a slight complication is that you also need to check for cycles of any length if you want to fix this properly: i.e. a chain of aliases a -> b -> c -> a should also be impossible. Furthermore you can have nasty things like a record with a field which points back to the record itself, which is a valid construct if the field is optional. Such definitions are OK in protobuf, but not in Erlang though...

yfyf commented 10 years ago

Although in Erlang you could workaround by desugaring the record to it's tuple representation and then manually specifying the field to be of this form. Or just not specifying the field type at all. Though you will have to give up Dialyzer for these for sure.

alavrik commented 10 years ago

Implemented in https://github.com/alavrik/piqi/commit/b7fbb9179854c4742094bb1da49a14d13fe3fbca

@yfyf this is exactly what I meant by infinite types. Obviously this includes looped alias chains as well. All of this should be covered now.

Also, record fields pointing back to the record has always been supported in piqi-erlang. This is actually very simple. piqic-erlang generates -type r() :: #r{} for each record -record(r, ...) and whenever record the record is referenced, type r() is used instead of #r{}.

yfyf commented 10 years ago

Wow, that was quick!

Confirmed to work:

[~/sandbox]λ piqi convert foo.piqi 
foo.piqi:1:8: alias "foo" forms a loop

Thanks!

P.S. Neat hack with the type aliasing for records, did not realise that could work.

yfyf commented 10 years ago

The loop-checker seems to be too restrictive, this is IMHO a fine definition:

$ cat sample.piqi 
.variant [
    .name where
    .option [ .name rec .type where-list ]
    .option [ .type uint ]
]
.list [ .name where-list .type where ]

$ piqi convert sample.piqi 
sample.piqi:6:7: list "where-list" forms a loop
alavrik commented 10 years ago

Fixed. Thanks!