[cfe-dev] False positives in clang-tidy performance-unnecessary-copy-initialization

[Stephan Bergmann via cfe-dev]

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> 
"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.)