Add [LegacyNamespace] to allow defining an interface in a namespace #425

littledan · 2017-08-23T15:14:11Z

This is necessary to specify certain APIs such as WebAssembly,
which includes several constructors as properties of the root
WebAssembly object. See a WebAssembly draft specification where
this is in use at https://github.com/littledan/spec/blob/new-js.bs/document/JS.bs

Preview | Diff

littledan · 2017-08-23T15:14:43Z

This is just a first pass; I'd be interested in any feedback you might have. cc @domenic .

domenic

A number of nits, but overall this seems good.

I'm on the fence as to whether we should add some sort of usage note, along the lines of "most places in the platform don't do this; maybe check first, and don't go crazy using it everywhere."

domenic · 2017-08-24T18:49:22Z

index.bs

+
+If the [{{InNamespace}}] [=extended attribute=] appears on an [=interface=], it indicates that
+the [=interface object=] for this interface will not be created as a property of the global
+object, but rather the namespace which is the argument to the extended attribute.


"but rather as a property of" is clearer, I think

"namespace identified by the argument", I think

[=namespace=]

domenic · 2017-08-24T18:51:47Z

index.bs

+The [=class string=] of an interface with the [{{InNamespace}}] extended
+attribute is the concatenation of the class string of the namespace, <code>"."</code>,
+and the class string that the interface would otherwise have without this
+extended attribute.


I think this should not be located here but instead be a modification to

The class string of a platform object that implements one or more interfaces must be the identifier of the primary interface of the platform object.

Also, no <code> around strings in Web IDL, it appears.

domenic · 2017-08-24T18:52:58Z

index.bs

