[clang] False negative clang-diagnostic-unused-variable for containers

[chrchr-github]

#include <vector>

struct S { int a, b; };
void f() {
    int i{}; // warning
    S s{}; // warning
    std::vector<int> vi; // FN
    std::vector<S> vs; // FN
}

https://godbolt.org/z/95xxG4exv

[firewave]

I once read about the reasoning why this isn't being reported but that was ages ago so it will be impossible to find.

This applies to the bugprone-unused-local-non-trivial-variable clang-tidy warning instead of the Clang warnings.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (chrchr-github)

~~~c++ #include <vector> struct S { int a, b; }; void f() { int i{}; // warning S s{}; // warning std::vector<int> vi; // FN std::vector<S> vs; // FN } ~~~ https://godbolt.org/z/95xxG4exv

[carlosgalvezp]

Yes, the warning is not active on non-trivial classes, whose constructors can have side effects (e.g. allocate heap memory). Consider std::lock_guard, it should not be warned about.

[chrchr-github]

A default-constructed STL container will not allocate any memory. Besides, "memory allocation" is likely not a desired side effect, as opposed to the synchronization provided by std::lock_guard.

[AaronBallman]

We typically don't want checks to have "special cases" though, because that doesn't scale and it comes across as mysterious. e.g., if the diagnostic is triggered for std::string s{}; why is it not triggered for my_awesome_class_with_a_non_trivial_ctor s{};?

Because of the importance of RAII as an idiom in C++, I think we have to err on the side of false negatives otherwise there will be a lot of false positives for users to silence.

[PiotrZSL]

There is check bugprone-unused-local-non-trivial-variable, you can configure it to detect unused containers. Just set IncludeTypes to .*

[chrchr-github]

There is check bugprone-unused-local-non-trivial-variable, you can configure it to detect unused containers. Just set IncludeTypes to .*

But that will then not check for side effects at all, right?

I would think that "STL containers of types with no side-effects" is a class worth special-casing, but ymmv.

[AaronBallman]

I would think that "STL containers of types with no side-effects" is a class worth special-casing, but ymmv.

Why containers and not, say, iterators? Or regular expressions? Or complex numbers? If we're special casing things, we'd want to special case the entirety of the STL otherwise it's going to be a game of whack-a-mole with more reports of false negatives in other circumstances. But there are about 3k classes in libc++ (including implementation detail classes) according to https://sourcegraph.com/search?q=context:global+repo:github.com/llvm/llvm-project+file:libcxx/include+%5E%28class%7Cstruct%29%5Cs%2B%5Cw%2B&patternType=regexp&sm=0 so that's what I meant about not scaling well; we'd have to analyze each class to see whether it's on the special case list or not, and that's ignoring the fact that STL implementations may be configured such that they CAN produce side effects sometimes (consider a hardened STL that checks constructor invariants).

Checking whether the constructor actually has side effects or not is an option, but requires a much slower analysis and will still have false negatives. Consider these as examples:

struct S {
  S() {  extern void foo(); foo(); }
};

struct T {
  T(bool b) { if (b) { puts("side effect"); } }
};

In the first case (S), we have no way to know what foo() does because we can't see its definition (clang-tidy operates on individual TUs, not the entire program), so we'd have to silence the unused variable diagnostic in that case. In the second case (T), we have to do control flow analysis to determine whether the object is constructed in a way that can lead to a side effect, which makes this check significantly slower to run, especially if you consider needing to chase your way through multiple levels of functions called via the constructor. That sort of analysis should be handled by the static analyzer and not by clang-tidy.

[chrchr-github]

To explain where I'm coming from, we had an unused container variable that was reported neither by cppcheck (due to a false negative) nor by clang-tidy, and I was surprised to learn that clang(-tidy) doesn't report unused containers at all. If you decide to close this as WONTFIX, that's OK with me.

Checking whether the constructor actually has side effects or not is an option, but requires a much slower analysis and will still have false negatives. Consider these as examples:

That kind of check must already exist somewhere though: https://godbolt.org/z/jG4jjbTof

[AaronBallman]

To explain where I'm coming from

Just to be clear -- I agree with you that it would be ideal to give true positives for these cases. I was saying that it's not practical for that to happen in clang-tidy, nor is it a bug (we intentionally wrote the check this way).

That kind of check must already exist somewhere though:

Clang produces unused variable diagnostics for trivial types (https://en.cppreference.com/w/cpp/named_req/TrivialType); int is a trivial type, but neither S nor T are trivial, which is why Clang doesn't diagnose those.