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

Regression with class template specialization matching in Doxygen 1.9.1 #251

Open
cz4rs opened this issue Oct 29, 2024 · 4 comments
Open

Comments

@cz4rs
Copy link

cz4rs commented Oct 29, 2024

Following assertion triggers in recent builds of DARMA-tasking/vt when using the latest version of m.css (699abdd):

assert compound.name.startswith(prefix)

Traceback with additional details:

diff --git a/documentation/doxygen.py b/documentation/doxygen.py
index d3ef69eb..ae214cc6 100755
--- a/documentation/doxygen.py
+++ b/documentation/doxygen.py
@@ -2597,7 +2597,7 @@ def postprocess_state(state: State):
             prefix = state.compounds[compound.parent].name + '::'
-            assert compound.name.startswith(prefix)
+            assert compound.name.startswith(prefix), f"compound.name: {compound.name}\nprefix: {prefix}"
             compound.leaf_name = compound.name[len(prefix):]
Traceback (most recent call last):
  File "/build/vt/./m.css/documentation/doxygen.py", line 4305, in <module>
    run(state, templates=os.path.abspath(args.templates), wildcard=args.wildcard, index_pages=args.index_pages, search_merge_subtrees=not args.search_no_subtree_merging, search_add_lookahead_barriers=not args.search_no_lookahead_barriers, search_merge_prefixes=not args.search_no_prefix_merging)
  File "/build/vt/./m.css/documentation/doxygen.py", line 4128, in run
    postprocess_state(state)
  File "/build/vt/./m.css/documentation/doxygen.py", line 2600, in postprocess_state
    assert compound.name.startswith(prefix), f"compound.name: {compound.name}\nprefix: {prefix}"
AssertionError: compound.name: vt::rdma::Handle&lt;T, E, IndexT, typename std::enable_if_t&lt;not std::is_same&lt;IndexT, vt::NodeType&gt;::value&gt;&gt;::IndexTagType
prefix: vt::rdma::Handle::

Proposed fix:

diff --git a/documentation/doxygen.py b/documentation/doxygen.py
index d3ef69eb..42378197 100755
--- a/documentation/doxygen.py
+++ b/documentation/doxygen.py
@@ -2597,7 +2597,7 @@ def postprocess_state(state: State):
             prefix = state.compounds[compound.parent].name + '::'
-            assert compound.name.startswith(prefix)
+            assert compound.name.startswith(state.compounds[compound.parent].name)
             compound.leaf_name = compound.name[len(prefix):]

This seems to be enough to get our build passing, not sure if any caveats apply.

@mosra
Copy link
Owner

mosra commented Nov 2, 2024

Hi, this actually seems like a regression in Doxygen 1.9.1 (or possibly other versions around that one). I managed to strip it down to just the following:

template<class T> struct Outer;

template<class T> struct Outer<
    std::enable_if_t<std::is_same<T, int>::value>
> {
    struct FirstInner {};
};

template<class T> struct Outer<
    std::enable_if_t<!std::is_same<T, int>::value>
> {
    struct SecondInner {};
};

On both 1.8.16 and 1.12, this produces a correct output, while on 1.9.1 this leads to the assertion you're encountering, and additionally Doxygen itself complains that

File.h:28: warning: Internal inconsistency: scope for class Outer< std::enable_if_t< std::is_same< T, int >::value >>::FirstInner not found!
File.h:35: warning: Internal inconsistency: scope for class Outer< std::enable_if_t<!std::is_same< T, int >::value >>::SecondInner not found!

Those warnings then lead to an unspecialized Outer base class being created, to which both FirstInner and SecondInner get inserted as members. Which is obviously incorrect. The same can be seen in your generated docs, screenshot for archival purposes:

image

Both the IndexTagType member from handle.index.h and the NodeTagType member from handle.node.h get listed, as if both specializations were a single class, and their docs get merged as well. The sanity check assertion is then just an inevitable result from this -- it gets a member listed in a scope in which it shouldn't be at all.

