Closed Bug 1253678 Opened 4 years ago Closed 4 years ago

including function.h leads to: |‘Function’ is not a class template| because of confusion with dom::Function

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

(Whiteboard: btpp-active)

Attachments

(3 files)

Attached file log
STR:
1) Build from 'hg pull -r d27ebada59cbf0228ac2eeb25fbe5863a87844f1 https://reviewboard-hg.mozilla.org/gecko'
I don't understand how this code is supposed to work. It looks like we have conflicting definitions of Function.
Flags: needinfo?(nfroyd)
No, that's how things are supposed to work.  We have a declaration of |Function| that takes one argument, and then we specialize it for function types.

What compiler (GCC?) + version is this?
Flags: needinfo?(nfroyd)
I see the same failure on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc4c49cba0df

On my bulldozer machine I have:
gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2

If it saves time I can easily authorized your public key. The machine is accessible via the mozilla VPN.
Ah, the MSVC error is clearer:

 14:58:45     INFO -  c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\obj-firefox\dist\include\mozilla/Function.h(135) : error C2872: 'Function' : ambiguous symbol
 14:58:45     INFO -          could be 'c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\obj-firefox\dist\include\mozilla/Function.h(132) : mozilla::Function'
14:58:45 INFO - or 'c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\dom\base\nsGlobalWindow.h(107) : mozilla::dom::Function' 

We have mozilla::dom::Function:

https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/mozilla/dom/FunctionBinding.h#27

But we have a number of places that (correctly) forward-declare mozilla::dom::Function.  Then when we attempt to forward-declare mozilla::Function, we get complaints about mozilla::dom::Function already existing.  Seems like somebody's namespace resolution algorithm doesn't work correctly?

I'm not really sure what to do about this. :(  I'd be kind of curious if moving the declaration of mozilla::Function to here:

http://mxr.mozilla.org/mozilla-central/source/mfbt/Function.h#41

