Bug 160480 – Fix bindings generation for dictionaries with nullable members

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Bindings

[Simon Fraser (smfr)]

Fix bindings generation for dictionaries with optional members

[Simon Fraser (smfr)]

Created attachment 285167 [details]
Patch

[Chris Dumez]

This title sounds odd. I am pretty sure we were supporting dictionaries with optional members since most members are optional.

[Chris Dumez]

Comment on attachment 285167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285167&action=review

> Source/WebCore/ChangeLog:3
> +        Fix bindings generation for dictionaries with optional members

Do you mean "nullable" members?

All dictionary members are optional unless stated otherwise. We already support optional dictionary members.

> Source/WebCore/bindings/js/JSDOMConvert.h:54
> +template<typename T> typename Converter<T>::OptionalValue convertOptionalOrNullable(JSC::ExecState&, JSC::JSValue);

Why are these declared but have no implementation?

> Source/WebCore/bindings/scripts/test/JS/JSTestWithInitDictionary.cpp:46
> +    auto root = convertWrappedOptionalOrNullable<Element*>(state, object->get(&state, Identifier::fromString(&state, "root")), nullptr);

convertWrappedOptionalOrNullable is not declared anywhere.

[Chris Dumez]

Comment on attachment 285167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285167&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:927
> +    if ($value eq "null") {

Could be on one line: $value = "nullptr" if $value eq "null";

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1010
> +            my $conversionOptionsString = "";

Why do we need this, wouldn't this work?
my $conversionOptionsString = join('Or', @conversionOptions);

[Simon Fraser (smfr)]

Created attachment 285177 [details]
Patch

[Chris Dumez]

Comment on attachment 285167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285167&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestWithInitDictionary.cpp:43
> +    auto target = convertWrapped<Element*>(state, object->get(&state, Identifier::fromString(&state, "target")));

convertWrapped<> takes 2 template parameters, not one. Also, convertWrapped<>() currently can currently return null even though target is not nullable.
I am not sure why we need convert<>() functions with "nullable" in the name (hard to say given I don't see the implementation in this patch).

What we usually do is generate a null check in the bindings after converting, in cases where the type is not nullable. e.g.

auto* target = convertWrapped<JSElement, Element>(state, object->get(&state, Identifier::fromString(&state, "target")));
if (UNLIKELY(!target))
    // throw TypeError

[Simon Fraser (smfr)]

I'll talk to Sam, who wrote the patch.

[Chris Dumez]

Comment on attachment 285177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285177&action=review

> Source/WebCore/bindings/js/JSDOMConvert.h:118
> +    return value.isUndefinedOrNull() ? typename Converter<T>::OptionalValue() : convert<T>(state, value);

This does not seem right. Consider this case:
dictionary D {
    DOMString? value = "def";
}

If the JS passes null, you want a null value on the native side (in this case a null String()), not the default value ("def").

[Chris Dumez]

FYI, the spec is here:
http://heycam.github.io/webidl/#es-dictionary

It says to use the default member value only if the JS value is undefined.

It the value is null, then it would go through step 5.3 which would convert null to the native type and following the normal conversion rules (e.g. Throw a TypeError for non-nullable wrapper types, get passed as null for nullable types, get converted to 0 for number types, ...).

[Chris Dumez]

Comment on attachment 285177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285177&action=review

r- because this will not build and I don't believe this is spec-compliant as it seem to confuse nullable and optional.

> Source/WebCore/bindings/scripts/test/JS/JSTestWithInitDictionary.cpp:43
> +    auto target = convertWrapped<Element*>(state, object->get(&state, Identifier::fromString(&state, "target")));

This will not build.

> Source/WebCore/bindings/scripts/test/JS/JSTestWithInitDictionary.cpp:46
> +    auto root = convertWrappedOptionalOrNullable<Element*>(state, object->get(&state, Identifier::fromString(&state, "root")), nullptr);

convertWrappedOptionalOrNullable() does not exist.

[Chris Dumez]

Assuming we only need support for nullable wrapper types for now, I would suggest the following easy approach:
1. Keep the convertWrapped() that is in this patch
2. Fix the generated bindings so that it calls convertWrapped<>() with the right number of parameters
3. Drop everything else in this patch (Nullable handling in convert<> for e.g.)

convertWrapped() will pass null to the implementation if the JS passes null / undefined. So if the wrapper type is "Element", the implementation would need to use the type "Element".

Ideally, we would throw an exception if the wrapper type is not nullable and the JS passes null but we can improve this later on.

It could be done via any of the following ways:
1. Have the implementation throw a TypeError if it gets a null pointer. The is what we used to do for non-nullable operation parameters until recently.
2. Update the bindings generator to generate a null check after the call to convertWrapped<>() and throw a TypeError if null. We would then be able to pass a reference instead of a raw pointer to the implementation. This is what we do nowadays (since fairly recently) for non-nullable operation parameters.
3. Pass an extra "isNullable" parameter to convertWrapped() and have it throw a TypeError if isNullable is False to JSX::toWapped() returns null.

[Chris Dumez]

(In reply to comment #11)
> Assuming we only need support for nullable wrapper types for now, I would
> suggest the following easy approach:
> 1. Keep the convertWrapped() that is in this patch
> 2. Fix the generated bindings so that it calls convertWrapped<>() with the
> right number of parameters
> 3. Drop everything else in this patch (Nullable handling in convert<> for
> e.g.)
> 
> convertWrapped() will pass null to the implementation if the JS passes null
> / undefined. So if the wrapper type is "Element", the implementation would
> need to use the type "Element".
> 
> Ideally, we would throw an exception if the wrapper type is not nullable and
> the JS passes null but we can improve this later on.
> 
> It could be done via any of the following ways:
> 1. Have the implementation throw a TypeError if it gets a null pointer. The
> is what we used to do for non-nullable operation parameters until recently.
> 2. Update the bindings generator to generate a null check after the call to
> convertWrapped<>() and throw a TypeError if null. We would then be able to
> pass a reference instead of a raw pointer to the implementation. This is
> what we do nowadays (since fairly recently) for non-nullable operation
> parameters.
> 3. Pass an extra "isNullable" parameter to convertWrapped() and have it
> throw a TypeError if isNullable is False to JSX::toWapped() returns null.

I uploaded such a proposal at:
https://bugs.webkit.org/show_bug.cgi?id=160487

Simon, I don't know if it is sufficient for your use cases. Let me know if it is not.

[Simon Fraser (smfr)]

Better approach in bug 160487.