[clang] Warn about by-value const auto from lvalue

[urnathan]

Summary

This adds a warning about:

auto const v = returns_a_ref();

when the deduced type is a record, and suggests perhaps a missing '&'.
There's usually no need to make a local copy -- unless the reference
is unstable or the object mutated in subsequent code. Adjust to use a
ref, or use constructor notation -- either brace or parens -- to
avoid:

auto const &ref_ok = returns_a_ref();
auto const want_value_1 (returns_a_ref());
auto const want_value_2 {returns_a_ref()};

Found 4 cases in llvm itself (there's a followup patch).

I bottled out of adding this to Wextra or something, but perhaps that'd be a good idea?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2370

Not super happy about "call's lvalue return". How about something along the lines of: initialization of %0 creates a value copy of the returned reference; did you mean '&'?

2371

We typically don't add new off-by-default diagnostics to Clang because we have evidence to suggest almost nobody enables them. Is something holding us back from enabling this by default? If so, does it suggest this should be a clang-tidy check instead (perhaps under the performance module)?

clang/lib/Sema/SemaDecl.cpp

12267–12268

You can pass VDecl directly instead of getting its name. Also, you should fix the formatting issues.

clang/test/SemaCXX/warn-auto-value-copy.cpp

9–10

Can you also add a test whether the return type is const Big & and another one where the return type is const Big? For the latter case, do you still think we should suggest adding & or is the const Ty <-> const auto signaling user intent of some kind?

[urnathan]

thanks for the feedback

[aaron.ballman]

I see you switched this to on-by-default, thanks! Have you run this over some other large code bases to see what the false positive rate is? I know you ran it over LLVM and it found some true positives, but were there false positives as well? If we do notice some false positives in practice (but not a significant amount), I think adding this to -Wextra would not be a terrible compromise either.

clang/include/clang/Basic/DiagnosticSemaKinds.td

2370–2371

Saves on my eyes a bit to remove the leading whitespace. :-D

[dblaikie]

The "use brace or paren init to suppress" seems pretty questionable - like "()" to suppress "-Wparentheses" which I think is generally considered poor form, but has enough history to be acceptable.

This seems more like a stylistic thing, suitable for clang-tidy, rather than a warning? (some codebases might use "always auto" and/or "const by default" that could trip over this warning)

Though, perhaps given we already have -Wrange-loop-construct there's already precedent. But is this new warning powered by the same heuristics (as much as possible) as that warning? It'd be good if it was, for consistency. (I believe it has some heuristics for the size of the object or non-trivial copy, etc): https://godbolt.org/z/ch3nseG6b

[aaron.ballman]

In D114989#3206719, @dblaikie wrote:

This seems more like a stylistic thing, suitable for clang-tidy, rather than a warning? (some codebases might use "always auto" and/or "const by default" that could trip over this warning)

+1, this is what I was trying to get at with the false positive question. I suspect we'll find this is quite chatty in practice, but it'd be good to know just how bad it is to decide whether this is clang-tidy material, -Wextra, something else, etc.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_implied_std_initializer_list_not_found : Error<
   "cannot deduce type of initializer list because std::initializer_list was "
   "not found; include <initializer_list>">;
 def err_malformed_std_initializer_list : Error<
   "std::initializer_list must be a class template with a single type parameter">;
 def err_auto_init_list_from_c : Error<
   "cannot use __auto_type with initializer list in C">;
 def err_auto_bitfield : Error<
   "cannot pass bit-field as __auto_type initializer in C">;
+def warn_auto_creates_copy : Warning<
+  "initialization of %0 creates a value copy of the retuned reference;"
+  " did you mean '&'?">,
+  InGroup<DiagGroup<"auto-by-value-copy">>;
 
 // C++1y decltype(auto) type
 def err_decltype_auto_invalid : Error<
   "'decltype(auto)' not allowed here">;
 def err_decltype_auto_cannot_be_combined : Error<
   "'decltype(auto)' cannot be combined with other type specifiers">;
 def err_decltype_auto_function_declarator_not_declaration : Error<
   "'decltype(auto)' can only be used as a return type "

clang/lib/Sema/SemaDecl.cpp

if (VDecl->getType()->isUndeducedType()) {
     if (Res.get()->containsErrors()) {
       // Invalidate the decl as we don't know the type for recovery-expr yet.
       RealDecl->setInvalidDecl();
       VDecl->setInit(Res.get());
       return;
     }
     Init = Res.get();
 
+    QualType OrigTy = VDecl->getType();
     if (DeduceVariableDeclarationType(VDecl, DirectInit, Init))
       return;
+
+    if (!DirectInit && isa<CallExpr>(Init) && OrigTy->isUndeducedAutoType() &&
+        OrigTy.isLocalConstQualified() && Init->getValueKind() == VK_LValue &&
+        isa<RecordType>(Init->getType()))
+      Diag(VDecl->getLocation(), diag::warn_auto_creates_copy) << VDecl;
   }
 
   // dllimport cannot be used on variable definitions.
   if (VDecl->hasAttr<DLLImportAttr>() && !VDecl->isStaticDataMember()) {
     Diag(VDecl->getLocation(), diag::err_attribute_dllimport_data_definition);
     VDecl->setInvalidDecl();
     return;
   }

clang/test/SemaCXX/warn-auto-value-copy.cpp

+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+struct Big {
+  Big(Big const &) noexcept;
+  unsigned r[50];
+};
+
+Big &RFn();
+Big &(*RPtr)();
+
+const Big &CRFn();
+const Big CVFn();
+
+Big VFn();
+int &SR();
+
+void f() {
+  auto const copy1 = RFn();  // expected-warning{{a value copy of}}
+  auto const copy2 = RPtr(); // expected-warning{{a value copy of}}
+  auto const copy3 = CRFn(); // expected-warning{{a value copy of}}
+
+  auto const paren1(RFn());  // paren-init
+  auto const brace1{RPtr()}; // brace-init
+
+  auto const ok1 = VFn();  // by-value
+  auto const ok2 = CVFn(); // by-value
+  auto const ok3 = SR();   // non-class
+}