IWYU incorrectly suggests <variant> for std::hash, as observed in #33922.
On Ubuntu with include-what-you-use 0.26 based clang version 22.1.1,
it incorrectly suggested <string_view>.
On macOS with IWYU 0.26 based on clang version 22.1.2, it correctly suggests <functional>.
Fix this by adding the mapping.
Tracked upstream: https://github.com/include-what-you-use/include-what-you-use/issues/2007
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | maflcko |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Can you link to the relevant upstream issue (or open one).
@fanquake I can't find an upstream issue for this specific case. IIUC https://github.com/include-what-you-use/include-what-you-use/issues/785 would prevent this in the most general way?
4 | @@ -5,4 +5,6 @@ 5 | { "include": [ "<mmintrin.h>", "private", "<immintrin.h>", "public" ] }, 6 | { "include": [ "<smmintrin.h>", "private", "<immintrin.h>", "public" ] }, 7 | { "include": [ "<tmmintrin.h>", "private", "<immintrin.h>", "public" ] }, 8 | + 9 | + { "symbol": ["std::hash", "private", "<functional>", "public"] },
Seems fine, but this is equally wrong.
Any header is fine, see:
https://en.cppreference.com/w/cpp/utility/hash.html
I think iwyu should just tolerate any of them
How do I specify "any header"?
Not sure if this is possible without fixing up iwyu.
In any case, I wanted to say that I don't see the point of this pull request. What is the exact harm you are trying to fix? What is the risk?
Right now, all you are doing is swapping one perfectly fine thing (for the compiler) with another one. Both are equally confusing and "wrong" for a reader, so I don't see the point for this churn here. Why does it need to change?
In the context of #33922, we can simply redeclare the primary template:
namespace std {
// Redeclaration of the primary template.
template <class T>
struct hash; // IWYU pragma: keep
/** Disable default std::hash for CTransactionRef to prevent accidentally
* comparing by pointer. Use CTransactionRefSaltedHash or provide a custom
* hasher. */
template <>
struct hash<CTransactionRef> {
size_t operator()(const CTransactionRef&) const = delete;
};
} // namespace std
What is the exact harm you are trying to fix?
CI spuriously fails for #33922.
Given how iwyu works, it is a harmless and arguably valid suggestion. Recall that the goal of using iwyu is to avoid missing includes, which have lead to compile failures in the past. Adding an include that already exists implicitly is harmless. And fwiw, CI did pass on commit 10acf818ed70d4cd2af864278a35a6b64de0bb16.
Working around that in the wrong way will just achieve the opposite. E.g. a no_include pragma may lead to the exact compile failures we are trying to avoid, down the line.
This might be a clang vs libc++ thing, because I can't reproduce the CI failure locally: include-what-you-use 0.26 based on clang version 22.1.2
I was able to reproduce a similar, but not identical, result on Ubuntu (see PR description), where it suggested <string_view> instead of <variant>. Since I can't reproduce the issue on macOS, the issue seems specific to libstdc++.
IWYU incorrectly suggested <variant> for std::hash in #33922.
On Ubuntu with include-what-you-use 0.26 based clang version 22.1.1,
it incorrectly suggested <string_view>.
On macOS with IWYU 0.26 based on clang version 22.1.2, it correctly
suggests <functional>.
Fix this by adding the mapping.
(accidentally pushed part of #33922, it should be good for review now)
Tracking upstream: https://github.com/include-what-you-use/include-what-you-use/issues/2007
4 | @@ -5,4 +5,10 @@ 5 | { "include": [ "<mmintrin.h>", "private", "<immintrin.h>", "public" ] }, 6 | { "include": [ "<smmintrin.h>", "private", "<immintrin.h>", "public" ] }, 7 | { "include": [ "<tmmintrin.h>", "private", "<immintrin.h>", "public" ] }, 8 | + 9 | + # IWYU's std_symbol_map.inc maps std::hash<T> specializations to their 10 | + # correct headers, but with libstdc++ IWYU still suggests a wrong
Can we use libc++, if libstdc++ yields the wrong result?
Not sure if it's worth switching based on this N=1 example.
To give some background why I suggested it (but I haven't confirmed any of this): libc++ has the option -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES. Further, assuming that the "wrong" header was just picked because it was the first one present anyway (ref https://github.com/include-what-you-use/include-what-you-use/issues/2007#issuecomment-4259912191), it seems that D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES may improve the end result minimally. Though, I haven't checked any of this.
cc @hebasto
Tracking upstream: include-what-you-use/include-what-you-use#2007
Left some comments in https://github.com/include-what-you-use/include-what-you-use/issues/2007.
@hebasto so should I use
<memory>instead of<functional>? Or is there just no way to get a universal rule and do we somehow disable the linter for this case?
I would suggest the following approach in #33922:
// IWYU erroneously suggests including <variant> instead of <memory>
// for std::hash<CTransactionRef>.
// See https://github.com/include-what-you-use/include-what-you-use/issues/2007.
// IWYU pragma: no_include <variant>
// IWYU pragma: no_include <variant>
I disagree. This patch would be harmful. If std::variant is used any time in the future, it would be missing the include and lead to compile failures.
As per my last comment (https://github.com/bitcoin/bitcoin/pull/35073#discussion_r3091132596), I don't understand why this pull requests exists and what it is trying to achieve. There is no real bug and any attempted fix is just going to add more harm and risks.
NACK
What exactly is the root problem here? Is it that IWYU does not recognize std::hash<CTransactionRef> as a template specialization for std::shared_ptr and therefore does not map it to <memory>? Would it help to add a mapping for that type?
{ "symbol": ["std::hash<CTransactionRef>", "private", "<memory>", "public"] },
What exactly is the root problem here? Is it that IWYU does not recognize
std::hash<CTransactionRef>as a template specialization forstd::shared_ptrand therefore does not map it to<memory>? Would it help to add a mapping for that type?{ "symbol": ["std::hash<CTransactionRef>", "private", "<memory>", "public"] },
See https://github.com/include-what-you-use/include-what-you-use/issues/2007#issuecomment-4259912191.
@hebasto's workaround from #35073 (review) doesn't work unfortunately, see (iwyu ci log).
If I keep the <functional> include, it wants it removed. When I remove it, demands <variant> again.
Happy to close this is it's incorrect, but I don't think we should be enforcing buggy linters project-wide. This sort of spurious CI failure is impossible to debug without deep compiler / build system knowledge.
redeclare the primary template: this doesn't work, per my previous comment
You pushed the wrong thing. You can use the copy-paste icon in the upper right on GitHub to properly copy-paste it.
@maflcko for (1) I pushed this:
template <class T>
struct hash; // IWYU pragma: keep
Which is from: #35073 (review)
Here's the commit: https://github.com/bitcoin/bitcoin/commit/4673836a656a2638292fd02453e5d8ef42cd8eab
Here's the CI failure: https://github.com/bitcoin/bitcoin/actions/runs/24566460651/job/71827949758
That didn't work either, but b89942d29f1e71429070b2d1140d2792eb33bf47 does.