@@ -12516,6 +12558,10 @@ The characteristics of a namespace object are described in [[#namespace-object]]
        1.  Let |F| be the result of [=creating an operation function|creating an operation function=]
            given |op|, |namespace|,  and |realm|.
        1.  Perform [=!=] [=CreateDataProperty=](|namespaceObject|, |op|'s [=identifier=], |F|).
+    1.  For each [=exposed=] [=interface=] |intr| which has the [{{InNamespace}}] extended attribute, with the identifier of this namespace as its argument,


Wrap to 100 chars here too

[=identifier=]

Just interface instead of intr is nicer, I think.

No comma before "with", I think.

domenic · 2017-08-24T18:58:12Z

index.bs

@@ -12516,6 +12558,10 @@ The characteristics of a namespace object are described in [[#namespace-object]]
        1.  Let |F| be the result of [=creating an operation function|creating an operation function=]
            given |op|, |namespace|,  and |realm|.
        1.  Perform [=!=] [=CreateDataProperty=](|namespaceObject|, |op|'s [=identifier=], |F|).
+    1.  For each [=exposed=] [=interface=] |intr| which has the [{{InNamespace}}] extended attribute, with the identifier of this namespace as its argument,
+        1.  Let |I| be the [=interface object=] for |intr|.
+        1.  Let |newDesc| be the PropertyDescriptor{ \[[Writable]]: <emu-val>true</emu-val>, \[[Enumerable]]: <emu-val>false</emu-val>, \[[Configurable]]: <emu-val>true</emu-val>, \[[Value]]: |I| }.


It looks like at this point we don't put spaces after { and before } in Web IDL.

tobie · 2017-08-24T20:22:54Z

Thanks for the PR!

A comment and some bikeshedding.

My understanding was the wanted to avoid such constructs in favor of waiting for modules instead, but maybe things have changed?

If we do go this route though, shouldn't we go for dedicated syntax here instead of (ab)using extended attributes? E.g.: something like:

namespace Foo { };

interface Foo.Bar {
};

interface Foo.Baz : Foo.Bar {
};

And if we do choose extended attributes, maybe [Namespace=Foo] reads better than [InNamespace=Foo].

tobie · 2017-08-24T20:23:40Z

I'm on the fence as to whether we should add some sort of usage note, along the lines of "most places in the platform don't do this; maybe check first, and don't go crazy using it everywhere."

I know @rwaldron is going to jump on this.

TimothyGu · 2017-08-25T00:06:09Z

I would definitely prefer @tobie's syntax.

domenic · 2017-08-25T00:12:50Z

My thinking for advising @littledan to go with this instead of new syntax is that we already use extended attributes to control what gets exposed for a given interface (viz. [NamedConstructor], [NoInterfaceObject], and [Exposed]). Plus, it's always a bonus when we don't need to update every IDL toolchain to add a new feature.

TimothyGu · 2017-08-25T00:22:33Z

The main issue for me is that this would disallow the following fragment since the interface name is the same (or at least I think, I cannot find the clause that disallows this but it seems reasonably logical):

[Exposed=Window]
interface Foo {};

[Exposed=Window]
namespace bar {};
[InNamespace=bar]
interface Foo {};

... and namespacing was kind of the point of namespaces.

On the other hand, even if we do allow that, it can be difficult (for a human) to immediately recognize Foo was not indeed exposed globally but through a namespace.

I agree that a syntax change can cause churn, no doubt about that, but I still believe it's the best way forward.

annevk · 2017-08-25T07:01:16Z

Whether more dedicated syntax makes sense very much depends on whether we want this kind of namespacing going forward or use some kind of builtin modules.

Does Wasm really need this?

tobie · 2017-08-25T07:29:22Z

My thinking for advising @littledan to go with this instead of new syntax is that we already use extended attributes to control what gets exposed for a given interface (viz. [NamedConstructor], [NoInterfaceObject], and [Exposed]).

If we could end-up in a situation where those extended attributes are mostly there for legacy purposes, I think that would be better. We're adding new syntax for mixins soon, so [NoInterfaceObject] will be a legacy construct.

Plus, it's always a bonus when we don't need to update every IDL toolchain to add a new feature.

Well, for namespaces to be at all useful, we'd be breaking the invariant that identifiers are unique, so we're in for a world of pain anyway (e.g. idlharness assumes interface identifiers are unique).

tobie · 2017-08-25T07:32:54Z

Whether more dedicated syntax makes sense very much depends on whether we want this kind of namespacing going forward or use some kind of builtin modules.

Does Wasm really need this?

I agree with @annevk's sentiment here. It seems weird to suddenly allow this for a part of the platform for implementor/editor convenience when we've been refusing it for the rest of the platform despite developers requiring it for a long time (e.g. for grouping all sensors under a single namespace, see w3c/sensors#127).

littledan · 2017-08-25T08:16:48Z

Historically, WebAssembly seems to have somehow skipped the WebIDL formalization and TAG review before shipping (blink-dev Intent to Ship thread), so it recently got out of the door with these namespaces. It has now be shipping in Chrome for around six months, and has also shipped in Firefox. At this point, it could be a risk to web compatibility to change the API to be anything but namespaced.

The issue that @TimothyGu raises sort of comes up in practice already: Between WebAssembly and HTML, there are two interfaces called Table. Bikeshed seems to be able to disambiguate just fine in the draft WebAssembly/JS spec, but I'm not sure if this could create some sort of problem in the future. @tobie 's syntax could get around this issue. I was confused here; not sure why I thought this was the case.

annevk · 2017-08-25T08:20:08Z

HTML doesn't define an interface called Table as far as I know.

littledan · 2017-08-25T08:36:13Z

Sorry, not sure why I got confused about that; I believe in some past version there were some funny cross-references generated, but I don't see them now.

tobie · 2017-08-25T08:38:54Z

HTML doesn't define an interface called Table as far as I know.

So if we agree this was a design mistake and that we keep it for Web compat reasons, we can restrict it to the existing set of interfaces defined by WebAssembly and keep the interface identifier uniqueness invariant as a requirement. In that case, we should call it [LegacyNamespace] and add an advisement to it such as we do for [LegacyArrayClass].

littledan · 2017-08-25T08:45:23Z

OK, LegacyNamespace seems to be the way to go for now, though I won't be surprised if, in the future, others come in wanting interfaces in namespaces ("WebAssembly has it, so why can't my API?").

tobie · 2017-08-25T09:20:44Z

though I won't be surprised if, in the future, others come in wanting interfaces in namespaces

I won't either. Which is why I suggested dedicated syntax. If this opens up to other use cases, the unique identifier invariant won't hold long, and we'll need to address this in the spec.

littledan · 2017-08-25T09:22:52Z

Would it be reasonable to start with the LegacyNamespace form, and consider adding dedicated syntax later (and going back and updating the WebAssembly spec) if it turns out to be needed for other specifications?

tobie · 2017-08-25T09:36:00Z

Would it be reasonable to start with the LegacyNamespace form, and consider adding dedicated syntax later (and going back and updating the WebAssembly spec) if it turns out to be needed for other specifications?

I'm fine with that. Can you add an advisement in the same style as those I just added for all the [LegacyX] stuff?

littledan · 2017-08-25T16:35:21Z

@tobie I believe this PR should be in that same style.

tobie · 2017-08-25T16:40:33Z

I just merged some more consistent text earlier for all of these advisement (always at the top, with a link to a file an "intent to use" GH issue before using it, etc. ), think you could follow that format?

This is necessary to specify certain APIs such as WebAssembly, which includes several constructors as properties of the root WebAssembly object. See a WebAssembly draft specification where this is in use at https://github.com/littledan/spec/blob/new-js.bs/document/JS.bs

littledan · 2017-08-25T16:53:22Z

Done

tobie

A few other nits (sorry for the back-and-forth, I've been mostly OoO today, and looking at this stuff on a phone isn't ideal). Would like to get @bzbarsky to have a look before this is merged. Think he's back next week.

tobie · 2017-08-25T17:24:10Z

index.bs

+<h4 id="LegacyNamespace" extended-attribute lt="LegacyNamespace">[LegacyNamespace]</h4>
+
+<p class="advisement">
+    The [{{LegacyArrayClass}}] [=extended attribute=] is an undesirable feature.


s/LegacyArrayClass/LegacyNamespace/

tobie · 2017-08-25T17:25:35Z

index.bs

@@ -10135,7 +10186,7 @@ Note: The HTML Standard defines how a security check is performed. [[!HTML]]

 For every non-callback [=interface=] that is [=exposed=] in
 a given ECMAScript global environment and that is not declared with
-the [{{NoInterfaceObject}}] [=extended attribute=],
+the [{{NoInterfaceObject}}] or [{{LegacyNamespace}}] [=extended attribute|extended attributes=],


s/[=extended attribute|extended attributes=]/[=extended attributes=]/ (Bikeshed should recognize the plurals.

littledan · 2017-08-25T18:20:28Z

Fixed those nits.

tobie · 2017-08-30T16:00:47Z

Just discussed this with @bzbarsky. He pointed out that Mozilla's Wasm implementation wasn't relying on WebIDL at all so far, which creates significant differences with what a WebIDL-based implementation would look like. Is Chrome's implementation WebIDL-based? @lukewagner, what's Mozilla's plan here? Are implementations going to converge on a WebIDL-based spec?

lukewagner · 2017-08-30T16:10:00Z

Yes, we currently hand-code our wasm JS API bindings in the JS engine. However, we should be able to precisely match the order of coercions and property descriptor values to match a Web IDL spec without much trouble. The plan is also to write plentiful wpts for all the observable differences.

tobie · 2017-08-30T16:12:54Z

Thanks, @lukewagner. So if the plan is for all to move to WebIDL semantics, we should just go-ahead with this.

tobie · 2017-08-30T16:32:17Z

That said, are we sure that the backwards compatibility issues that will be created by moving to a WebIDL-based specification aren't the occasion to eschew [LegacyNamespace] altogether and move to the more standard NamespaceInterface format seen elsewhere?

lukewagner · 2017-08-30T16:35:04Z

From the changes I anticipate (order of coercions, enumerability of properties), I don't think breakage will actually occur in the wild at this point by simply moving to WebIDL (and there's probably already divergence between browsers in these minute details).

littledan · 2017-08-30T16:39:11Z

V8's implementation is not WebIDL-based either, but I plan to work on those wpts. Thanks for the merge!

annevk · 2017-08-30T16:55:57Z

As far as I know the differences are fairly minor: #226. But they are a source of annoyance. I still wonder whether we should offer some kind of opt-in mechanism to get JavaScript-style classes rather than IDL-style classes. Then we could also potentially describe other things with IDL, such as Promise and ReadableStream, if we wanted to.

lukewagner · 2017-08-30T16:58:05Z

@annevk That sounds even better :)

domenic self-assigned this Aug 23, 2017

domenic reviewed Aug 24, 2017

View reviewed changes

littledan changed the title ~~Add [InNamespace] to allow defining an interface in a namespace~~ Add [LegacyNamespace] to allow defining an interface in a namespace Aug 25, 2017

tobie requested a review from bzbarsky August 25, 2017 12:52

littledan added 4 commits August 25, 2017 18:47

Changes based on PR feedback

cb42d76

Delete stray text accidentally copied

d22bded

Alphabetize and harmonize advertisement

6ecbbc9

littledan force-pushed the namespace-interface branch from 2d01439 to 6ecbbc9 Compare August 25, 2017 16:53

tobie reviewed Aug 25, 2017

View reviewed changes

Fixups from review

7f1899b

tobie mentioned this pull request Aug 30, 2017

Track PR #425 #429

Closed

8 tasks

tobie merged commit 93dc13d into whatwg:master Aug 30, 2017

littledan mentioned this pull request Dec 4, 2018

Declaring modules in WebIDL #592

Open

anssiko mentioned this pull request Apr 1, 2021

Prefix generic interface names? webmachinelearning/webnn#141

Closed

Add [LegacyNamespace] to allow defining an interface in a namespace #425

Add [LegacyNamespace] to allow defining an interface in a namespace #425

Conversation

littledan commented Aug 23, 2017 • edited by pr-preview bot Loading

littledan commented Aug 23, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Aug 24, 2017

tobie commented Aug 24, 2017

TimothyGu commented Aug 25, 2017

domenic commented Aug 25, 2017

TimothyGu commented Aug 25, 2017

annevk commented Aug 25, 2017

tobie commented Aug 25, 2017

tobie commented Aug 25, 2017

littledan commented Aug 25, 2017 • edited Loading

annevk commented Aug 25, 2017

littledan commented Aug 25, 2017

tobie commented Aug 25, 2017

littledan commented Aug 25, 2017

tobie commented Aug 25, 2017

littledan commented Aug 25, 2017

tobie commented Aug 25, 2017 • edited Loading

littledan commented Aug 25, 2017

tobie commented Aug 25, 2017

littledan commented Aug 25, 2017

tobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Aug 25, 2017

tobie commented Aug 30, 2017

lukewagner commented Aug 30, 2017

tobie commented Aug 30, 2017

tobie commented Aug 30, 2017

lukewagner commented Aug 30, 2017

littledan commented Aug 30, 2017

annevk commented Aug 30, 2017

lukewagner commented Aug 30, 2017

littledan commented Aug 23, 2017 •

edited by pr-preview bot

Loading

littledan commented Aug 25, 2017 •

edited

Loading

tobie commented Aug 25, 2017 •

edited

Loading