949501 - Fix support for nullable unions in sequences and dictionaries

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This got missed in bug 767926.

Sequences and dictionaries assert that their member types have no holders.  Which is fine, but nullable unions were in fact producing a (unusable) holder.

Patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make nullable unions that are isMember stop claiming to have a holder.

Comment 2

[David Zbarsky (:dzbarsky)]

Comment on attachment 8346609 [details] [diff] [review]
Make nullable unions that are isMember stop claiming to have a holder.

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

::: dom/bindings/Codegen.py
@@ +3320,5 @@
>                  holderArgs = "${declName}.SetValue()"
> +            if holderType is not None:
> +                holderType = CGTemplatedType("Maybe", holderType)
> +                constructHolder = CGGeneric("${holderName}.construct(%s);" % holderArgs)
> +            else:

I think this is getting a little complicated.  holderType is not None only when not isMember, so maybe this could be

if isMember:
  holderType = CGTempaltedType("Maybe", CGGeneric(argumentTypeName)) and set it to CGGeneric(argumentTypeName) in the else and to None otherwise?  

It's probably a little longer but maybe easier to reason about.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow.  We have three cases here:

1)  not isMember, nullable: holderType should be a "Maybe"
2)  not isMember, non-nullable: holderType should be argumentTypeName
3)  isMember: holderType should be None

which is fine, but case 1 has additional complexity, because the constructHolder depends on isOptional.

So I could basically structure this like so:

  if nullable:
    # do the optional bits
    if isMember:
      holderType = None
    else:
      holderType = CGTemplatedType("Maybe", CGGeneric(argumentTypeName))
  else:
    if isMember:
      holderType = None
    else:
      holderType = CGGeneric(argumentTypeName)

Is that what you're suggesting?  Or something else?

Comment 4

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #3)
> I'm not sure I follow.  We have three cases here:
> 
> 1)  not isMember, nullable: holderType should be a "Maybe"
> 2)  not isMember, non-nullable: holderType should be argumentTypeName
> 3)  isMember: holderType should be None
> 
> which is fine, but case 1 has additional complexity, because the
> constructHolder depends on isOptional.
> 
> So I could basically structure this like so:
> 
>   if nullable:
>     # do the optional bits
>     if isMember:
>       holderType = None
>     else:
>       holderType = CGTemplatedType("Maybe", CGGeneric(argumentTypeName))
>   else:
>     if isMember:
>       holderType = None
>     else:
>       holderType = CGGeneric(argumentTypeName)
> 
> Is that what you're suggesting?  Or something else?

How about something like so:

if isMember:
  holderType = None
else:
  holderType = CGGeneric(argumentTypeName)
  if nullable:
    holderType = CGTemplatedType("Maybe", holderType)

and then initialize the rest of the holder and decl stuff after that?

Comment 5

[Boris Zbarsky [:bzbarsky]]

With that change, https://hg.mozilla.org/integration/mozilla-inbound/rev/745ca55d396a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/745ca55d396a

Comment 7

[Andrea Marchesini [:baku]]

Working on bug 949456 I still see errors:

MessageEventBinding.cpp:170:37: error: ‘class mozilla::dom::Optional<mozilla::dom::Nullable<mozilla::dom::OwningWindowProxyOrMessagePort> >’ has no member named ‘SetValue’
MessageEventBinding.cpp:171:37: error: ‘class mozilla::dom::Optional<mozilla::dom::Nullable<mozilla::dom::OwningWindowProxyOrMessagePort> >’ has no member named ‘SetValue’
MessageEventBinding.cpp:308:105: error: invalid initialization of reference of type ‘const mozilla::dom::Optional<mozilla::dom::Nullable<mozilla::dom::OwningWindowProxyOrMessagePort> >&’ from expression of type ‘const mozilla::dom::Nullable<mozilla::dom::OwningWindowProxyOrMessagePort>’
MessageEventBinding.cpp:309:24: error: ‘const class mozilla::dom::Optional<mozilla::dom::Nullable<mozilla::dom::OwningWindowProxyOrMessagePort> >’ has no member named ‘IsNull’

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  That's actually a slightly separate issue which only affects dictionary member unions without default values, which I missed because the tests I added all used things with default values by accident.  I filed bug 952073 to deal with that.