778150 - Infallibility annotations are ugly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Peter was just adding infallibility annotations to Element and they ugly up the IDL a good bit.

We just talked about this on IRC, and we think it would look nicer to do something more like what WebKit's IDL does.  Specifically, annotate the things that are _fallible_, and do it like so:

  FooInterface myMethod() 
    throws;
  readonly attribute long myAttr
    getter throws;
  attribute long canAlwaysGet
    setter throws;
  attribute long maybeMaybeNot
    getter throws, setter throws;

or something.  Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and the above presumes we'd drop the separate worker/mainthread stuff in the interests of simplicity of syntax.

One drawback to this approach is that it relocates the ';' and hence makes diffing against spec IDL harder.  Maybe no one but me cares?  Maybe we should be checking in the spec IDL as-is alongside our own IDL and diffing against that when looking for spec changes?

Comment 2

[(no longer active)]

What are you exactly referring to by ugliness?  The frequency of the occurrences of [Infallible]?  What comment 0 suggest will then make the interfaces which have more fallible methods ugly then...  I personally don't have a problem with the existing syntax (except for the the CamelCase... ;-)

And I definitely think that we should at least partially optimize for the ease of diffing against newer versions of the spec.

Comment 3

[Boris Zbarsky [:bzbarsky]]

It's partly the frequency and partly the fact that [] is a big readability hit, as far as I can tell.

We could use trailing "infallible" instead of "throws" if desired, of course.

As far as diffing, I think I like the idea of checking in the spec IDL more and more...

Comment 4

[(no longer active)]

