False positives in clang-tidy performance-unnecessary-copy-initialization

I've noticed that clang-tidy performance-unnecessary-copy-initialization can give misleading recommendations:

void modifyAnything();
ExpensiveToCopyType copyFromReference(const ExpensiveToCopyType &ref) {
  ExpensiveToCopyType tmp = ref;

                       ^
warning: local copy 'tmp' of the variable 'ref' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]

  modifyAnything();
  return tmp;
}

If you actually do that change, it can break the code, as the object referenced by ref may get modified during the call to modifyAnything(). The above is a stripped-down version of a bug introduced in LibreOffice when such a clang-tidy recommendation had been taken at face value, see <https://cgit.freedesktop.org/libreoffice/core/commit/?id=36a329b6395257d7df2013d23ba4205a5ef72f4d&gt; "Fix regression in bubbleSortVersion".

Is it intentional that clang-tidy gives such a potentially problematic recommendation here (after all, it's phrased sufficiently vague, "consider...", implying that the code should be inspected further before blindly applying the fixit), or should that better be avoided?

(My blunt attempt at fixing it,

diff --git a/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 177497c..4718339 100644
--- a/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -132,7 +132,8 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
     const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
     bool IssueFix, ASTContext &Context) {
- if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+ if (OldVar.getType()->isReferenceType() ||
+ !isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
       !isOnlyUsedAsConst(OldVar, BlockStmt, Context))
     return;

diff --git a/test/clang-tidy/performance-unnecessary-copy-initialization.cpp b/test/clang-tidy/performance-unnecessary-copy-initialization.cpp
index 50dcfd8..8b29dae 100644
--- a/test/clang-tidy/performance-unnecessary-copy-initialization.cpp
+++ b/test/clang-tidy/performance-unnecessary-copy-initialization.cpp
@@ -387,3 +387,10 @@ void implicitVarFalsePositive() {
   for (const Element &E : Container()) {
   }
}
+
+void modifyAnything();
+ExpensiveToCopyType copyFromReference(const ExpensiveToCopyType &ref) {
+ ExpensiveToCopyType tmp = ref;
+ modifyAnything();
+ return tmp;
+}

would cause test/clang-tidy/performance-unnecessary-copy-initialization.cpp's

void PositiveLocalCopyCopyIsArgument(const ExpensiveToCopyType &orig) {
  ExpensiveToCopyType copy_3 = orig;
  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_3'
  // CHECK-FIXES: const ExpensiveToCopyType& copy_3 = orig;
  copy_3.constMethod();
}

to no longer emit the warning.)

That's a strange way to perform a swap.

There's no warning for this:
rtl::Reference<VendorBase> less = next;
next = cur;
cur = less;

Or it could just use std::swap(cur, next).

I'll let Alex comment on how the check should be fixed.

The
above is a stripped-down version of a bug introduced in LibreOffice when
such a clang-tidy recommendation had been taken at face value, see
<https://cgit.freedesktop.org/libreoffice/core/commit/?id=36a329b6395257d7df2013d23ba4205a5ef72f4d&gt;
"Fix regression in bubbleSortVersion".

That's a strange way to perform a swap.

[...]

Or it could just use std::swap(cur, next).

Both true, for sure; but not relevant here.

> There's no warning for this:
> rtl::Reference<VendorBase> less = next;
> next = cur;
> cur = less;

...because UnnecessaryCopyInitialization now sees that !isOnlyUsedAsConst(OldVar,...)