[PATCH wayland] Add "enum" attribute to "arg" elements

[Pekka Paalanen]

On Tue, 21 Apr 2015 16:32:40 +0000
Jeroen Bollen <jbinero at gmail.com> wrote:

> I think I have thought out a solution.
> 
> The scanner should be able to parse multiple specification files. There
> would be the default wayland.xml, and for example a gnome.xml, which is an
> extension to the wayland.xml. It does only contain things like added
> interfaces, added requests, added events and added enum variants. The
> scanner would merge all these files together.

You cannot add events or requests to existing interfaces without
requiring the world to always use exactly the same aggregate set of xml
files. It is much simpler to just extend existing interfaces properly,
in the xml file they are defined in in the first place. That way you
can also do the proper versioning, and people are not surprised by
that using only wayland.xml breaks things. This is not a tractable
approach. The fundamental reason for this is how we assign message
opcodes.

Adding interfaces is simple, you don't need to parse several XML files
in one go to do that. Unless your specific language and generator
simply cannot work otherwise, but that is your problem. Mind, that
people probably want to use specific protocol extensions in modules
(whatever those are in your language), so would be really nice to not
deny them from that. If your generator requires also all the dependant
XML files to be present when parsing a new XML file, I think that would
be fine. But I do not think making it a generic mechanism to extend
existing interfaces is an idea that could work.

Adding values to existing <enum> sets in additional XML files is less
prone to break, but still not a good idea. You will be splitting the
value allocations to several places, which requires careful
coordination to avoid clashes. It doesn't feel like a good idea to me
to support this generally for everything.

There are two ways to add values to an existing <enum>:

- Bumping the interface version, and ensuring the new values are used
  only with the appropriate runtime interface version.

- From the first definition of an interface, specify how unknown values
  should be handled. Otherwise users do not expect unknown values to
  appear.

The third case is error enums. IMO, error codes can be added without
any special care, because a protocol error event which carries these is
always fatal anyway. They always terminate the Wayland connection, so
there is nothing a client could do afterwards apart from just reporting
the error.

> This means that enums can be complete. If a user wants to have additional
> values, they should simply add an XML for the extended feature set. This
> would also reduce and fragmentation, as extensions to the protocol don't
> require one to maintain an entire wayland.xml file anymore. It also allows
> people to use multiple protocol extensions at once, without merging
> multiple, already-forked specifications.
> 
> What are the opinions on this?

I didn't think anyone was maintaining modified wayland.xml files. It
would certainly be very fragile and break interoperability between
projects.

Feature development is one thing, but releasing a project with a
modified wayland.xml seems as good as providing your own
modified versions of libc headers. I see it as asking for trouble and
quite likely succeeding.

I also think this discussion is going off-topic. You wanted to add
annotations to the XML, so you could find out about enum and bitfield
arguments, so let's keep to that. There is value in simplicity.


How about this:

Add three new, mutually exclusive attributes for <arg> tags:

	docenum="enumname"

Docenum would be for documentation linking only, and should not affect
code generation. The effect would be in the documentation to add a link
to the definition of the "enumname" <enum>. This attribute is
applicable for both int and uint type <arg>s.

	enumeration="enumname"

Enumeration would be a doc link, but also specify that the <enum>
"enumname" is a complete enumeration: no other values are legal. You
can use this for code generation. You will rely on interface version
negotiation to avoid unknown values in case you have an old definition
of the interface and your opponent is using a new definition which
added values. This attribute is applicable for both int and uint type
<arg>s.

	bitfield="enumname"

Bitfield would be a doc link, but also specify that the values listed
in <enum> "enumanme" can be orred together to form new legal values.
Bits that are not settable by using the listed values must be zero. You
rely on the interface versioning to avoid getting undefined bits set,
just like enumeration relies for adding new values. This attribute is
applicable only to uint type <arg>s.


So, both enumeration and bitfield could be used for codegen, docenum
would not. Docenum or nothing would be used for cases that do not fit
as enumeration or bitfield, including cases where unknown values are
always allowed but the interface specification defines what to do when
encountering one (ignore, use as a literal value, ...).

Wayland-scanner would generate the doclink references in header
comments, and check that the referenced enum exists and the <arg> type.

However, a remaining problem is that an interface cannot reference an
<enum> defined for a different interface. That is a special case I
don't know if we should support in this scheme, because it would
require the depended-on XML file to be used during parsing, and we do
not require that for wayland-scanner. So I'd rather leave that special
case for pure documentation and no attribute, at least for now.
Developing a way to reference external interfaces is for another time.