(In reply to comment #3)
> It's partly the frequency and partly the fact that [] is a big readability hit,
> as far as I can tell.
> 
> We could use trailing "infallible" instead of "throws" if desired, of course.
> 
> As far as diffing, I think I like the idea of checking in the spec IDL more and
> more...

Here's a crazy idea.  How about going after a slightly different model, like checking in the spec IDL unmodified, and then have an IDL like syntax right below it to denote custom attributes?  Something like:

interface Node : EventTarget {
  readonly attribute unsigned short nodeType;
  // copy of what's in the spec...
};

interfacedesc Node {
  nodeType: infallible,
  // ...
};

I don't like the custom IDL syntax a lot, but it should help with readability and diff-ability...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I prefer having everything in one place.
Also 'throws' sound a lot better than fallible.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I like the current syntax better than any of the proposed alternatives.

I think if we keep non-spec information inside [] annotations, allow multiple consecutive [] blocks, and additionally enforce the rule that a single [] block must not mix spec and non-spec annotations, then we could easily use regexps to strip out the non-spec annotations. After that you can diff -w against spec WebIDL.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> It's partly the frequency and partly the fact that [] is a big readability
> hit, as far as I can tell.

If you don't like [] that's too bad, because it's all over WebIDL :-). Personally I'm fine with it. Other languages (C#) use it for annotations too, even when there are lots of annotations. Syntax highlighting in your editor might help with readability.

If infallible is a lot more frequent than fallible, then sure, we should switch the default. In that case I would suggest [Throws], [GetterThrows] and [SetterThrows].

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If infallible is a lot more frequent than fallible

We don't have good data yet.  :(  It's certainly more frequence in something like WebGLContext, but not sure about other cases....

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Personally I would guess infallible is a lot more frequent than fallible.

Maybe you could scan Webkit's IDL files to get an estimate?

Comment 10

[Boris Zbarsky [:bzbarsky]]

In WebCore/dom/, we have the following numbers:

% grep attribute *.idl | wc -l
     441
% grep "getter raises" *.idl | wc -l
       7
% grep "setter raises" *.idl | wc -l
      13
% grep "(" *.idl | grep -v "#" | grep -v "*" | wc -l
     341
% grep raises *.idl | grep -v getter | grep -v setter | wc -l
      83

The grep -v things on "(" are to filter out their #ifdefs and license comments.

So it certainly looks like most of their stuff does NOT throw.  Not least because they're just generally more failure-is-fatal happy than us.  For example, their removeAttribute never throws, apparently, while ours can at the moment.  Of course nothing stops us from making ours infallible too (we'd need to swallow errors from Before/AfterSetAttr, and make nsAttrAndChildArray::RemoveAttrAt infallible by making GetModifiableMapped infallible which it already is, since new is infallible.

Peter, if we swapped the fallible/infallible annotations, how would your Element stuff look?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the vestigial ability to specify infallibility stuff in the conf file.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Default methods to infallible. Allow annotation of fallible methods in WebIDL using [Throws]. can either take no value, or can take MainThread or Workers if the

throwing behavior is only happening on main thread or in workers.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Default attributes to infallible. Allow annotation of fallible attributes in WebIDL. valid annotations are [Throws], [GetterThrows], and [SetterThrows], which can all either take no value or

be set to MainThread or Workers if the throwing behavior is only happening on main thread or in workers

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Methods, with one more infallible removed

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Attributes, with the remaining infallibility annotations removed.

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 657514 [details] [diff] [review]
part 1.  Remove the vestigial ability to specify infallibility stuff in the conf file.

Review of attachment 657514 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +2839,5 @@
>      A class to generate an actual call to a C++ object.  Assumes that the C++
>      object is stored in a variable whose name is given by the |object| argument.
> +
> +    errorReport should be a CGThing for an error report or None if no
> +      error reporting is needed.

Spacing looks weird for that second line.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 657518 [details] [diff] [review]
Methods, with one more infallible removed

Review of attachment 657518 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +677,5 @@
>  
>                  method = IDLMethod(self.location, identifier, retType, args)
> +                # Constructors are always Creators and are always
> +                # assumed to be able to throw (since there's no way to
> +                # indicate otherwise) and never have any other

"to indicate failure"?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> "to indicate failure"?

No, to indicate in the WebIDL that the constructor is infallible.  As in, we can't put an extended attr on a constructor...

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Spacing looks weird for that second line.

Fixed.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 657519 [details] [diff] [review]
Attributes, with the remaining infallibility annotations removed.

Review of attachment 657519 [details] [diff] [review]:
-----------------------------------------------------------------

You also need to do:

diff --git a/dom/bindings/GenerateCSS2PropertiesWebIDL.py b/dom/bindings/GenerateCSS2PropertiesWebIDL.py
--- a/dom/bindings/GenerateCSS2PropertiesWebIDL.py
+++ b/dom/bindings/GenerateCSS2PropertiesWebIDL.py
@@ -3,17 +3,17 @@
 # You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import sys
 import string
 
 propList = eval(sys.stdin.read())
 props = ""
 for [prop, pref] in propList:
-    extendedAttrs = ["TreatNullAs=EmptyString"]
+    extendedAttrs = ["Throws", "TreatNullAs=EmptyString"]
     if pref is not "":
         extendedAttrs.append("Pref=%s" % pref)
     if not prop.startswith("Moz"):
         prop = prop[0].lower() + prop[1:]
     # Unfortunately, even some of the getters here are fallible
     # (e.g. on nsComputedDOMStyle).
     props += "  [%s] attribute DOMString %s;\n" % (", ".join(extendedAttrs),
                                                    prop)

::: dom/bindings/Configuration.py
@@ +294,4 @@
>              assert(attr is None or attr is True or len(attr) == 1)
>              if (attr is not None and attr is not True and
>                  'Workers' not in attr and 'MainThread' not in attr):
> +                raise TypeError(("Unknown value for 'Throws': " % name) + attr[0])

Remove | % name|.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> You also need to do:

Yikes, indeed.  This was calling the XPCOM signatures, I guess.  That's an unfortunate failure mode...  :(

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed the CSS2Properties thing, removed the "% name" bit and pushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d625d938c36d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb9f5f29ead
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6afc8e389883

Comment 23

[Boris Zbarsky [:bzbarsky]]

Documented the new setup.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d625d938c36d
https://hg.mozilla.org/mozilla-central/rev/5cb9f5f29ead
https://hg.mozilla.org/mozilla-central/rev/6afc8e389883