Clang-tidy invalid suggestion: `performance-unnecessary-copy-initialization`

[joker-eph]

With clang-tidy built at current head and applying it on MLIR:

$ clang-tidy -p build mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp --checks=-*,performance-unnecessary-copy-initialization -fix -fix-errors

Yield this diff:

diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
index 20f9333e7c95..1438266472f2 100644
--- a/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
+++ b/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
@@ -381,7 +381,7 @@ LogicalResult TosaProfileCompliance::checkProfileOrExtension(
 
   // Find the required profiles or extensions according to the operand type
   // combination.
-  const auto opRequiredMode = maybeOpRequiredMode.value();
+  const auto &&opRequiredMode = maybeOpRequiredMode.value();
   if (opRequiredMode.size() == 0) {
     // No matched restriction found.
     return success();

Fails to build with:

FAILED: tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /home/mamini/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ibuild/tools/mlir/lib/Dialect/Tosa/Transforms -Imlir/lib/Dialect/Tosa/Transforms -Ibuild/tools/mlir/include -Imlir/include -Ibuild/include -Illvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wundef -Werror=mismatched-tags -Werror=global-constructors -O3 -DNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o -MF tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o.d -o tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o -c mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:384:16: error: rvalue reference to type 'const SmallVector<...>' cannot bind to lvalue of type 'const SmallVector<...>'
  384 |   const auto &&opRequiredMode = maybeOpRequiredMode.value();
      |                ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:447:12: note: in instantiation of function template specialization 'TosaProfileCompliance::checkProfileOrExtension<mlir::tosa::Profile>' requested here
  447 |     return checkProfileOrExtension<Profile>(op, targetEnv,
      |            ^
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:384:16: error: rvalue reference to type 'const SmallVector<...>' cannot bind to lvalue of type 'const SmallVector<...>'
  384 |   const auto &&opRequiredMode = maybeOpRequiredMode.value();
      |                ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:457:12: note: in instantiation of function template specialization 'TosaProfileCompliance::checkProfileOrExtension<mlir::tosa::Extension>' requested here
  457 |     return checkProfileOrExtension<Extension>(op, targetEnv,
      |            ^
2 errors generated.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Mehdi Amini (joker-eph)

With clang-tidy built at current head and applying it on MLIR:

$ clang-tidy -p build mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp --checks=-*,performance-unnecessary-copy-initialization -fix -fix-errors

Yield this diff:

diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
index 20f9333e7c95..1438266472f2 100644
--- a/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
+++ b/mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
@@ -381,7 +381,7 @@ LogicalResult TosaProfileCompliance::checkProfileOrExtension(
 
   // Find the required profiles or extensions according to the operand type
   // combination.
-  const auto opRequiredMode = maybeOpRequiredMode.value();
+  const auto &&opRequiredMode = maybeOpRequiredMode.value();
   if (opRequiredMode.size() == 0) {
     // No matched restriction found.
     return success();

Fails to build with:

FAILED: tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /home/mamini/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ibuild/tools/mlir/lib/Dialect/Tosa/Transforms -Imlir/lib/Dialect/Tosa/Transforms -Ibuild/tools/mlir/include -Imlir/include -Ibuild/include -Illvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wundef -Werror=mismatched-tags -Werror=global-constructors -O3 -DNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o -MF tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o.d -o tools/mlir/lib/Dialect/Tosa/Transforms/CMakeFiles/obj.MLIRTosaTransforms.dir/TosaProfileCompliance.cpp.o -c mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:384:16: error: rvalue reference to type 'const SmallVector<...>' cannot bind to lvalue of type 'const SmallVector<...>'
  384 |   const auto &&opRequiredMode = maybeOpRequiredMode.value();
      |                ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:447:12: note: in instantiation of function template specialization 'TosaProfileCompliance::checkProfileOrExtension<mlir::tosa::Profile>' requested here
  447 |     return checkProfileOrExtension<Profile>(op, targetEnv,
      |            ^
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:384:16: error: rvalue reference to type 'const SmallVector<...>' cannot bind to lvalue of type 'const SmallVector<...>'
  384 |   const auto &&opRequiredMode = maybeOpRequiredMode.value();
      |                ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~
mlir/lib/Dialect/Tosa/Transforms/TosaProfileCompliance.cpp:457:12: note: in instantiation of function template specialization 'TosaProfileCompliance::checkProfileOrExtension<mlir::tosa::Extension>' requested here
  457 |     return checkProfileOrExtension<Extension>(op, targetEnv,
      |            ^
2 errors generated.

[localspook]

It doesn’t look like that check offers fix-its that make variables rvalue references, so it looks like the fix-it to make the variable an lvalue reference (add an &) is somehow being applied twice?

[vbvictor]

It doesn’t look like that check offers fix-its that make variables rvalue references, so it looks like the fix-it to make the variable an lvalue reference (add an &) is somehow being applied twice?

Yes, it's most likely a bug in duplication of & in fix-it.

[localspook]

Reproducer (Godbolt):

template <typename T>
struct S {
  S();
  S(const S&);
};

template <typename T>
void f() {
  const T a;
  const T b = a;
  (void)b;
}

template void f<S<int>>();
template void f<S<long>>();

Each instantiation of f() causes another warning, and another fix-it. I don't see an elegant way to fix that. Maybe track the SourceLocations at which we've warned, and don't warn for the same location twice? Stop offering fix-its for substituted template parameters?

[vbvictor]

Oh, it must be a regression after #152101.
Generally clang-tidy deduplicate error messages if they have the same "message line".
Before that path, clang-tidy didn't print type information so we emitted only one message:

a.cpp:10:11: warning: local copy 'b' of the variable 'a' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]
   10 |   const T b = a;
      |           ^
      |          &

Now, we also have types in output message so clang-tidy can not automatically deduplicate messages:

a.cpp:10:11: warning: local copy 'b' of the variable 'a' of type 'const S<int>' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]
   10 |   const T b = a;
      |           ^
      |          &
a.cpp:10:11: warning: local copy 'b' of the variable 'a' of type 'const S<long>' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]
   10 |   const T b = a;
      |           ^
      |          &

CC @carlosgalvezp

[vbvictor]

I guess one potential way to fix it is to place type information inside "note" message so that deduplication of "warnings" still works.

This is how AI sees it:

1. Only warnings/errors are deduplicated: The comparison operators (LessClangTidyError and EqualClangTidyError) compare entire
  ClangTidyError objects based on:
    - File path
    - File offset
    - Diagnostic name (check name)
    - Message text
  2. Notes are preserved: Notes are stored as part of each ClangTidyError object in a Notes vector. They are not involved in the
  deduplication comparison - they're just carried along with their parent warning/error.
  3. Deduplication process:
    - Errors are sorted using LessClangTidyError
    - Then llvm::unique with EqualClangTidyError removes consecutive duplicates
    - This effectively removes duplicate warnings at the same location with the same message
  4. Additional deduplication for alias checkers: There's a separate mechanism (removeDuplicatedDiagnosticsOfAliasCheckers) that
  removes diagnostics from different checkers that point to the same issue (same location and message but different check names).

  So to answer your question directly: deduplication only suppresses duplicate warning/error messages, not notes. Notes remain
  attached to their parent warnings and are shown whenever the warning is displayed.

Another one is disable fixits for templates

Maybe track the SourceLocations at which we've warned, and don't warn for the same location twice?

This seems hard and time-consuming to implement IMO, and we should reuse standard deduplication

[carlosgalvezp]

I'm OK with reverting the change and finding a better solution afterwards!