Last Comment Bug 743906 - Use Optional<> around optional arguments without default values
: Use Optional<> around optional arguments without default values
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-04-09 20:35 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-05-30 07:35 PDT (History)
4 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Improve test coverage for various interface arguments. (8.02 KB, patch)
2012-05-22 19:24 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
part 2. Use Optional<> for optional arguments that don't have default values. (60.38 KB, patch)
2012-05-22 19:29 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
part 3. Stop listing default values for optional arguments with no defaults, now that we're guaranteed to only examine such values if argc is big enough. (1.84 KB, patch)
2012-05-22 19:31 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-04-09 20:35:41 PDT
Any such method is going to have prose describing what should actually happen when the argument is not specified, and we won't be able to handle that in the bindings.  Anything we _can_ handle in the bindings can be expressed as a default value and we should push back on specs about that as needed.

Note that we may need [TreatUndefinedAs=Missing] support before we can really make use of this effectively for XHR.

The benefit is that we won't need the "needs argc" annotation business; methods which should have one will automatically get one.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 08:36:12 PDT
Heycam is changing webidl to make it saner.  After that change, function arguments will have to come in the following order:

1)  Required arguments
2)  Optional arguments with default values specified
3)  Optional arguments without default values
4)  Variadics

In that setup, I think the right meaning for optional_argc is the number of arguments in buckets 3 and 4 that was passed.  The only drawback is that if a default value gets added to another argument, the compares for optional_argc for that function need to change.  If we want to avoid that problem, we should just have optional_argc mean everything from buckets 2,3,4.  Thoughts?
Comment 2 Peter Van der Beken [:peterv] 2012-05-06 05:07:50 PDT
(In reply to Boris Zbarsky (:bz) from comment #1)
> 3)  Optional arguments without default values

The other option would be to use a Optional<...> for these (with a NotPassedIn() type of method), similar to Nullable<...>.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-06 16:34:08 PDT
I prefer 2,3,4.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-05-06 18:40:20 PDT
Either 3,4 or 2,3,4 sounds good to me. I suspect it'll be somewhat rare for an argument to gain a default value, so going through the list of places that use optional_argc in those rare instances doesn't seem like a big problem.

Actually, it might even be simpler to make it be 1,2,3,4. I know I always think in terms of "how many arguments were passed" as opposed to "how many of the optional arguments were passed".
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-05-06 18:47:10 PDT
Just passing through argc would certainly be easy!
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-05-06 18:51:03 PDT
So the Optional<> thing is an interesting question.

If we think we're going to need to support optional arguments that are not settable to a sane default (e.g. optional non-nullable interface types that we actually pass through as a reference), then we may need some sort of Optional<>.  But I'm not sure it makes sense to do it for just optional arguments in general, because it makes it look like some argument can be set when preceding ones are not set, which is not the case in WebIDL.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-05-06 20:01:33 PDT
One thing to keep in mind is that the spec will quite possibly change such that optional arguments that have a that are passed 'undefined' should get the default value rather than 'undefined'.

WebIDL is basically deferring to TC39 on the issue (ES6 is adding syntax for default values) and in the last email to the TC39 list there seemed to be more people in favor of treating undefined as "argument wasn't passed" rather than "argument was passed the value undefined".

Specs still say that undefined is treated just as any other value though, so it's still anybody's guess what the final result will be.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-05-06 20:25:21 PDT
> One thing to keep in mind is that the spec will quite possibly change such that optional
> arguments that have a that are passed 'undefined' should get the default value rather
> than 'undefined'.

I assume you mean "have a default value"?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-05-06 22:02:55 PDT
No, what I mean is that for something like:

interface XMLHttpRequest {
  ...
  void open(DOMString method, DOMString url, optional boolean async = true,
            optional DOMString? user, optional DOMString? password);
  ...
};



xhr.open("GET", someurl, undefined);

should use <true> as value for the async argument. I.e. when an optional argument is passed <undefined>, we should treat it as if the argument wasn't passed at all.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-05-06 22:13:46 PDT
Yes, that's what I assumed you meant.

