[Sema] Avoid Wrange-loop-analysis false positives

[Mordante]

Summary

When Wrange-loop-analysis issues a diagnostic on a dependent type in a template the diagnostic may not be valid for all instantiations. Therefore the diagnostic is suppressed during the instantiation. Non dependent types still issue a diagnostic.

The same can happen when using macros. Therefore the diagnostic is disabled for macros.

Fixes https://bugs.llvm.org/show_bug.cgi?id=44556

[merge_guards_bot]

Unit tests: pass. 62002 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[aaron.ballman]

LGTM! Do you think this should also be pushed onto the release branch?

[aaronpuchert]

As I wrote on the bug, I think we should only suppress one of the warnings in templates (and maybe always):

<source>:18:22: warning: loop variable '_' is always a copy because the range of type 'const Rng' does not return a reference [-Wrange-loop-analysis]
    for (const auto& _ : t)
                     ^
<source>:26:5: note: in instantiation of function template specialization 'f<Rng>' requested here
    f(Rng{});
    ^
<source>:18:10: note: use non-reference type 'int'
    for (const auto& _ : t)
         ^~~~~~~~~~~~~~~

However, I think we shouldn't suppress the other:

<source>:18:21: warning: loop variable '_' of type 'const X' creates a copy from type 'const X' [-Wrange-loop-analysis]
    for (const auto _ : t)
                    ^
<source>:25:5: note: in instantiation of function template specialization 'f<X [3]>' requested here
    f(array);
    ^
<source>:18:10: note: use reference type 'const X &' to prevent copying
    for (const auto _ : t)
         ^~~~~~~~~~~~~~
                    &

This generates a bunch of unneeded non-trivial copies, and I think we want to warn about that.

[xbolva00]

Yes, but minimal fix is better for release branch, so @hans should merge it.

Handling your case probably needs more work and patches..

[aaronpuchert]

In D73007#1829709, @xbolva00 wrote:

Yes, but minimal fix is better for release branch, so @hans should merge it.

I think -Wall shouldn't warn about reference types in a range-based for loop (unless it's the wrong type and inadvertently creates a copy).

Handling your case probably needs more work and patches..

See my inline comments. The bug in itself is not terribly troublesome, this issue has existed in many previous releases. The problem is that the warning is now in -Wall, and I think we should be more conservative in -Wall.

clang/lib/Sema/SemaStmt.cpp

2703–2704

That's the part the bothers me.

2768–2778

We could either remove this part or group warn_for_range_variable_always_copy under a separate flag that's not in -Wall.

[Mordante]

In D73007#1829709, @xbolva00 wrote:

Yes, but minimal fix is better for release branch, so @hans should merge it.

Handling your case probably needs more work and patches..

Agreed.

[Mordante]

In D73007#1829597, @aaron.ballman wrote:

LGTM! Do you think this should also be pushed onto the release branch?

Thanks for the review. Yes the bug has been marked as a release blocker.

[Mordante]

clang/lib/Sema/SemaStmt.cpp

2703–2704

I think we shouldn't remove the warning, it is useful. We can consider to move this out of -Wall, but I first like to hear the opinion of the other reviewers in this matter. If we want this change I'll be happy to create a new patch.

[aaronpuchert]

I also still think that warn_for_range_copy should be emitted even in templates.

These aren't false positives in my opinion, especially since we're now pretty conservative about that warning.

clang/lib/Sema/SemaStmt.cpp

2703–2704

I don't see a problem with having this as an optional warning that can be turned on for those who want it.

Note that this isn't my personal opinion, I'm actually fine with it. But I've met considerable resistance to turning the warning on before, and it's somewhat hard to argue against that. Binding references to temporaries is perfectly normal C++, and we don't discourage it anywhere else.

[aaron.ballman]

clang/lib/Sema/SemaStmt.cpp

2703–2704

I think we shouldn't remove the warning, it is useful.

Agreed.

We can consider to move this out of -Wall, but I first like to hear the opinion of the other reviewers in this matter. If we want this change I'll be happy to create a new patch.

