Page MenuHomePhabricator

[Sema] Add warning for unused lambda captures
ClosedPublic

Authored by malcolm.parsons on Jan 9 2017, 5:33 AM.

Details

Summary

Warn when a lambda explicitly captures something that is not used in its body.

The warning is part of -Wunused and can be enabled with -Wunused-lambda-capture.

Diff Detail

Repository
rL LLVM

Event Timeline

malcolm.parsons retitled this revision from to [Sema] Add warning for unused lambda captures.
malcolm.parsons updated this object.
malcolm.parsons added a subscriber: cfe-commits.
arphaman added inline comments.
test/SemaCXX/uninitialized.cpp
1437 ↗(On Diff #83615)

I think that expected-warning shouldn't be used here as you have (void)fname in the lambda (I don't get this warning if I test this locally).

Don't warn in a dependent context.
Remove spurious expected-warning.

malcolm.parsons marked an inline comment as done.Jan 9 2017, 9:22 AM
arphaman edited edge metadata.Jan 10 2017, 2:34 AM

I think that the patch would be neater if you add "-Wno-unused-lambda-capture" to the options for all of the tests that are modified by this patch in the CXX/ directory. This would avoid redundant (void) uses and ensure that the (void) uses won't interfere with the original intent where things might be only used in the capture list.

malcolm.parsons edited edge metadata.

Use -Wno-unused-lambda-capture in existing tests.

