fix: fully qualify usages of concat to protect against ADL #4955
Conversation
|
That looks like an easy fix, but
I believe depending on what is used, any of those could run into a conflict with modernjson. Which leads me to think we should insert the But I also briefly looked at the modernjson code: I see all but one |
|
I wanted to be sure that this is actually something worthwhile to add before making the changes everywhere. I can of course add the prefix to all occurrences of I did not discuss this issue yet in the modernjson project. It could indeed be an option to add the |
I think that would be best. I'd be OK to add the prefixes here, too, but what is likely to happen over time is that new code is added here without the prefix, and then there are probably hundreds of custom |
|
Normally functions in different namespaces ( Also, I'd fully qualify them, by the way, with |
|
Do you have a little more of the error message? |
+1 (I just looked, we have many
Good question. Initially I thought it's probably through argument dependent lookup, but at second look, that doesn't really make sense. |
|
@volkm This is an OK change but also very adhoc. Is this actually still needed? Did you get to understand these two questions? From me (Nov 29):
From @henryiii (also Nov 29):
|
|
I don't think this hurts, it's just general cleanup that happens to also (maybe) help someone? We should be qualifying our usage. |
By that token we'd have to qualify everything. I think it's better to not merge this PR unless
|
|
We already qualify some other things. If it makes sense to qualify it, why not go ahead and qualify it? We don't call some random concat (as in the description), but only ours? |
|
(I'd also be okay with more qualified names too, unqualified names really should only be used when it's fine for a user to inject their own code, and we don't support that with concat) |
My thinking: Fully qualifying only makes sense in macros. Or if there is a well-understood name lookup issue. Other than that I look at Also: Either do this systematically, or not at all. Making a point change for one random snowflake seems odd, especially since we don't even know if it's actually still needed, or why. |
|
Okay, I'll give up until more can be understood. @volkm is patching pybind11 at https://github.com/moves-rwth/stormpy/tree/6c983bc9395646ccbe04cbf7238d6f5aeb922946/resources, so I'd say the answer to "is it needed" is yes. Personally, I'd be fine to add |
|
@volkm I really don't see how this could be leaking, and I'd really like to know how it does leak. Do you have the full error message, not just the snipped part with pybind11 in it? Is there a simple way to build stormpy / storm? |
|
This would also fix #5072, FYI. |
And passes with the patch. :) |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
There was a problem hiding this comment.
The test only covers one of the two concat in pybind11/cast.h (I tried it out locally).
That led me to look systematically. I think we need to add more prefixes, see below.
My guess: We need to add prefixes for all concat(make_caster<>::name) but not the rest.
$ grep -R 'concat(' | grep -v '^detail/descr.h:'
cast.h: = const_name("tuple[") + concat(make_caster<Ts>::name...) + const_name("]");
cast.h: static constexpr auto arg_names = concat(type_descr(make_caster<Args>::name)...);
numpy.h: concat(const_name<N>(), array_info<T>::extents), const_name<N>());
eigen/tensor.h: static constexpr auto value = concat(const_name(((void) Is, "?"))...);
eigen/tensor.h: static constexpr auto dimensions_descriptor = concat(const_name<Indices>()...);
functional.h: const_name("Callable[[") + concat(make_caster<Args>::name...)
stl.h: const_name("Union[") + detail::concat(make_caster<Ts>::name...)
typing.h: = const_name("tuple[") + concat(make_caster<Types>::name...) + const_name("]");
typing.h: static constexpr auto name = const_name("Callable[[") + concat(make_caster<Args>::name...)```
|
I wonder if there is a more robust and general solution, so that we don't have to remember to add prefixes when writing new code. I found this: https://indii.org/blog/avoiding-argument-dependent-lookup-in-cpp/ I guess the "culprit type" in our case is |
|
When you know the namespace and don't want ADL, the general solution is to fully qualify it. Tricks are out there for the other direction, but if you know the namespace, just qualify it. It's simpler than tricks and easy on the compiler. |
|
What I was hoping for is a setup that doesn't leave us playing a whack-a-mole game (new code missing the prefix). But I'm fine if we just get to a consistent state at this moment, by systematically adding the prefix for all functional.h, stl.h, typing.h |
|
I'm okay to add those, though I wasn't able to get them to trigger easily. I suspect a few other of our functions are susceptible, but |
I think so, too, but there is only so much time in a day. :-/
Yeah... I was afraid that could be the case. On balance, pollution vs precaution, systematically prefixing |
|
Ah, the reason the My test code for reference: m.def("_adl_issue_tuple", [](const std::tuple<const ADL_issue::test> &) {}); |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
henryiii
changed the title
Description
Replaced calls to
concatby using complete namespacepybind11::detail::concatin filecast.h.We encountered issues in our library, because the
concatfunction from modernjson was erroneously used instead of the correct one from pybind11:Adding the namespace in pybind fixed this issue.
See this issue for details.
Suggested changelog entry:
* Qualify ``py::detail::concat`` usage to avoid ADL selecting one from somewhere else, such as modernjson's concat.