That's somewhat orthogonal to this whole discussion, except insofar as it might make the "argc" passed to the C++ not actually equal to the JSAPI's "argc".
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-05-22 19:24:30 PDT
Created attachment 626288 [details] [diff] [review]
part 1.  Improve test coverage for various interface arguments.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-05-22 19:29:52 PDT
Created attachment 626289 [details] [diff] [review]
part 2.  Use Optional<> for optional arguments that don't have default values.

The one thing here that I'm not sure about is the complexity I needed to avoid Optional<NonNull<TypedArray>>, instead this code generates Optional<TypedArray>.  If I'd gone with the other, the typed array changes would have been about as trivial as most of the rest of them...  Peter, what do you think?  It's a bit weird because it would create an Optional<Maybe<TypedArray>> holder, which is a bit wasteful.  But that'd all be internal to the binding code and only matter for optional typed arrays, which I expect to be a rare case anyway.  Let me know, please
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-05-22 19:31:33 PDT
Created attachment 626290 [details] [diff] [review]
part 3.  Stop listing default values for optional arguments with no defaults, now that we're guaranteed to only examine such values if argc is big enough.
Comment 14 Peter Van der Beken [:peterv] 2012-05-29 07:55:48 PDT
Comment on attachment 626289 [details] [diff] [review]
part 2.  Use Optional<> for optional arguments that don't have default values.

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

It's a bit weird that the we use |const Optional<...>| and then cast it away. Is this just because you want to pass |const Optional<...>| to the API?
The typedarray stuff is pretty complicated, but removing that NonNull looks worthwhile. Would it be easier to use |const Optional<Typedarray&>|?

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1877,5 @@
>      // Disallowed by content policy
>      return NS_ERROR_CONTENT_BLOCKED;
>    }
>  
> +  // XXXbz this is wrong: we should only be looking at whether

Is there a bug on this?

::: dom/bindings/BindingUtils.h
@@ +760,5 @@
> +  bool WasPassed() const {
> +    return !mImpl.empty();
> +  }
> +
> +  void construct() {

Capital C?

@@ +801,5 @@
> +    mStr = str;
> +    mPassed = true;
> +  }
> +
> +  const nsAString& Value() const {

It's a bit odd that Optional<T> has a |T& Value()| but this doesn't.

::: dom/bindings/Codegen.py
@@ +1582,4 @@
>          holderType = "Maybe<%s>" % name
> +        constructLoc = "${holderName}"
> +        if isOptional:
> +            if type.nullable():

Nit: maybe restructure this code to be like the one below to be

if nullable():
    if isOptional:
    else:
else:

so it's easier to match them up. Up to you.

::: dom/bindings/test/TestBindingHeader.h
@@ +63,5 @@
>    int8_t ReceiveByte(ErrorResult&);
> +  void PassOptionalByte(const Optional<int8_t>&, ErrorResult&);
> +  void PassOptionalByteWithDefault(int8_t, ErrorResult&);
> +  void PassNullableByte(Nullable<int8_t>&, ErrorResult&);
> +  void PassOptionalNullableByte(const Optional< Nullable<int8_t> >&,

Nit, drop the whitespace before Nullable.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-05-29 20:42:02 PDT
> Is this just because you want to pass |const Optional<...>| to the API?

Yes.

> Would it be easier to use |const Optional<Typedarray&>|?

You mean create a specialization of Optional for T& in general and have it store a pointer or something, the way NonNull works?  That's the only way I can see it working...  And it would still be pretty complicated, I suspect.  Possibly just as complicated.

> Is there a bug on this?

Filed bug 759624.

> Capital C?

OK, done.

> It's a bit odd that Optional<T> has a |T& Value()| but this doesn't.

There was no use case for it, so I didn't add one...  The Optional<T> needs a non-const Value() so a value can be assigned to it, but the Optional<nsAString> just uses an assignment operator.

> Nit: maybe restructure this code to be like the one below to be

Done.

> Nit, drop the whitespace before Nullable.

Done.

Note You need to log in before you can comment on or make changes to this bug.