i.e. prior to the |namespace detail| bit, would do anything different.
(In reply to Nathan Froyd [:froydnj] from comment #4)
> Seems like somebody's namespace resolution algorithm doesn't work
> correctly?

But two compilers? That seems unlikely.

> 
> I'm not really sure what to do about this. :(  I'd be kind of curious if
> moving the declaration of mozilla::Function to here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Function.h#41
> 
> i.e. prior to the |namespace detail| bit, would do anything different.

I don't understand but I tried it and it didn't help.
Couldn't this just be someone doing 'using namespace dom' before we include Function.h?
(In reply to Benoit Girard (:BenWa) from comment #6)
> Couldn't this just be someone doing 'using namespace dom' before we include
> Function.h?

Ah, that seems like a reasonable hypothesis, especially with unified compilation.
But that would still seem to indicate a similar bug in both compilers...
It's probably not a compiler bug since it occurs in MSVC and GCC. Mac-clang might just be okay because of the unification/include path being different.
I think if we rename mozilla::dom::Function to mozilla::dom::DOMFunction this will go away. IMO this is cleaner since DOMFunction isn't compatible with mozilla::Function/std::Function.
Blocks: 1242609
Summary: including function.h leads to: ‘Function’ is not a class template on win/linux but not osx → including function.h leads to: |‘Function’ is not a class template| because of confusion with dom::Function
This patch fixes the problem
Assignee: nobody → bgirard
Component: MFBT → DOM
I think we might want to do the translation in codegen, since this is a standard WebIDL type and so we'd have WebIDL files diverging from their specs for a silly reason :-/. I'm going to confer with bz to see if we can do something clever.
So the problem is that we effectively have two conflicting specs here, right?

On the one hand, there is std::function and we have decided to consistently map std::foo into our code as mozilla::Foo (why not mozilla::foo?).

On the other hand, there is the standard Web IDL Function type, and we map Web IDL Foo into our code as mozilla::dom::Foo.

Is that understanding of the situation correct?   Why are we doing mozilla::Foo instead of mozilla::foo for our equivalents of std::foo?
(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Benoit Girard (:BenWa) from comment #6)
> > Couldn't this just be someone doing 'using namespace dom' before we include
> > Function.h?
> 
> Ah, that seems like a reasonable hypothesis, especially with unified
> compilation.

Confirmed. Here's a reduced testcase triggering the gcc error:


// From dom/FunctionBinding.h
namespace mozilla {
  namespace dom {
    class Function {};
  }
}

// From some cpp file, present here thanks to unified compilation
namespace mozilla {
  using namespace dom;
}

// From mfbt/Function.h
namespace mozilla {
  template <typename>
  class Function;

  template <typename R, typename... A>
  class Function<R(A...)> {};
}


Interestingly, the error only occurs if the partial specialization is present; gcc doesn't mind the declaration of the primary template. That, and the fact that clang accepts this, suggests that this is a compiler bug, though I haven't checked with the standard.
(In reply to Boris Zbarsky [:bz] from comment #14)
> Why are we doing
> mozilla::Foo instead of mozilla::foo for our equivalents of std::foo?

I think it's just to have consistent naming conventions throughout our code.
(In reply to Boris Zbarsky [:bz] from comment #14)
> So the problem is that we effectively have two conflicting specs here, right?
> 
> On the one hand, there is std::function and we have decided to consistently
> map std::foo into our code as mozilla::Foo (why not mozilla::foo?).
> 
> On the other hand, there is the standard Web IDL Function type, and we map
> Web IDL Foo into our code as mozilla::dom::Foo.
> 
> Is that understanding of the situation correct?

Yes, except I don't think this is really a conflict. Namespaces were added to C++ precisely to deal with name clashes like this. Since the two Functions are declared in different namespaces (mozilla and mozilla::dom), they should be able to co-exist peacefully.

Rather, I think the problem is our use of "using namespace". In the presence of unified compilation, this isn't a hygienic construct to use.

We have precedent for removing uses of "using namespace" because they caused trouble with name clashes (e.g. bug 1132153). My suggestion for fixing this bug, is to do the same for "using namespace mozilla::dom". Thoughts?
That would require either a ton of "using mozilla::dom::Foo" or a ton of explicit namespace-qualification through a huge chunk of our codebase.  Doable, but not exactly desirable....

That said, maybe the fact that some of the relevant code is already inside "namespace mozilla { namespace dom {" will make it better.  It's still a good bit of work to actually try it, of course, before you know how ugly the resulting code is.  :(
I don't think it's that bad. These changes are sufficient to get BenWa's patch to compile. WDYT?
Attachment #8728581 - Flags: feedback?(bzbarsky)
Comment on attachment 8728581 [details] [diff] [review]
Remove some uses of "using namespace dom"

Needing to randomly (in the sense that it depends on unification and whether people include Function.h) sprinkle "dom::" all over DOM code is not great.  :(  People will be confused about why "using namespace mozilla::dom" is ok to do in some dom/ directories but not others, for example....

I mean, feedback+ in the sense that it solves the immediate problem, but I'm not terribly happy with the solution.  Renaming one or the other thing would be a strictly better option in my opinion, in terms of developer ergonomics.
Attachment #8728581 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #20)
> Needing to randomly (in the sense that it depends on unification and whether
> people include Function.h) sprinkle "dom::" all over DOM code is not great. 
> :(  People will be confused about why "using namespace mozilla::dom" is ok
> to do in some dom/ directories but not others, for example....

So my answer to this was going to be "the rule will be that you can't do 'using namespace mozilla::dom' anywhere, because we'll remove it from everywhere", but then I saw that there are ~600 uses of "using namespace mozilla::dom" in the codebase, so that's probably not a reasonable proposition. (Previously, I only searched for "using namespace dom", of which there are 36 uses, which seemed more tractable).

> I mean, feedback+ in the sense that it solves the immediate problem, but I'm
> not terribly happy with the solution.  Renaming one or the other thing would
> be a strictly better option in my opinion, in terms of developer ergonomics.

Given the above, I agree.

The question that remains is which to rename. My understanding from comment 13 is that we can't rename the WebIDL Function type, and so our options are:

  - Hack the WebIDL binding generator to generate a C++ name other than Function
    for the WebIDL Function type.

  - Rename mozilla::Function. We could choose a more verbose name, like
    mozilla::TypeErasedFunction, or break consistency with naming convention 
    and call it mozilla::function.

Do you have a preference?
We _can_ rename the Web IDL function type.  It might be a bit confusing to people, but should be doable.  We can avoid the divergence in actual Web IDL files via typedef, at least until we start spitting out C++ typedefs for Web IDL typedefs.

I think my personal preference is to go with mozilla::function, but I'm not set on it.
(In reply to Boris Zbarsky [:bz] from comment #22)
> I think my personal preference is to go with mozilla::function, but I'm not
> set on it.

We would need to do this anyway when we start using std::function...we might as well do it now and get it over with.
Ohhh I though it was std::Function which would have had the same problem. If this issue is not case insensitive then that's probably the best approach.
Moving to std::function is going to require redoing all the |using| statements for Function, isn't it?  Seems like there's work no matter what you do.  And getting rid of |using namespace| has benefits independent of this particular problem point.  I would prefer working to kill off |using namespace| usage.

Frankly, over time I've been warming to what WebKit's WTF does: have all its basic data structures, algorithms, etc. *not* in the main namespace used by WebKit code.  We would end this sort of problem for all time if mfbt stuff were in namespace mfbt.  (We would of course have to *not* do what WTF does, and have |using WTF::Vector;| things in every one of the WTF headers.)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25)
> Moving to std::function is going to require redoing all the |using|
> statements for Function, isn't it?

My understanding is the proposal on the table is to rename mozilla::Function to mozilla::function (no namespace change).

> Seems like there's work no matter what
> you do.  And getting rid of |using namespace| has benefits independent of
> this particular problem point.  I would prefer working to kill off |using
> namespace| usage.

Agreed, but it's a lot more work (~600 usages of |using namespace mozilla::dom|) than renaming mozilla::Function (a handful of usages).
Whiteboard: btpp-active
I agree with Botond. I'm going to write a patch to fix the usage of mozilla::Function. We can spin off the DOM namespace changes in another bug since it has a larger scope and it's required to address this problem.
Sorry I meant not required to address this problem.
Comment on attachment 8726923 [details]
MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38331/diff/1-2/
Attachment #8726923 - Attachment description: MozReview Request: Bug 1253678 - Rename Function to DOMFunction to avoid confusion with mozilla::Function. r?peterv → MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd
Attachment #8726923 - Flags: review?(peterv) → review?(nfroyd)
Comment on attachment 8726923 [details]
MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd

https://reviewboard.mozilla.org/r/38331/#review36211

r=me.  Thanks for doing this!
Attachment #8726923 - Flags: review?(nfroyd) → review+
Build failure is coming from a skia build trick. We're colliding with std::function on platforms that have it be because it's import it in the std namespace. I'm using c++ alias declaration to continue to use the same work around.

diff --git a/gfx/skia/skia/include/private/SkTLogic.h b/gfx/skia/skia/include/private/SkTLogic.h

-    using mozilla::Function;
+    // If we have 'using mozilla::function', we're going to collide with
+    // 'std::function' on platforms that have it. Therefore we use a macro
+    // work around.
+    template<typename Signature>
+    using Function = mozilla::function<Signature>;
     #define function Function

 
I'll assume I can carry forward review with this.
Comment on attachment 8726923 [details]
MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38331/diff/2-3/
Attachment #8726923 - Flags: review?(peterv)
Flags: needinfo?(bgirard)
Attachment #8726923 - Flags: review?(peterv)
https://hg.mozilla.org/mozilla-central/rev/c3c4bc792bb5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.