Closed Bug 1470325 Opened 6 years ago Closed 6 years ago

WebIDL `interface FooBinding` generated code name-conflicts with generated `interface Foo` code

Categories

(Core :: DOM: Bindings (WebIDL), enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(3 files)

The WebGPU sketch IDL includes `WebGPUBuffer` and `WebGPUBufferBinding` among other pairs.
As I posted to dev-platform, I took a stab at switching from dom::%sBinding to dom::bindings::%s, and I think I'm almost there aside from a testcase that has a New And Exciting name collision:

> 1:54.21 In file included from /Users/jgilbert/dev/mozilla/objs/gecko5/js/xpconnect/src/Unified_cpp_js_xpconnect_src0.cpp:11:
> 1:54.21 In file included from /Users/jgilbert/dev/mozilla/gecko5/js/xpconnect/src/Sandbox.cpp:59:
> 1:54.21 In file included from /Users/jgilbert/dev/mozilla/objs/gecko5/dist/include/mozilla/dom/UnionConversions.h:69:
> 1:54.21 /Users/jgilbert/dev/mozilla/objs/gecko5/dist/include/mozilla/dom/TestInterfaceJSMaplikeSetlikeIterableBinding.h:744:78: error: must use 'class' tag to refer to type 'TestInterfaceMaplike' in this scope
> 1:54.21   Set(mozilla::dom::TestInterfaceMaplikeObject* self, const nsAString& aKey, TestInterfaceMaplike& aValue, ErrorResult& aRv);
> 1:54.21                                                                              ^
> 1:54.21                                                                              class
> 1:54.21 /Users/jgilbert/dev/mozilla/objs/gecko5/dist/include/mozilla/dom/TestInterfaceJSMaplikeSetlikeIterableBinding.h:660:11: note: class 'TestInterfaceMaplike' is hidden by a non-type declaration of 'TestInterfaceMaplike' here
> 1:54.21 namespace TestInterfaceMaplike {
> 1:54.21           ^

Since this declaration is within namespace `dom::bindings::TestInterfaceMapLikeObject`, `TestInterfaceMapLike` either needs a class/typename annotation or to be properly namespaced. I played around with Codegen.py a bunch but couldn't see an easy way to do this.

Thoughts?

Current changes:
https://github.com/jdashg/gecko-cinn/commits/bindings-namespace
Flags: needinfo?(kyle)
Flags: needinfo?(kyle)
Priority: -- → P3
Oops. Forgot to unmark the ni? resolve on this. I'm still trying to figure out what the best way to do this is so re-ni?'ing myself until that's done.
Flags: needinfo?(kyle)
See Also: → 1469376
Comment on attachment 8987725 [details]
Bug 1470325 - Update Codegen.py to emit Foo_Binding instead of FooBinding. -

https://reviewboard.mozilla.org/r/252984/#review259760

lgtm but I want to get bz's input since the namespace idea was already discussed with him. I'm fine with underscores and think they'll be less confusing than the namespaces and possible collisions, just want to make sure I'm not missing anything.

We'll probably need to update the WebIDL MDN page too, once it lands.

::: dom/bindings/Codegen.py:1747
(Diff revision 1)
>  
>  def objectMovedHook(descriptor, hookName, obj, old):
>      assert descriptor.wrapperCache
>      return fill("""
>          if (self) {
> -          UpdateWrapper(self, self, ${obj}, ${old});
> +        UpdateWrapper(self, self, ${obj}, ${old});

nit: Accidental whitespace removal?
Attachment #8987725 - Flags: review?(kyle) → review+
Flags: needinfo?(kyle)
Attachment #8987725 - Flags: review?(bzbarsky)
So the problem with the dom::bindings::Foo proposal was that then you get name collisions with the dom::Foo implementation class when actually inside the dom::bindings namespace?

For the specific example of TestInterfaceMapLike, we're running into a problem because we use the same code for example codegen and callback codegen and the former wants pretty-looking types so we strip off "mozilla::dom::" when we can (see bug 1000675).  That's pretty simple to fix and limit the stripping to example codegen if we want.
Comment on attachment 8987725 [details]
Bug 1470325 - Update Codegen.py to emit Foo_Binding instead of FooBinding. -

https://reviewboard.mozilla.org/r/252984/#review259800

r=me.  I guess this is fewer chars than "bindings::" and definitely won't cause name collisions... until someone adds an interface with '_' in it, I guess.
Attachment #8987725 - Flags: review?(bzbarsky) → review+
Comment on attachment 8987726 [details]
Bug 1470325 - s/FooBinding/Foo_Binding/g -

https://reviewboard.mozilla.org/r/252986/#review259814
Attachment #8987726 - Flags: review?(kyle) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> So the problem with the dom::bindings::Foo proposal was that then you get
> name collisions with the dom::Foo implementation class when actually inside
> the dom::bindings namespace?
> 
> For the specific example of TestInterfaceMapLike, we're running into a
> problem because we use the same code for example codegen and callback
> codegen and the former wants pretty-looking types so we strip off
> "mozilla::dom::" when we can (see bug 1000675).  That's pretty simple to fix
> and limit the stripping to example codegen if we want.

Yeah, also having poked at it some while working on this, I'm not sure how obvious it'll be to other devs that you need something from the binding::X namespace now when using class X, versus having a completely different X_Binding class. X_Binding isn't exactly pretty (and does break with style in general), but it's slightly more obviously than binding::X, and hopefully more difficult to screw up.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> Comment on attachment 8987725 [details]
> Bug 1470325 - Update Codegen.py to emit Foo_Binding instead of FooBinding. -
> 
> https://reviewboard.mozilla.org/r/252984/#review259800
> 
> r=me.  I guess this is fewer chars than "bindings::" and definitely won't
> cause name collisions... until someone adds an interface with '_' in it, I
> guess.

I'm hoping that a webidl interface 'Foo_Binding' wouldn't be acceptable web-style-wise, so we shouldn't have to deal with this, yeah?
> I'm hoping that a webidl interface 'Foo_Binding' wouldn't be acceptable web-style-wise

No guarantees of that, sadly.  Especially for cases that are not web-exposed names like NoInterfaceObject interfaces, callbacks, dictionaries, etc.
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba24316f5d84
Update Codegen.py to emit Foo_Binding instead of FooBinding. - r=bz,qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97feb8161b7
s/FooBinding/Foo_Binding/g - r=qdot
https://hg.mozilla.org/mozilla-central/rev/ba24316f5d84
https://hg.mozilla.org/mozilla-central/rev/a97feb8161b7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.