Fix compile error when compiling boost graphs with recent clang

[saraedum]

this fixes an underlying type confusion in our wrapper that has lead to warnings before but now produces an actual error, see #39249

[saraedum]

This is not necessary to fix the warning. But since these fields are not part of the Cython interface, they can as well be private (would have helped me to understand things more quickly.

[github-actions]

Documentation preview for this PR (built with commit 395ca33; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[user202729]

I don't think this change is necessary. I think you got confused when the programmer lies to cython or something

https://cython-guidelines.readthedocs.io/en/latest/articles/lie.html

similar to #39098 (comment)

In other words: even though in the pxd the declaration is

    ctypedef unsigned long v_index
    ctypedef unsigned long e_index

in compiled code it should still use int. (before the change)

Or so I think.

[saraedum]

I don't understand why the programmer would want to lie to Cython here? So, e_index should be the type that num_edges returns. That's boost::graph_traits<G>::edges_size_type. I don't think I can reference that type directly from a pxd (but maybe there is a way, then I should probably use it.) Let me put a static assert into the code to check that this is actually unsigned long. Similarly, v_index is vertex_index_t and size_t and vertices_size_type. I guess they are all just unsigned long but I'll add a static assert to be sure.
This is not terribly clean. If we end up building on a platform where this is not true, then we might have to stop identifying these types but I think it's just the same type on all platforms anyway.

Maybe I'll put in the extra work and actually separate these types but I think in Cython I'll just set them all to unsigned long.

I am not sure I really understood your objection here though. Maybe you can rephrase what you had in mind?

[saraedum]

I see now what you meant with the v_index. It's the type used in the definition of adjacency_list. So if I change it to an unsigned type, this actually changes something.

[user202729]

Anyway I think the problem is why is in the original code

    typedef typename boost::property_map<adjacency_list, boost::vertex_index_t>::type vertex_to_int_map;

the boost::vertex_index_t is not a v_index. And more importantly why is v_index different from boost::vertex_index_t? (1)

tl;dr while the two places changing int to v_index look good, I don't think the fix is semantically correct. Though I haven't got the time to fully understand the author's intention yet (see (1) above)

[saraedum]

Thanks for your thoughts, I'll try to make the code be a bit more careful with the types.

[saraedum]

Anyway I think the problem is why is in the original code

    typedef typename boost::property_map<adjacency_list, boost::vertex_index_t>::type vertex_to_int_map;

the boost::vertex_index_t is not a v_index. And more importantly why is v_index different from boost::vertex_index_t? (1)

I think boost::vertex_index_t is just used to single out a "slot" of the boost::property. It's not a type that is meant to be used for values I'd say.

[user202729]

Okay, maybe I misunderstood.

But anyway, the "morally correct" typedef is to make v_index being type-def-ed to whichever type that

    typedef typename boost::property_map<adjacency_list, boost::vertex_index_t>::type vertex_to_int_map;
    vertex_to_int_map index;
index[boost::source(*ei, graph)]

belong to, right? (which is typename vertex_to_int_map::value_type, but I can't find where it's defined yet) Why is it unsigned long?

Or is that too much of a breaking API change and we should just cast it to v_index type instead?

---

Edit: because of

    typedef typename boost::adjacency_list<
        OutEdgeListS, VertexListS, DirectedS,
        boost::property<boost::vertex_index_t, v_index>,  # ⟵ this line
        EdgeProperty, boost::no_property, EdgeListS> adjacency_list;

then typename vertex_to_int_map::value_type is supposed to be v_index. But why is it not?

Edit: turns out it's because VertexListS is vecS or something else.

#include <boost/graph/adjacency_list.hpp>
#include <boost/property_map/property_map.hpp>
#include <boost/type_traits/is_same.hpp>
#include <type_traits>
#include <vector>

// type aliases remain unchanged
typedef int v_index;
typedef long e_index;

typedef boost::adjacency_list<
    boost::vecS,              // OutEdgeList
    boost::vecS,              // VertexList
    boost::undirectedS,       // DirectedS
    boost::property<boost::vertex_index_t, v_index>, // Vertex property.
    boost::no_property,       // Edge property (can be enhanced if needed).
    boost::no_property,       // Graph property.
    boost::vecS               // EdgeList
> Graph;

typedef boost::property_map<Graph, boost::vertex_index_t>::type vertex_to_int_map;

static_assert(std::is_same_v<typename vertex_to_int_map::value_type, v_index>,
              "The vertex property map's value type must be v_index");

when vecS is changed to listS then the static_assert passes.

Not sure if this is intended. I decide to open an issue on boost graph boostorg/graph#420 to see whether it is.

[saraedum]

I think it makes sense to split this into two separate issues. One that just fixes the compile error but keeps the status quo. And a second one that fixes all the confusion with the types that seems to be happening in the three .cpp .pxd and .pyx files that are involved here.
I don't think I have the bandwidth to work on the latter right now. But I'll create an issue to track this and try to push a fix for the first problem here.

[tobiasdiez]

At https://github.com/sagemath/sage/pull/38872/files#diff-5787682a0554d08f4dc34e60e792af0bbf764ecae08c8c941865e2573edf50f0, I have a more "minimal" fix. I'm lacking the background to see which one is "better".

            v_index source = index[boost::source(*ei, graph)];
            v_index target = index[boost::target(*ei, graph)];
            double weight = boost::get(boost::edge_weight, graph, *ei);
            to_return.push_back({source, {target, weight}});

[user202729]

just fixes the compile error but keeps the status quo

I think we can just do this (which means performing an explicit casting at the use site, possibly adding a note explaining why the casting is needed)

(which is what the more minimal fix does.)

[tobiasdiez]

Setting this to CI Fix to resolve the macos build errors on macos.