include/clang/Sema/ScopeInfo.h
457 ↗(On Diff #83792)

Should this be moved into one of the PointerIntPairs?

arphaman added inline comments.Jan 10 2017, 6:27 AM
include/clang/Sema/ScopeInfo.h
457 ↗(On Diff #83792)

I'm not sure.. If we needed other capturing information in the future I would say that this should be its own field, but I'm not aware of anything else that's needed for this class. So I guess it would be better to pack it into VarAndNestedAndThis, but I wouldn't say it's a deal breaker.

aaron.ballman added inline comments.Jan 10 2017, 5:16 PM
include/clang/Sema/ScopeInfo.h
457 ↗(On Diff #83792)

I'm not keen on inconsistently initializating data members; can you perform this initialization in the constructor instead?

lib/Parse/ParseExprCXX.cpp
738 ↗(On Diff #83792)

This change is unrelated to the patch; you can go ahead and commit it without review rather than have it as part of this patch.

lib/Sema/SemaLambda.cpp
1479 ↗(On Diff #83792)

Why does From need to be a non-const reference?

1483 ↗(On Diff #83792)

Missing a full stop at the end of the comment.

test/SemaCXX/warn-unused-lambda-capture.cpp
17 ↗(On Diff #83792)

This does not match the behavior for other -Wunused flags. e.g.,

void f() {
  int i;
  (void)sizeof(i);
}

I don't think diagnosing this test case is correct.

59 ↗(On Diff #83792)

Likewise here.

Quuxplusone added inline comments.
test/SemaCXX/warn-unused-lambda-capture.cpp
17 ↗(On Diff #83792)

I see some value to maybe diagnosing *something* here. For example, [] { return sizeof(i); } would produce the same result as [i] { return sizeof(i); } but with one fewer capture, so removing the i might be seen as an improvement.

But I'm not sure how to convey this information to the user. You could say "variable i used in unevaluated context refers to the i in the outer scope, not the captured i"... except that I think that would be false. Given that you *have* captured an i, sizeof(i) definitely refers to the captured one AFAIK. The fact that the captured i shadows an i in the outer scope is irrelevant --- in fact the user is *expecting* to shadow the outer i.

Perhaps it would be appropriate to reword the diagnostic to "lambda captures variable i unnecessarily". I would also lean toward splitting it into two diagnostics — one for "this capture is unnecessary" (as in this example) and one for "this capture doesn't even appear lexically in the body of the lambda". Not sure how other people would feel about that.

You should add some test cases with decltype(i) for the same reason as sizeof(i).

26 ↗(On Diff #83792)

Would this still produce a diagnostic if decltype(j) were something complicated like lock_guard or string? Presumably it should do the same thing, more or less, as the other -Wunused diagnostics, which I think is "don't warn if the constructor/destructor might actually do important work".

malcolm.parsons marked an inline comment as done.Jan 11 2017, 1:40 AM
malcolm.parsons added inline comments.
include/clang/Sema/ScopeInfo.h
457 ↗(On Diff #83792)

I'm not keen on repeating the initialization in every constructor.

lib/Sema/SemaLambda.cpp
1479 ↗(On Diff #83792)

It's not related to the warning; it looks like an unnecessary copy.

test/SemaCXX/warn-unused-lambda-capture.cpp
17 ↗(On Diff #83792)

Does "lambda capture 'i' is not odr-used" make more sense?

26 ↗(On Diff #83792)

I was planning to check for that; thanks for the reminder.

Quuxplusone added inline comments.Jan 11 2017, 2:06 AM
test/SemaCXX/warn-unused-lambda-capture.cpp
17 ↗(On Diff #83792)

Does "lambda capture 'i' is not odr-used" make more sense?

That would completely satisfy *me*, for what that's worth. It admittedly doesn't match the other -Wunused diagnostics, but it is concise and correct — at least I assume it's correct. :)

test/SemaCXX/warn-unused-lambda-capture.cpp
17 ↗(On Diff #83792)

C++14 [expr.prim.lambda]p18:

[ Note: An id-expression that is not an odr-use refers to the original entity, never to a member
of the closure type. Furthermore, such an id-expression does not cause the implicit capture of the entity.
— end note ]

malcolm.parsons marked 9 inline comments as done.

Change warning message.
Check for side effects.
Add tests for side effects.
Add tests for decltype.
Use const reference.
Add . to comment.
Remove unrelated comment fix.

aaron.ballman added inline comments.Jan 12 2017, 6:14 AM
include/clang/Basic/DiagnosticSemaKinds.td
319 ↗(On Diff #83942)

We do not use the term "odr-use" in any of our other frontend diagnostics; I don't think this is a term that users will actually understand. I think we should drop the term and just use "used".

include/clang/Sema/ScopeInfo.h
457 ↗(On Diff #83792)

Consistency is the important bit (see the golden rule in http://llvm.org/docs/CodingStandards.html) -- if you want to move things to be in-class initializers, that would be a reasonable follow-up patch. For this patch, please put the initializer into the constructor.

lib/Sema/SemaLambda.cpp
1479 ↗(On Diff #83792)

Thanks for adding the const qualifier.

test/SemaCXX/warn-unused-lambda-capture.cpp
17 ↗(On Diff #83792)

Yes, that is correct; it's just incredibly confusing to most mortal programmers. Perhaps instead we can add extra information to the diagnostic telling the user that the variable need not be captured when it's in an unevaluated context (if possible). e.g.,

"lambda capture %0 %select{is not used|is not required to be captured for this use}1"

or something similar. My concern here is that coding standards tell users to explicitly capture variables that are referenced within the lambda, and the user will get a diagnostic that they assume is a false positive because most people don't know what an odr-use is. A quick search online found a handful of such coding standards, btw.

26 ↗(On Diff #83792)

One more complex case:

#include <typeinfo>

struct B {
  virtual ~B() = default;
};

struct D : B {};

struct C {
  B& get_d() const;
  C get_c() const;
};

void f() {
  C c1, c2;
  [c1, c2] {
    (void)typeid(c1.get_d());
    (void)typeid(c2.get_c());
  }();
}

Hopefully this does not diagnose c1 as being unused but still diagnoses c2 as unused.

malcolm.parsons marked 9 inline comments as done.

Improve warning message for use in unevaluated context.
Initialise used flags in constructors.

test/SemaCXX/warn-unused-lambda-capture.cpp
26 ↗(On Diff #83792)

These tests are run with -nostdsysteminc, so I ran it manually:

aaron.cpp:17:21: warning: expression with side effects will be evaluated despite being used as an operand to 'typeid'
      [-Wpotentially-evaluated-expression]
    (void)typeid(c1.get_d());
                    ^
aaron.cpp:16:8: warning: lambda capture 'c2' is not required to be captured for use in an unevaluated context [-Wunused-lambda-capture]
  [c1, c2] {
       ^
2 warnings generated.
aaron.ballman accepted this revision.Jan 12 2017, 6:45 PM
aaron.ballman edited edge metadata.

LGTM; this is an awesome new diagnostic, thank you for working on it!

test/SemaCXX/warn-unused-lambda-capture.cpp
26 ↗(On Diff #83792)

Perfect!

This revision is now accepted and ready to land.Jan 12 2017, 6:45 PM
test/SemaCXX/warn-unused-lambda-capture.cpp
86 ↗(On Diff #84122)

I don't know why the unevaluated context warning didn't work here.

aaron.ballman added inline comments.Jan 13 2017, 5:27 AM
test/SemaCXX/warn-unused-lambda-capture.cpp
86 ↗(On Diff #84122)

Hmm, it should have, come to think of it. I don't think that's an odr-use (when I get near a standard, I'll double-check), and it is unevaluated. Worth poking at.

test/SemaCXX/warn-unused-lambda-capture.cpp
86 ↗(On Diff #84122)

The unevaluated context warning worked on line 38 where it's not in a template and on line 85 where it's sizeof instead of decltype.

Somehow the template instantiation is avoiding some work when the capture doesn't have a dependent type.

aaron.ballman added inline comments.Jan 13 2017, 6:19 AM
test/SemaCXX/warn-unused-lambda-capture.cpp
86 ↗(On Diff #84122)

That might make some degree of sense. Btw, I did double check, and it's definitely not an odr-use.

I think this edge case can be handled in a follow-up patch if you want to get the primary work in. If you'd prefer to have it fixed for the initial commit, that's obviously fine too.

This revision was automatically updated to reflect the committed changes.

http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/1122/steps/bootstrap%20clang/logs/stdio

/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1116:17: error: lambda capture 'BitWidth' is not used [-Werror,-Wunused-lambda-capture]
    auto KOF = [BitWidth](const APInt &KnownOne, unsigned ShiftAmt) {
                ^
/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1127:17: error: lambda capture 'BitWidth' is not used [-Werror,-Wunused-lambda-capture]
    auto KZF = [BitWidth](const APInt &KnownZero, unsigned ShiftAmt) {
                ^
/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1131:17: error: lambda capture 'BitWidth' is not used [-Werror,-Wunused-lambda-capture]
    auto KOF = [BitWidth](const APInt &KnownOne, unsigned ShiftAmt) {
                ^
3 errors generated.

The warnings are correct.

Should I remove the unused lambda captures or revert this changeset?

IMO, you can go ahead and commit a fix that removes the captures, and the fix can be reviewed post-commit. I don't think you should revert this patch as you'll have to remove the captures anyway before reinstating this patch, so might as well do it now.

http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/1122/steps/bootstrap%20clang/logs/stdio

/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1116:17: error: lambda capture 'BitWidth' is not used [-Werror,-Wunused-lambda-capture]
    auto KOF = [BitWidth](const APInt &KnownOne, unsigned ShiftAmt) {
                ^
/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1127:17: error: lambda capture 'BitWidth' is not used [-Werror,-Wunused-lambda-capture]
    auto KZF = [BitWidth](const APInt &KnownZero, unsigned ShiftAmt) {
                ^
/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1131:17: error: lambda capture 'BitWidth' is not used [-Werror,-Wunused-lambda-capture]
    auto KOF = [BitWidth](const APInt &KnownOne, unsigned ShiftAmt) {
                ^
3 errors generated.

The warnings are correct.

Should I remove the unused lambda captures or revert this changeset?

The warnings look correct to me; I think you should remove the unused lambda captures (no need for a test or review since changes are NFC).

There are quite a few of them!

krasin added a subscriber: krasin.Jan 18 2017, 2:03 PM

This change makes Clang hardly incompatible with MSVC++. Consider the following program:

#include <stdio.h>

int main(void) {
  const int kDelta = 10000001;
  auto g = [kDelta](int i)
           {
             printf("%d\n", i % kDelta);
           };
  g(2);
}

Clang will warn about the unused lambda capture:

$ clang++ lala.cc -o lala -std=c++14 -Wall -Werror && ./lala
lala.cc:5:13: error: lambda capture 'kDelta' is not required to be captured for use in an unevaluated context [-Werror,-Wunused-lambda-capture]
  auto g = [kDelta](int i)
            ^
1 error generated.

It will compile without any warnings if I remove kDelta from the list of captures:

#include <stdio.h>

int main(void) {
  const int kDelta = 10000001;
  auto g = [](int i)
           {
             printf("%d\n", i % kDelta);
           };
  g(2);
}

But then Microsoft C++ compiler will raise the error:

error C3493: 'kDelta' cannot be implicitly captured because no default capture mode has been specified

At this point, I am unsure how can this be improved, but I feel that pointing out this inconsistency might be useful.
For real world case, see https://codereview.chromium.org/2646553002/, where I tried to fix unused lambda captures in V8, in particular, this failure: https://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21172/steps/compile/logs/stdio

This change makes Clang hardly incompatible with MSVC++. Consider the following program:

#include <stdio.h>

int main(void) {
  const int kDelta = 10000001;
  auto g = [kDelta](int i)
           {
             printf("%d\n", i % kDelta);
           };
  g(2);
}

Clang will warn about the unused lambda capture:

$ clang++ lala.cc -o lala -std=c++14 -Wall -Werror && ./lala
lala.cc:5:13: error: lambda capture 'kDelta' is not required to be captured for use in an unevaluated context [-Werror,-Wunused-lambda-capture]
  auto g = [kDelta](int i)
            ^
1 error generated.

Is kDelta considered to be used in an unevaluated context here? I thought unevaluated context in c++ means the expression is used as operands of operators such as typeid and decltype.