Hi,
Unions have been added late, and they are basically implemented as an
extension to struct. The idea was to be wire-compatible with
non-struct-implementing languages. It would be a good thing if we keep it
that way.
> It seems to me that this needs fixing.
> But it's a possibly- breaking change.
Ok, but ... why breaking? If unions are used as they are supposed to, this
should be a nonbreaker. If they arent, it is wrong anyway.
> I can starting fixing it in C++, Python, Ocaml, and a few
> other languages. Should I do so?
If you're not sure, you can discuss it. If you are sure, provide a PR
(ideally one per language) and we look at it.
From my feelings, I'm not really sure if I fully understand the problem and
how you plan to fix it. Maybe it is a good idea to sketch the idea behind
and get some comments before you deep-dive in the code.
Have fun,
JensG
-----Ursprüngliche Nachricht-----
From: Chet Murthy
Sent: Friday, December 1, 2017 9:37 AM
To: dev@thrift.apache.org
Subject: Thrift unions don't work as documented
I dug into this question b/c I noticed that in plugin.thrift, the
t_const_value seemed to not be behaving as a union should. To wit, it
seems that in the case of a type like
struct Foo {
1: required Enum E x = E.A
}
(for some enum E), the t_const_value that represents the initializer, ends
up having both a "identifier_val" and an "enum_val". And yet, in
plugin.thrift, it's a UNION.
So looking further, it seems that the (at least C++) code of a union, e.g.
union Bar {
1: i32 a ;
2: string b;
}
DOES NOT enforce the rule (from https://thrift.apache.org/docs/idl) that
Unions are similar to structs, except that they provide a means to
transport exactly one field of a possible set of fields, just like union {}
in C++. Consequently, union members are implicitly considered optional (see
requiredness).
In no way, is there any enforcement of "exactly one field".
It seems to me that this needs fixing. But it's a possibly- breaking
change.
I can starting fixing it in C++, Python, Ocaml, and a few other languages.
Should I do so?
--chet--
