Page MenuHomePhabricator

[Sema] Avoid Wrange-loop-analysis false positives
ClosedPublic

Authored by Mordante on Jan 19 2020, 10:03 AM.

Details

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

Diff Detail

Event Timeline

Mordante created this revision.Jan 19 2020, 10:03 AM

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 accepted this revision.Jan 20 2020, 7:41 AM

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

This revision is now accepted and ready to land.Jan 20 2020, 7:41 AM

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 added a subscriber: hans.Jan 20 2020, 8:41 AM

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

Handling your case probably needs more work and patches..

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.

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

Handling your case probably needs more work and patches..

Agreed.

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 marked 2 inline comments as done.Jan 21 2020, 11:53 AM
Mordante added inline comments.
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.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.

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 added inline comments.Jan 21 2020, 2:10 PM
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 added a comment.Thu, Jan 23, 11:06 AM

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

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.

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.

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.

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 added a comment.Fri, Jan 24, 10:22 AM

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.

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

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.

@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.

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.