performance-unnecessary-value-param false positive with absl::flat_hash_map::emplace

[llvmbot]

Bugzilla Link	51239
Version	unspecified
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@JonasToth,@fberger

Extended Description

The following code generates a false positive with the performance-unnecessary-value-param clang-tidy rule:

#include <absl/container/flat_hash_map.h>

#include <memory>

class Foo {
 public:
  Foo(std::shared_ptr<int> ptr) { m_map.emplace(0, std::move(ptr)); }

 private:
  absl::flat_hash_map<int, std::shared_ptr<int> > m_map;
};

clang-tidy claims:

<source>:9:28: warning: the parameter 'ptr' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
  Foo(std::shared_ptr<int> ptr) { m_map.emplace(0, std::move(ptr)); }
                           ^
      const               &
1 warning generated.

Looking at the generated assembly, I understand that actually emplace is instantiated with two rvalue arguments, so this doesn't appear to be accurate.

See https://godbolt.org/z/hKb4qvGzP for the comparison with std::unordered_map and the clang-tidy output.

[sfc-gh-bhannel]

I dug deeper into this issue and it appears to be due to passing the rvalue reference via a tuple. Here is a minimal reproduction without absl: https://godbolt.org/z/8WGqM9Evr

The relevant snippet:

std::vector<TrackedType> storage;

template <class T>
void emplace(T&& arg) {
  std::tuple<T&&> tup(std::forward<T>(arg));
  storage.emplace_back(std::move(std::get<0>(tup)));
}

void byValue(TrackedType value) {
    emplace(std::move(value));
}

clang-tidy 17 says that byValue should pass by reference, when in fact that would add another copy. The clang-tidy analysis seems to not understand that the semantics of std::tuple<T&&> tup(std::forward<T>(arg)); change for the worse when arg is a const reference rather than rvalue reference.

It would be really great to fix this. The incompatibility with absl::flat_hash_map makes it impractical to use this otherwise very useful flag on my repo.

[sfc-gh-bhannel]

Looking through the implementation, a parameter is considered to be mutated if it is casted to a mutable reference type: https://github.com/llvm/llvm-project/blob/384f916ea899ea6ac9af4a3fb9d0a5b03937acfe/clang/lib/Analysis/ExprMutationAnalyzer.cpp#L431

I see when I perform a cast to T&& or T& explicitly, then we don't get this false positive: https://godbolt.org/z/vMax894fE

However, std::move, despite being implemented as a static cast to rvalue reference, is special cased: https://github.com/llvm/llvm-project/blob/384f916ea899ea6ac9af4a3fb9d0a5b03937acfe/clang/lib/Analysis/ExprMutationAnalyzer.cpp#L458

The special case handling doesn't seem to work correctly, though I don't yet understand why.