We typically want warnings to be enabled by default and have a low-false positive rate so that users get the benefit of the diagnostic without undue effort (like manually disabling it because they find the diagnostics low-value). I think the point @aaronpuchert raises is an interesting one in that binding a reference to a temp is not necessarily wrong, so I could see some users wanting to disable the diagnostic rather than change their code, which suggests that -Wall may not be the best place for it.

[hans]

I've cherry-picked this to 10.x in 318677e78def0023d210a29f4b3cf648e02f9fdc. Please let me know if there are any follow-ups.

[aaronpuchert]

I think we can actually view this in more general terms, unrelated to range-based for loops.

// X is non-trivially copyable.
struct X { X(const X&); };
struct Y { Y(const X&); };

X retVal();
const X& retRef();

// In function scope ...
const X &x1 = retVal(); // (1) Ok, but maybe misleading.
const X  x2 = retRef(); // (2) Creates a copy that's unnecessary.
const Y &y1 = retVal(); // (3) Ok, but maybe misleading, and maybe not intended.
const Y &y2 = retRef(); // (4) Ok, but maybe misleading, and maybe not intended.

My point is that the standard explicitly allows binding temporaries to const references and mandates a lifetime extension for the temporary that makes this safe.

It definitely makes sense to warn about (2) in -Wall under certain circumstances, because it might be a performance issue. (We have the similar -Wpessimizing-move.) But (1) is a stylistic issue, and doesn't belong in -Wall. I'm somewhat uncertain about (3) and (4): these are implicit conversions at work and one could argue that we're not warning about them anywhere else, but one could also argue that this is a bit harder to follow in a range-based for loop.

[aaronpuchert]

In D73007#1837271, @aaronpuchert wrote:

one could also argue that this is a bit harder to follow in a range-based for loop.

This seems to be the argumentation of https://bugs.llvm.org/show_bug.cgi?id=32823#c0, so I guess it's Ok.

[rtrieu]

I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall.

[aaron.ballman]

In D73007#1837621, @rtrieu wrote:

I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall.

+1

[hans]

In D73007#1837621, @rtrieu wrote:

I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall.

It sounds like we may want this to go to the 10.x branch too, to avoid changing the interface too much between releases. So please keep me posted about how this progresses.

[aaronpuchert]

Here is a proposal: we add two children to -Wrange-loop-analysis.

• -Wrange-loop-construct warns about possibly unintended constructor calls. This might be in -Wall. It contains
  • warn_for_range_copy: loop variable A of type B creates a copy from type C
  • warn_for_range_const_reference_copy: loop variable A is initialized with a value of a different type resulting in a copy
• -Wrange-loop-bind-ref[erence] warns about misleading use of reference types. This might not be in -Wall. It contains
  • warn_for_range_variable_always_copy: loop variable A is always a copy because the range of type B does not return a reference

[aaron.ballman]

In D73007#1839648, @aaronpuchert wrote:

Here is a proposal: we add two children to -Wrange-loop-analysis.

• -Wrange-loop-construct warns about possibly unintended constructor calls. This might be in -Wall. It contains
  • warn_for_range_copy: loop variable A of type B creates a copy from type C
  • warn_for_range_const_reference_copy: loop variable A is initialized with a value of a different type resulting in a copy
• -Wrange-loop-bind-ref[erence] warns about misleading use of reference types. This might not be in -Wall. It contains
  • warn_for_range_variable_always_copy: loop variable A is always a copy because the range of type B does not return a reference

I think this seems like a reasonable approach.

[Mordante]

@aaronpuchert, @aaron.ballman I agree with the proposal, I'll work on a patch.

@hans Once the patch is accepted I'll send you a pull request and an update for the release notes.

[Mordante]

Created D73434.

[aaronpuchert]

I've been thinking about the warning messages, which seem to a bit inaccurate. Sorry for piggy-backing this onto the change, I hope you don't mind.

clang/lib/Sema/SemaStmt.cpp

2758–2759

The message here says: "loop variable A has type B but is initialized with type C resulting in a copy".

That's not necessarily true, we just know (I think) that a constructor is called. The constructor might copy, but it might also not. However, the constructed object is bound to a reference, which is potentially misleading. One writes const X& x : range and x doesn't bind to the return value of *begin(range), but to a temporary constructed from that.

Perhaps a more accurate message would be: "loop variable A of type B binds to a temporary constructed from type C" with the appropriate types.