Your fix attempt then makes the build passing, yes, but the parent name doesn't get correctly stripped -- it should list just IndexTagType there, not <T, E, IndexT, typename std::enable_if_t<not std::is_same<IndexT, vt::NodeType>::value>>::IndexTagType, and the two types should be in two different specializations anyway. Not sure what to do here, besides expanding the assertion with some more reasonable message, or ignoring the type that got inserted to a wrong place -- there's not really anything it could do to "unbreak" the input.

Funnily enough, if I change my stripped-down case to the following (i.e., using the C++11-style enable_if, not C++17-style enable_if_t), it works on 1.9.1. So maybe that's what could be a workaround on your end, apart from trying to use some other Doxygen version?

template<class T> struct Outer;

template<class T> struct Outer<
    typename std::enable_if<std::is_same<T, int>::value>::type
> {
    struct FirstInner {};
};

template<class T> struct Outer<
    typename std::enable_if<!std::is_same<T, int>::value>::type
> {
    /** @brief Foo */
    struct SecondInner {};
};

@cz4rs
Copy link
Author

cz4rs commented Nov 4, 2024

Hi, this actually seems like a regression in Doxygen 1.9.1 (or possibly other versions around that one). I managed to strip it down to just the following (...)

Thanks a lot for the detailed (!) response. I was suspecting Doxygen to be the culprit, but I didn't have the patience to track it down properly.

On both 1.8.16 and 1.12, this produces a correct output, while on 1.9.1 this leads to the assertion you're encountering (...)

Alright, if it works with 1.12 then I guess there's no point in reporting this in Doxygen. I will try to adapt our documentation workflow to use the latest version (maybe the binary release will work, since Ubuntu is using 1.9.8).

@mosra
Copy link
Owner

mosra commented Nov 4, 2024

Please see #215 -- I personally had to revert back to 1.8.16 because it's far from great. On the m.css side all the workarounds for 1.12 are in place so it shouldn't wildly assert and such, but there are a few serious regressions for which I so far wasn't able to figure out a workaround nor a patch. I hope to get back to that before the maintainers publish a new release (which they often do during summer holidays or christmas), but it's an uphill battle. #239 might also affect you.

If the above-suggested workaround with typename std::enable_if works, then I think that's the best way forward. If not, then I'd suggest trying with 1.9.4, because 1.9.5+ forward contains the massive regressions listed in #215. Doxygen binaries from the official website should work as long as you aren't on a distro that's too old, IIRC 18.04 / 20.04 cannot run the new ones because a glibc mismatch. (This is what I do here on the CI.)

In any case, I'll add a better assertion message for this, just wanted to do some bisecting to know what all versions are affected.

@mosra mosra changed the title incorrect compound name assertion Regression with class template specialization matching in Doxygen 1.9.1 Nov 4, 2024
@cz4rs
Copy link
Author

cz4rs commented Nov 20, 2024

Please see #215 -- I personally had to revert back to 1.8.16 because it's far from great. On the m.css side all the workarounds for 1.12 are in place so it shouldn't wildly assert and such, but there are a few serious regressions for which I so far wasn't able to figure out a workaround nor a patch. I hope to get back to that before the maintainers publish a new release (which they often do during summer holidays or christmas), but it's an uphill battle. #239 might also affect you.

If the above-suggested workaround with typename std::enable_if works, then I think that's the best way forward. If not, then I'd suggest trying with 1.9.4, because 1.9.5+ forward contains the massive regressions listed in #215. Doxygen binaries from the official website should work as long as you aren't on a distro that's too old, IIRC 18.04 / 20.04 cannot run the new ones because a glibc mismatch. (This is what I do here on the CI.)

In any case, I'll add a better assertion message for this, just wanted to do some bisecting to know what all versions are affected.

Just FYI I have opted to downgrade Doxygen to 1.8.16 to keep in sync with the CI here. The generated docs seem to be correct now.
+1 for improving the assertion message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants