Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend the dfn of object types. #211

Closed
wants to merge 2 commits into from
Closed

Conversation

tobie
Copy link
Collaborator

@tobie tobie commented Oct 27, 2016

Update [SameObject] and [NewObject] to rely on it.

Fixes whatwg#71
Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27605
the [=promise type=],
the [=exception types=],
the [=buffer source types=], and
[=union types=] whose [=member types|members=] are [=object types=]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/are/contain/, no? Otherwise a union with a string would not be considered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise a union with a string would not be considered.

Mistakenly thought this was the point. Good catch.

@tobie
Copy link
Collaborator Author

tobie commented Oct 27, 2016

Do we have to account for null types, especially within unions? i.e. are the following object types (currently neither are, if my understanding of what I wrote is correct)?

(DOMstring or DOMException?)
(float or (DOMstring or DOMException)?)

@domenic
Copy link
Member

domenic commented Oct 27, 2016

This LGTM but I'd like @bzbarsky and/or @heycam to check, as I'm not confident about the cases like union types or null types. For example I'm not sure we actually want [SameObject]/[NewObject] to apply to (DOMString or DOMException), but I guess in https://www.w3.org/Bugs/Public/show_bug.cgi?id=27605#c1 @bzbarsky says we do, so I'd rather trust him on this.

@tobie
Copy link
Collaborator Author

tobie commented Nov 7, 2016

Two questions for @bzbarsky so I can fix and merge this:

  1. In your bugzilla comment you say:

    I think restricting [NewObject] to only things that return interface types is just wrong. It should be allowed if the return value is any object type. Specifically, any interface type, "object", a promise, an Error, a DOMException, any buffer source type, or a union of types on which [NewObject] is allowed.

    It is not clear, from my perspective, whether "a union of types on which [NewObject] is allowed" means (object_type or object_type) or (object_type or non_object_type) (@annevk needs the latter for "fetch", I think you mean the former).

  2. Do we have to account for null types, especially within unions? See my previous comment.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Nov 7, 2016

it is not clear, from my perspective, whether "a union of types on which [NewObject] is allowed" means (object_type or object_type) or (object_type or non_object_type)

Good question.

For a return value that doesn't always return an object (i.e. the latter case), it's not clear what [NewObject] would really mean. Or rather, the current definition of [NewObject] as "a reference to a newly created object must always be returned" doesn't make sense in that case, right? Nullable types have the same concern. That makes the given PR not really make much sense, when read literally.

We could make that definition more complex to make it more broadly applicable, of course. But normally [NewObject] functions would be some sort of a "construct an object and return it" function. It's pretty odd to allow such a thing to also return non-objects.

What is the fetch use case here?

@tobie
Copy link
Collaborator Author

tobie commented Nov 7, 2016

That makes the given PR not really make much sense, when read literally.

Agreed, my initial commit read: [=union types=] whose [=member types|members=] are [=object types=], which I believe is what you had in mind.

What is the fetch use case here?

I'm not sure. @annevk?

@bzbarsky
Copy link
Collaborator

bzbarsky commented Nov 7, 2016

my initial commit read: [=union types=] whose [=member types|members=] are [=object types=], which I believe is what you had in mind.

It's what I had in mind, but I'm open to considering the other option, and a more complicated definition of [NewObject], once I understand the use cases.

Note that [SameObject] need not be restricted to the same types as [NewObject]; we can have a [Constant] annotation that makes sense for all types, not just object types.... and I think we should have done that instead of [SameObject].

@tobie
Copy link
Collaborator Author

tobie commented Nov 7, 2016

we can have a [Constant] annotation that makes sense for all types, not just object types.... and I think we should have done that instead of [SameObject].

If the issue is just the name, I'm sure we can find something more suitable than [NewObject] to express the opposite of [Constant].

@bzbarsky
Copy link
Collaborator

bzbarsky commented Nov 7, 2016

Well, the opposite of [Constant] is "yeah, it might change". [NewObject] is not the same thing as the opposite of [Constant] at all. ;)

@tobie
Copy link
Collaborator Author

tobie commented Nov 7, 2016

Yeah, I meant something with the same semantics as [NewObject] but whose name would make sense for literals too.

@tobie tobie assigned annevk and unassigned bzbarsky and heycam Nov 7, 2016
@annevk
Copy link
Member

annevk commented Nov 8, 2016

Sorry, for Fetch I only need [NewObject] to work for promises. I don't need it to work for strings or nullables. I think I ended up confusing this issue a bit.

#212 (and its corresponding Bugzilla bug) has a proposal for revamping [SameObject]. I agree with @bzbarsky that we don't have to retain parity with [NewObject]. [SameObject] or [Constant] should be its own thing with its own semantics that make sense for it.