2772–2773

The message here is: "loop variable A is always a copy because the range of type B does not return a reference". This is somewhat misleading, some ranges create objects on-the-fly, in which case these aren't copies. (Assuming RVO the copy constructor might have never been called.)

I would suggest to rephrase this as something like "loop variable A binds to a temporary value produced by a range of type B".

2823–2824

Given that VariableType == VD->getType() and InitExpr->getType() == VD->getInit()->getType(), can these two types ever be different other than cv-qualifiers? We only call this function if VariableType is not a reference type.

Which begs the question: which value provides C in "loop variable A of type B creates a copy from type C"? I would suggest to omit the second type and write: "loop variable A creates a copy of type B", perhaps with stripping const from B.

clang/lib/Sema/SemaStmt.cpp

StmtResult Sema::FinishObjCForCollectionStmt(Stmt *S, Stmt *B) {
 
   ForStmt->setBody(B);
   return S;
 }
 
 // Warn when the loop variable is a const reference that creates a copy.
 // Suggest using the non-reference type for copies.  If a copy can be prevented
 // suggest the const reference type that would do so.
 // For instance, given "for (const &Foo : Range)", suggest
 // "for (const Foo : Range)" to denote a copy is made for the loop.  If
 // possible, also suggest "for (const &Bar : Range)" if this type prevents
 // the copy altogether.
 static void DiagnoseForRangeReferenceVariableCopies(Sema &SemaRef,
                                                     const VarDecl *VD,
                                                     QualType RangeInitType) {
   const Expr *InitExpr = VD->getInit();
   if (!InitExpr)
     return;
if (isa<UnaryOperator>(E)) {
     QualType ReturnType = FD->getReturnType();
     ReturnsReference = ReturnType->isReferenceType();
   }
 
   if (ReturnsReference) {
     // Loop variable creates a temporary.  Suggest either to go with
     // non-reference loop variable to indicate a copy is made, or
     // the correct time to bind a const reference.
     SemaRef.Diag(VD->getLocation(), diag::warn_for_range_const_reference_copy)
         << VD << VariableType << E->getType();
     QualType NonReferenceType = VariableType.getNonReferenceType();
     NonReferenceType.removeLocalConst();
     QualType NewReferenceType =
         SemaRef.Context.getLValueReferenceType(E->getType().withConst());
     SemaRef.Diag(VD->getBeginLoc(), diag::note_use_type_or_non_reference)
         << NonReferenceType << NewReferenceType << VD->getSourceRange()
         << FixItHint::CreateRemoval(VD->getTypeSpecEndLoc());
   } else if (!VariableType->isRValueReferenceType()) {
     // The range always returns a copy, so a temporary is always created.
     // Suggest removing the reference from the loop variable.
     // If the type is a rvalue reference do not warn since that changes the
     // semantic of the code.
     SemaRef.Diag(VD->getLocation(), diag::warn_for_range_variable_always_copy)
         << VD << RangeInitType;
     QualType NonReferenceType = VariableType.getNonReferenceType();
     NonReferenceType.removeLocalConst();
     SemaRef.Diag(VD->getBeginLoc(), diag::note_use_non_reference_type)
         << NonReferenceType << VD->getSourceRange()
         << FixItHint::CreateRemoval(VD->getTypeSpecEndLoc());
   }
 }
 
 /// Determines whether the @p VariableType's declaration is a record with the
 /// clang::trivial_abi attribute.
 static bool hasTrivialABIAttr(QualType VariableType) {
   if (CXXRecordDecl *RD = VariableType->getAsCXXRecordDecl())
     return RD->hasAttr<TrivialABIAttr>();
static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef,
   ASTContext &Ctx = SemaRef.Context;
   if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
       (VariableType.isTriviallyCopyableType(Ctx) ||
        hasTrivialABIAttr(VariableType)))
     return;
 
   // Suggest changing from a const variable to a const reference variable
   // if doing so will prevent a copy.
   SemaRef.Diag(VD->getLocation(), diag::warn_for_range_copy)
       << VD << VariableType << InitExpr->getType();
   SemaRef.Diag(VD->getBeginLoc(), diag::note_use_reference_type)
       << SemaRef.Context.getLValueReferenceType(VariableType)
       << VD->getSourceRange()
       << FixItHint::CreateInsertion(VD->getLocation(), "&");
 }
 
 /// DiagnoseForRangeVariableCopies - Diagnose three cases and fixes for them.
 /// 1) for (const foo &x : foos) where foos only returns a copy.  Suggest
 ///    using "const foo x" to show that a copy is made
 /// 2) for (const bar &x : foos) where bar is a temporary initialized by bar.
 ///    Suggest either "const bar x" to keep the copying or "const foo& x" to
 ///    prevent the copy.
 /// 3) for (const foo x : foos) where x is constructed from a reference foo.
 ///    Suggest "const foo &x" to prevent the copy.
 static void DiagnoseForRangeVariableCopies(Sema &SemaRef,
                                            const CXXForRangeStmt *ForStmt) {
+  if (SemaRef.inTemplateInstantiation())
+    return;
+
   if (SemaRef.Diags.isIgnored(diag::warn_for_range_const_reference_copy,
                               ForStmt->getBeginLoc()) &&
       SemaRef.Diags.isIgnored(diag::warn_for_range_variable_always_copy,
                               ForStmt->getBeginLoc()) &&
       SemaRef.Diags.isIgnored(diag::warn_for_range_copy,
                               ForStmt->getBeginLoc())) {
     return;
   }
 
   const VarDecl *VD = ForStmt->getLoopVariable();
   if (!VD)
     return;
 
   QualType VariableType = VD->getType();
 
   if (VariableType->isIncompleteType())
     return;
 
   const Expr *InitExpr = VD->getInit();
   if (!InitExpr)
     return;
 
+  if (InitExpr->getExprLoc().isMacroID())
+    return;
+
   if (VariableType->isReferenceType()) {
     DiagnoseForRangeReferenceVariableCopies(SemaRef, VD,
                                             ForStmt->getRangeInit()->getType());
   } else if (VariableType.isConstQualified()) {
     DiagnoseForRangeConstVariableCopies(SemaRef, VD);
   }
 }
 