All these additions are backwards compatible. Scanners should ignore
tags and attributes they do not recognize. Existing XML files will
continue to work exactly as they were. We could even implement only the
docenum for now, and think about the rest more.

Note, that the meaning of the <enum> tag remains exactly the same: a
namespaced collection of named constants. Nothing more strict.


All that said, this is just a quick draft I haven't really thought
through. You could name the attributes differently, change docenum to
be an attribute for <description> rather than <arg>, etc.


Thanks,
pq


> 
> On Tue, 21 Apr 2015 at 09:18 Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
> > On Mon, 20 Apr 2015 15:54:59 +0000
> > Jeroen Bollen <jbinero at gmail.com> wrote:
> >
> > > On Mon, 20 Apr 2015 at 09:03 Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > > >
> > > > Also, adding the strict type information to the XML spec has no benefit
> > > > for C, which is the de facto language for Wayland core developers. A C
> > > > compiler also does not raise errors if you violate the rules. This and
> > > > all the above are the likely reasons why adding the strict type
> > > > information is not interesting, at least for me.
> > > >
> > > > Making the enum rules more strict has a possibility to break existing
> > > > users, but to me it is unclear if the benefit would outweigh that
> > > > con or the freedom.
> > > >
> > > > On the wire, an enum or bitfield is still just an uint (or int), and a
> > > > buggy client or server may cause the other to receive illegal values.
> > > > Do the strongly typed languages have checks against that? Can you
> > > > define what happens if the value is illegal for an enum? Or do you have
> > > > to write that check manually in any case?
> > > >
> > > > So, the big question here is: do we even want to have strict enum
> > types?
> > > >
> > >
> > > It does indeed not provide any benefit for C, but if C is the only
> > language
> > > to be targeted, there is not much use in there being an XML specification
> > > at all. It could've just been written out in C. I do agree that any
> > changes
> > > should be compatible with C.
> > >
> > > I know in Rust, conversion from the `uint` to the `enum` type would have
> > to
> > > be done manually, and error checking happens here. It does stop the user
> > > from passing illegal values to other servers however. It also makes it
> > more
> > > clear what `enum`s are to be used with which arguments, and enforces
> > this.
> > >
> > > I don't really see why adding that to the specification is an issue. If
> > > some `enum` types aren't complete, as in, they aren't the only valid
> > > values, then I see little value in having the `enum` in the specification
> > > in the first place.
> >
> > The foremost purpose of the <enum> is to let the generator create named
> > constants in a namespace. This is something that has to stay due to
> > API stability guarantees.
> >
> > I had some discussion in IRC, and I'm coming around. It does seem a
> > useful addition, but you have to be clear in specifying what it all
> > means. For instance, saying an attribute referencing an <enum> in an
> > <arg> for purely documentation linkage would be fine. We don't have
> > docs for the XML language, but at least define it in a commit message.
> >
> > If such annotation is intended to be useful for code generation on
> > relevant languages, you need to define clearly what it means. I'm not
> > familiar with those languages you are interested in, so I cannot judge
> > that.
> >
> > What I would want, is patches to wayland-scanner to use these
> > additional annotations, if only for syntax checking the XML and maybe
> > generating one more comment for an <arg> or whatever. Unfortunately
> > wayland-scanner still is the definition of the Wayland XML language.
> > It should be kept updated in any case.
> >
> > Two things I came up with in the IRC discussion was that only <arg>
> > types of int an uint are eligible for enums, and only uint for
> > bitfields. I think wayland-scanner should enforce that.
> >
> > One question was about whether bitfield values should be limited to
> > single bits (powers of two). I think that would be an artificial
> > restriction, and it would not allow using short-hand names for sets of
> > bits. I see no reason to deny those.
> >
> > Another is the question about incomplete enumerations. I don't think we
> > can deny their existence. Therefore they must be supported. If that
> > means you cannot put the enum attribute in such an <arg>, then so be
> > it; but you lose also the doc linkage. Extending value or bit
> > enumerations is something that happens, and with the expectation that
> > it doesn't break anything. An interface that expects to see new entries
> > in an enumeration in the future will specify how unknown values should
> > be dealt with. You maybe have your code generated according to an old
> > specification, while the other party is using a new specification, and
> > you need to obey the interface specification regarding unknown values.
> > This has to work.
> >
> > Btw. the wayland.dtd is abandoned. The rationale was that if it catches
> > errors that wayland-scanner doesn't, then wayland-scanner should be
> > fixed. We should probably remove that file, and preferably replace it
> > with documentation that agrees with wayland-scanner. If only had the
> > time...
> >
> >
> > Thanks,
> > pq
> >