Use Optional<> around optional arguments without default values

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
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?
(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<...>.
I prefer 2,3,4.
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".
Just passing through argc would certainly be easy!
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.
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.
> 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"?
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.
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".
Assignee: nobody → bzbarsky
Summary: Auto-generate an optional_argc argument for methods that have trailing optional args with no default values → Use Optional<> around optional arguments without default values
Whiteboard: [need review]
Created attachment 626288 [details] [diff] [review]
part 1.  Improve test coverage for various interface arguments.
Attachment #626288 - Flags: review?(peterv)
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
Attachment #626289 - Flags: review?(peterv)
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.
Attachment #626290 - Flags: review?(peterv)
Attachment #626288 - Flags: review?(peterv) → review+
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.
Attachment #626289 - Flags: review?(peterv) → review+
Attachment #626290 - Flags: review?(peterv) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/49e146df4c93
https://hg.mozilla.org/integration/mozilla-inbound/rev/0655a84b2739
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c031a7d86a1
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/49e146df4c93
https://hg.mozilla.org/mozilla-central/rev/0655a84b2739
https://hg.mozilla.org/mozilla-central/rev/6c031a7d86a1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.