Page MenuHomePhabricator

[Sema] Add some basic lambda capture fix-its
ClosedPublic

Authored by njames93 on Feb 18 2021, 10:07 AM.

Details

Summary

Adds fix-its when users forget to explicitly capture variables or this in lambdas

Addresses https://github.com/clangd/clangd/issues/697

Diff Detail

Event Timeline

njames93 created this revision.Feb 18 2021, 10:07 AM
njames93 requested review of this revision.Feb 18 2021, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 10:07 AM
njames93 updated this revision to Diff 324741.Feb 18 2021, 12:43 PM

Remove default capture by value fixit if 'this' is explicitly captured pre c++20.
Fix default capture insertion loc to be at the start of the capture list.

njames93 updated this revision to Diff 324823.Feb 18 2021, 5:29 PM

Fix tests

njames93 added inline comments.Feb 18 2021, 5:37 PM
clang/lib/Sema/SemaExprCXX.cpp
1229–1235

Could we just attach this fix directly to the warning?

njames93 updated this revision to Diff 324981.Feb 19 2021, 7:41 AM

Restrict availability of default capture fixit based on init captures already specified.
[=, A]() {} and [&, &A]() {} are both invalid as explicit captures can't be specified with the same type as default capture.
So we shouldn't offer '=' fixit if any init captures are by value and '&' if any by ref.

njames93 added inline comments.Feb 19 2021, 7:43 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7475–7476

Does the variable name need attaching to the note, given the note is attached to a fix-it containing the name of the variable already?

Ping

Hi Nathan! Thanks again for this patch and sorry for not taking time to review it yet. It is a busy week for me and my team, so I wasn't able to do much coding/reviewing, apologies for that. I expect to get some time for this in the upcoming days and I do think it's an important thing to dedicate time for.

Thanks! The code looks right, my comments are mostly for the comments around to validate my mental model of the code structure and behavior.

clang/lib/Sema/SemaExpr.cpp
17439

Would be useful to have high-level comments for all three cases you have here. This one is for individual variables, right?

17448

Not having return; after this one or the other ones throws me off-guard and I came to the realization that what you intend here is: the function can issue all FixItHints - for capturing a variable by value, reference and default captures by value and reference. So, in total there could be 4 FixItHint so that the user could choose between all of those and chose the one they want. Is that correct? I think this is something worth documenting.

17451

This comment seems a bit off to me? Could you please expand? (I also don't understand how this relates to the next line).

17703–17704

now there are 3 cast<LambdaScopeInfo>, maybe introduce a variable? would've been much better with C++ init + condition but still

kbobyrev added inline comments.Mar 5 2021, 10:46 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7478

also, "capture all" is a little confusing to me but I don't know what would be better. "capture everything"/"capture all variables/objects"? don't know if it's better.

njames93 updated this revision to Diff 328663.Mar 5 2021, 3:10 PM
njames93 marked 5 inline comments as done.

Address comments

njames93 updated this revision to Diff 328859.Mar 7 2021, 5:53 AM

Add check to prevent emitting copy capture fixes for non-copyable types

kbobyrev accepted this revision.Mar 8 2021, 12:34 AM

LGTM, thanks for implementing this and apologies for the review delay (was a tough week for me & my team).

The code is very easy to follow now and the only suggestions are stylistic nits.

clang/lib/Sema/SemaExpr.cpp
17440

This code heavily relies on not having any default captures and I can see that all of the code paths are correct (this condition is checked before calling the function), but I think it's better to explicitly check this with an assert.

17448
17454

nit: the style seems to be omitting {} for single-line sequences of if/for/whiles, probably remove in this case?

17455

nit: CT -> Ctor? CT looks a little unexpected.

17496

also allows removing enclosing {}

17504

maybe just pull the outside if into the inner one? does not seem to hurt readability and reduces the indentation + LOC number.

This revision is now accepted and ready to land.Mar 8 2021, 12:34 AM
njames93 updated this revision to Diff 328967.Mar 8 2021, 4:08 AM
njames93 marked 6 inline comments as done.

Address comments.

kbobyrev added inline comments.Mar 8 2021, 5:00 AM
clang/lib/Sema/SemaExpr.cpp
17456

(braces are not needed here, too)

Nice fixes! A few drive-by comments from me, up to you though.

clang/include/clang/Basic/DiagnosticSemaKinds.td
7475–7476

perhaps not, but I don't think it hurts

clang/lib/Sema/SemaExpr.cpp
17444

this lambda looks like it could/should be a standalone function

17453

I suspect you need to handle the case where RD is incomplete

17462

POD is rarely precisely the right concept (and deprecated for that reason).
Is isTriviallyCopyableType() just as good here?

17466

This logic assumes there's no default capture, whic his reasonable but not locally obvious - add an assert?

17498

While it's technically possible to transform [&a, &b] to [=, &a, &b], it seems very unlikely the user actually wants this: we should capture the new variable by copy and make that the default, even though so far we've been listing captures explicitly and by reference.

On the other hand, transforming [a, b] to [=] seems more useful, but isn't supported.

I'd suggest just leaving both cases out - only offer a default capture if there are no explicit captures already.

kbobyrev added inline comments.Mar 8 2021, 5:26 AM
clang/lib/Sema/SemaExpr.cpp
17466

I think this is the same as https://reviews.llvm.org/D96975#inline-920917 (which is done), am I wrong?

kbobyrev added inline comments.Mar 8 2021, 5:56 AM
clang/lib/Sema/SemaExpr.cpp
17462

Also, maybe if (T->isTriviallyCopyableType(Sema.getASTContext())) return true; at the top and just return false; here? This saves us the necessity to iterate through the ctors.

njames93 updated this revision to Diff 329011.Mar 8 2021, 7:43 AM
njames93 marked 8 inline comments as done.

Address more comments.

njames93 added inline comments.Mar 8 2021, 8:03 AM
clang/lib/Sema/SemaExpr.cpp
17453

I thought that could be skipped, but yeah you're right you could have a lambda parameter be an incomplete type if you're only referencing it.

17498

Transforming [a, b] to [=] is not a good idea imo. We would be replacing perfectly legal, non ambiguous code.

The reason default capture fix-its are emitted last is because more often than not they wont be the users intentions, however It's still nice to show that it is a possibility.

sammccall added inline comments.Mar 8 2021, 8:24 AM
clang/lib/Sema/SemaExpr.cpp
17498

I don't think fixits are for surfacing code transformations that are unlikely to be what the user wants.

njames93 updated this revision to Diff 329066.Mar 8 2021, 10:31 AM

Update to not offer Default capture fix if other variables are also captured.

njames93 marked 2 inline comments as done.Mar 8 2021, 10:48 AM
njames93 updated this revision to Diff 329075.Mar 8 2021, 11:12 AM

Prevent explicit 'this' capture fix-it if default capture mode is '=' and not in c++20 mode.

This revision was automatically updated to reflect the committed changes.