@tobie
Copy link
Collaborator Author

tobie commented Nov 8, 2016

Sorry, for Fetch I only need [NewObject] to work for promises. I don't need it to work for strings or nullables. I think I ended up confusing this issue a bit.

It's absolutely all your fault. :)

There are currently no use cases for the (non_object or object) construct, so I can just remove that out from the PR, e.g.:

The {{object}} type,
all [=interface types=],
the [=promise type=],
the [=exception types=],
the [=buffer source types=], and
[=union types=] whose [=member types|members=] are [=object types=]
are known as <dfn id="dfn-object-type" export>object types</dfn>.

On the other hand, as @dontcallmedom mentions in #71, The ServiceWorker spec uses a bunch of constructs with a nullable type, e.g.:

[SameObject] readonly attribute (Client or ServiceWorker or MessagePort)? source;

Would something like this work to cover that use case and still make sense given the dfn name?

The {{object}} type,
all [=interface types=],
the [=promise type=],
the [=exception types=],
the [=buffer source types=],
[=union types=] whose [=member types|members=] are [=object types=], and
[=nullable types=] whose [=inner type=] is an [=object type=]
are known as <dfn id="dfn-object-type" export>object types</dfn>.

@annevk
Copy link
Member

annevk commented Nov 8, 2016

I don't think we should focus on addressing [SameObject] use cases here. I think everyone is coming around that we want to do it differently per the other issues. If we did that, we'd automatically account for nullables and strings and such there I think.

@tobie
Copy link
Collaborator Author

tobie commented Nov 8, 2016

Well, my understanding is the only difference between the requirements is [SameObject] wants objects to be nullable.

Given null is always === null, it doesn't bother me that [NewObject] could return the same null twice and thus to treat both [NewObject] and [SameObject] similarly (of course, we'd need to special case the language for the case of null in [NewObject]).

@bzbarsky
Copy link
Collaborator

bzbarsky commented Nov 8, 2016

Here's a concrete example of why the current [SameObject] thing is silly: Node has various string-or-null-valued attributes (localName, namespaceID) that can never change and we should be able to annotate that. Right now we can't.

@annevk
Copy link
Member

annevk commented Nov 8, 2016

@tobie do we have a case where a [NewObject]-operation can return null or a string? On the face of it something like that seems rather weird.

@tobie
Copy link
Collaborator Author

tobie commented Nov 8, 2016

@bzbarsky wrote:

Here's a concrete example of why the current [SameObject] thing is silly: Node has various string-or-null-valued attributes (localName, namespaceID) that can never change and we should be able to annotate that. Right now we can't.

Oh, OK. So there are requirements I wasn't aware of.

So what's the plan?

  1. Define object types as:

    The {{object}} type,
    all [=interface types=],
    the [=promise type=],
    the [=exception types=],
    the [=buffer source types=], and
    [=union types=] whose [=member types|members=] are [=object types=]
    are known as <dfn id="dfn-object-type" export>object types</dfn>.
    
  2. Make [NewObject] refer to it.

  3. Add a new [Constant] extended attribute that's works on any type and always returns the same reference for objects and the same value otherwise (so [Constant] readonly attribute DOMString tagName; means that if tagName returns "DIV" once, it'll always do so.)

  4. What to do with [SameObject]? Mark it as legacy?

@annevk wrote:

do we have a case where a [NewObject]-operation can return null or a string? On the face of it something like that seems rather weird.

I wasn't aware string was a requirement for [SameObject]. Agreed it's potentially a bit weird for [NewObject] but null doesn't strike me as absurd.

@annevk
Copy link
Member

annevk commented Nov 8, 2016

Maybe not absurd, but if we have no legit cases… I think we should just remove [SameObject] when we add the improved version and file issues downstream.

@tobie
Copy link
Collaborator Author

tobie commented Nov 8, 2016

Maybe not absurd, but if we have no legit cases… I think we should just remove [SameObject] when we add the improved version and file issues downstream.

What's the process for removing something like that? Keep it around the spec and mark it as deprecated? Remove it altogether? How do we easily know what specs to file against? Or should we just get a warning out in Bikeshed and Respec and hope for the best?

@annevk
Copy link
Member

annevk commented Nov 8, 2016

Just doing it and gradually fixing dependencies seems fine. Browser engineers can prolly grep all IDL files too to see what needs fixing.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Nov 8, 2016

How do we easily know what specs to file against?

In Gecko's IDL, looks like [SameObject] is used for:

There are also some cases of specs using it while Gecko does not (e.g. PushSubscription.options)...

@tobie
Copy link
Collaborator Author

tobie commented Mar 27, 2017

This needs to be completely revisited. Closing.

@tobie tobie closed this Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants