[analyzer] False positive "moved-from object is moved" inside map

[jh7370]

I've got a bit of code which iterates over a map. If the key is a specific value, it will move a variable as part of constructing another object. As this is iterating over a constant map (ignoring for a moment unsafe updates of the map in another thread), this specific case can only be hit once, because each key in a map can only be seen once. However, clang produces a "Moved-from object ... is moved" warning for this line, which is therefore a false positive.

I've tested with a recent main (but not head) as well as 19.0, using clang-cl --analyze.

Reproducible:

#include <unordered_map>

struct Foo {};

void func(const std::unordered_map<int, int> &m)
{
    Foo f;

    for (const auto &[key, value] : m)
    {
        if (key == 1)
        {
            Foo f2(std::move(f));
        }
    }
}

Output:

C:\telemetry\telemetry-tooling\telemetry-tooling-api>clang-cl.exe --analyze C:\Work\TempWork\StaticAnalysis.cpp
C:\Work\TempWork\StaticAnalysis.cpp(13,17): warning: Moved-from object 'f'
      is moved [cplusplus.Move]
   13 |             Foo f2(std::move(f));
      |                 ^~~~~~~~~~~~~~~~
1 warning generated.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: James Henderson (jh7370)

I've got a bit of code which iterates over a map. If the key is a specific value, it will move a variable as part of constructing another object. As this is iterating over a constant map (ignoring for a moment unsafe updates of the map in another thread), this specific case can only be hit once, because each key in a map can only be seen once. However, clang produces a "Moved-from object ... is moved" warning for this line, which is therefore a false positive.

I've tested with a recent main (but not head) as well as 19.0, using clang-cl --analyze.

Reproducible:

#include <unordered_map>

struct Foo {};

void func(const std::unordered_map<int, int> &m)
{
    Foo f;

    for (const auto &[key, value] : m)
    {
        if (key == 1)
        {
            Foo f2(std::move(f));
        }
    }
}

Output:

C:\telemetry\telemetry-tooling\telemetry-tooling-api>clang-cl.exe --analyze C:\Work\TempWork\StaticAnalysis.cpp
C:\Work\TempWork\StaticAnalysis.cpp(13,17): warning: Moved-from object 'f'
      is moved [cplusplus.Move]
   13 |             Foo f2(std::move(f));
      |                 ^~~~~~~~~~~~~~~~
1 warning generated.

[chrchr-github]

Clang-tidys's bugprone-use-after-move also has a false positive on this code:
https://godbolt.org/z/4z7sPr59a

[steakhal]

Thank you for taking your time and report this.
Do you have a real case for this code pattern? By looking at the example you shared, it feels oddly specific.
Tools could know the semantics of standard containers, but in general, all of those needs engineering, thus one needs to judge the ROI.
Looking at the example, I'm tempted to just close this.

I think what confuses the analyzer is the loop and the container invariant that a key can only appear once.
So if you could achieve the same with std algorithms instead of a raw loop, probably the FP would go away.
Same should happen if the code would no longer exploit the container invariant.

[jh7370]

The actual code is a fair bit more complex than what I posted, but it's proprietary, so I can't share it exactly and I've tried to boil it down to a minimal reproducer. In the real code, the moved-from object is passed into the example function (as an r-value reference, not that I think it matters that much), rather than being a local object, then passed into another function, which ultimately takes ownership of the object (via move construction).

Our map object is a representation of a dictionary, which is nested inside another dictionary, to give us a series of named maps ultimately. There is one instance of the moved-from object per inner object, and it is pre-populated with data from earlier. One of the fields in the inner dictionaries contains references to other objects contained elsewhere in the outer dictionary and we need to essentially copy the second object's members into the first. The moved object is used as part of that process.

I could of course use std::for_each rather than the range-based for loop, but fundamentally that's the same as a range-based for loop, just with wrapping the loop body in a lambda and having to call begin/end to pass it all in, which all feels suboptimal. It does silence the false positive though.

For the record, the Visual Studio static analyzer also runs into the same issue with near-identical code. See https://developercommunity.visualstudio.com/t/False-positive-use-after-move-code-ana/10990268.