clang/test/SemaCXX/warn-range-loop-analysis.cpp

void test10() {
   // expected-note@-2 {{'Bar'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:20}:""
 
   for (const Bar&x : C) {}
   // expected-warning@-1 {{always a copy}}
   // expected-note@-2 {{'Bar'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:" "
 }
+
+template <class T>
+void test_template_function() {
+  // In a template instantiation the diagnostics should not be emitted for
+  // loops with dependent types.
+  Container<Bar> C;
+  for (const Bar &x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
+
+  Container<T> Dependent;
+  for (const T &x : Dependent) {}
+}
+template void test_template_function<Bar>();
+
+template <class T>
+struct test_template_struct {
+  static void static_member() {
+    Container<Bar> C;
+    for (const Bar &x : C) {}
+    // expected-warning@-1 {{always a copy}}
+    // expected-note@-2 {{'Bar'}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:21}:""
+
+    Container<T> Dependent;
+    for (const T &x : Dependent) {}
+  }
+
+  void member() {
+    Container<Bar> C;
+    for (const Bar &x : C) {}
+    // expected-warning@-1 {{always a copy}}
+    // expected-note@-2 {{'Bar'}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:21}:""
+
+    Container<T> Dependent;
+    for (const T &x : Dependent) {}
+  }
+};
+template struct test_template_struct<Bar>;
+
+struct test_struct_with_templated_member {
+  void member() {
+    Container<Bar> C;
+    for (const Bar &x : C) {}
+    // expected-warning@-1 {{always a copy}}
+    // expected-note@-2 {{'Bar'}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:21}:""
+  }
+
+  template <class T>
+  void template_member() {
+    Container<Bar> C;
+    for (const Bar &x : C) {}
+    // expected-warning@-1 {{always a copy}}
+    // expected-note@-2 {{'Bar'}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:21}:""
+
+    Container<T> Dependent;
+    for (const T &x : Dependent) {}
+  }
+};
+template void test_struct_with_templated_member::template_member<Bar>();
+
+#define TEST_MACRO            \
+  void test_macro() {         \
+    Container<Bar> C;         \
+    for (const Bar &x : C) {} \
+  }
+
+TEST_MACRO