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

Unable to resolve friend function and implementation #627

Open
YannickJadoul opened this issue Jan 28, 2021 · 6 comments
Open

Unable to resolve friend function and implementation #627

YannickJadoul opened this issue Jan 28, 2021 · 6 comments
Labels
bug Problem in existing code code Source code

Comments

@YannickJadoul
Copy link

YannickJadoul commented Jan 28, 2021

I might be doing something wrong, but could it be that the changes from #623 (related to #613) have made forward declarations of friend functions ambiguous?

Our code (in pybind/pybind11#2828) looks something like this:

class object : public handle {
// ...
protected:
// ...
    template <typename T> friend T reinterpret_borrow(handle);
    template <typename T> friend T reinterpret_steal(handle);
// ...
};

/** \rst
    Blah blah
\endrst */
template <typename T> T reinterpret_borrow(handle h) { return {h, object::borrowed_t{}}; }

/** \rst
    Blah blah blah
\endrst */
template <typename T> T reinterpret_steal(handle h) { return {h, object::stolen_t{}}; }

When I don't wrap the two friend forward declarations in a #ifdef DOXYGEN_SHOULD_SKIP_THIS, breathe comes complaining as follows:

/home/yannick/pybind11/docs/reference.rst:42: WARNING: doxygenfunction: Unable to resolve function "reinterpret_borrow" with arguments None in doxygen xml output for project "pybind11" from directory: .build/doxygenxml/.
Potential matches:
- template<typename T> T reinterpret_borrow(handle h)
- template<typename T> friend T reinterpret_borrow(handle)
/home/yannick/pybind11/docs/reference.rst:44: WARNING: doxygenfunction: Unable to resolve function "reinterpret_steal" with arguments None in doxygen xml output for project "pybind11" from directory: .build/doxygenxml/.
Potential matches:
- template<typename T> T reinterpret_steal(handle h)
- template<typename T> friend T reinterpret_steal(handle)

I bumped the version from 4.25.1 to 4.26.1, and this error appeared. As far as I know, things just worked before that version bump. Can I disambiguate between these two in some way, or is this an issue?

@vermeeren
Copy link
Collaborator

@YannickJadoul Can you post the RST in question? You should be able to make it work by specifying the arguments explicitly, the checks for this have gotten more strict in #623 as you have noticed.

@YannickJadoul
Copy link
Author

@vermeeren Sure! It's just a simple

.. doxygenfunction:: reinterpret_borrow

.. doxygenfunction:: reinterpret_steal

I've also tried reinterpret_borrow(handle) or reinterpret_borrow(handle h), but this gives the same issue.

.. doxygenfunction:: friend reinterpret_borrow(handle) would select the wrong version (as that one is the forward declaration that doesn't contain the documentation), but doesn't work anyway:

/home/yannick/pybind11/docs/reference.rst:42: WARNING: doxygenfunction: Cannot find function "friend reinterpret_borrow" in doxygen xml output for project "pybind11" from directory: .build/doxygenxml/

@YannickJadoul
Copy link
Author

I've made a minimal example. That should hopefully make things easier for you than a vague description.

class Foo {
    friend void bar();
};

namespace mwe {
/// Wenn ist das Nunstück git und Slotermeyer? Ja! Beiherhund das Oder die Flipperwaldt gersput!
void bar() {}
}
.. doxygenfunction:: bar

Here's the full minimal working example zipped, which should hopefully just show the error when running make html: mwe.zip

PS: I had to also wrap foo in a namespace, or the function wouldn't be found. Is this intentional, or another issue?

@jakobandersen
Copy link
Collaborator

I think this is a real issue, but a long-standing one that got exposed by the friend fixes. But I don't think it is easy to fix.
The fundamental problem is that a friend function in class scope may sometimes be just an instruction to consider the function a friend, but sometimes it may the a declaration of a function only visible through ADL, which in some sense makes it tighter coupled with the class.
For example, if one has code like

struct Foo;
struct Bar:


void doStuff(const Foo &f, const Bar &b); // #1

struct Foo {
   // ...
public:
   friend void doStuff(const Foo &f, const Bar &b); // #2
};

struct Bar {
   // ...
public:
   friend void doStuff(const Foo &f, const Bar &b); // #3
};

Then I would consider (1) to be the point of documentation, and (2), (3) should not be documented as function declarations, but similarly to a friend class declaration, optimally with doStuff linked to the actual function.

On the other hand, if we have:

// doStuff not declared yet

struct Foo {
   // ...
public:
   friend void doStuff(const Foo &f); // #1
};

// ... optionally
void doStuff(const Foo &f) { /* ... */ } // #2

(E.g., think of doStuff a operator<< or another operator.)
If (2) is not there, then doStuff is only available via ADL, so I see it documentation-wise essentially a part of Foo, and I would consider the point of documentation to be (1).
If (2) is there, then the function can be called via qualified lookup, so I think it depends on the specific case whether (1) or (2) should be considered the point of documentation.

For the initial example with reinterpret_borrow and reinterpret_steal it could look like (2) has the documentation. I think #623 in a sense made it possible to use (1) for documentation.

The mwe looks different, due to the namespace around the second function declaration, so the two bar can not refer to the same functions.

Some issues and thoughts:

  • The C++ domain does not directly have syntax for friend declarations that are not main declarations. For classes I don't think this is too big an issue, but for functions it may be nice to use the alias declaration somehow to render friend declarations.
  • Is it possible via the Doxygen XML to distinguish between the cases above?
  • Unable to resolve friend function and implementation #627 (comment) indicates there may be some additional lookup issues with doxygenfunction. It feels icky that it can find bar in both Foo and mwe without qualification. @YannickJadoul, can you make a small example with the wrapped namespace you mention? Just to be sure we are aligned in what that example would be and the issues it reveals.

@YannickJadoul
Copy link
Author

  • @YannickJadoul, can you make a small example with the wrapped namespace you mention? Just to be sure we are aligned in what that example would be and the issues it reveals.

Sure, definitely! This fails in the same way (i.e., Unable to resolve function "bar" with arguments None in doxygen xml output for project "mwe" from directory: _build/doxygenxml/.). I hadn't really realized, but I agree that the lack of mwe in the Foo::bar declaration is fishy as well!

namespace mwe {

class Foo {
    friend void bar();
};

/// Wenn ist das Nunstück git und Slotermeyer? Ja! Beiherhund das Oder die Flipperwaldt gersput!
void bar() {}

}

The reason I added the namespace in the first place is because of another error without it. Just having

void bar() {}

gives

/home/yannick/breathe_mwe/index.rst:1: WARNING: doxygenfunction: Cannot find function "bar" in doxygen xml output for project "mwe" from directory: _build/doxygenxml/

And with the Foo class and friend function but without namespace, it gets even weirder:

/home/yannick/breathe_mwe/index.rst:1: WARNING: Error when parsing function declaration.
If the function has no return type:
  Error in declarator or parameters-and-qualifiers
  Invalid C++ declaration: Expected identifier in nested name, got keyword: friend [error at 13]
    friend friend void Foo::bar ()
    -------------^
If the function has a return type:
  Invalid C++ declaration: Expected identifier in nested name, got keyword: friend [error at 13]
    friend friend void Foo::bar ()
    -------------^

I'll attach this here: mwe_no_namespace.zip

@YannickJadoul
Copy link
Author

I appreciate this is a hairy problem, by the way! No rush from my part, as I've work around it with a well-placed DOXYGEN_SHOULD_SKIP_THIS, but I thought I should report this.

If there's anything I can do, do let me know!

@vermeeren vermeeren added bug Problem in existing code code Source code labels Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Problem in existing code code Source code
Projects
None yet
Development

No branches or pull requests

3 participants