Page MenuHomePhabricator

[Analyzer][WebKit] UncountedLambdaCaptureChecker
ClosedPublic

Authored by jkorous on Jun 29 2020, 9:37 PM.

Diff Detail

Event Timeline

jkorous created this revision.Jun 29 2020, 9:37 PM
NoQ added a comment.Jun 29 2020, 10:41 PM

Normally this isn't a big deal but in case of this checker i'm veeery curious about the highlighted columns (not just lines) and source ranges. Especially for the implicit case: like, how obvious is it which specific captured variable is problematic? I don't see a better way to test that than making a few FileCheck tests with those ^~~~~~ thingies but i think it's worth at least a couple of such test cases.

You can also add extra source ranges manually with BugReport::addRange.

It's probably also worth it to include the name of the captured variable in the warning message.

I know what you mean - actionability of these warnings was my major concern too.

jkorous updated this revision to Diff 276869.Jul 9 2020, 4:31 PM
  • There's actually no reason to not print the name of the variable in either case.
  • I added a test for each explicit/implicit capture and made tests more specific overall.
jkorous updated this revision to Diff 276877.Jul 9 2020, 5:10 PM
jkorous edited the summary of this revision. (Show Details)
  • added documentation
NoQ added a comment.Jul 9 2020, 7:16 PM

That looks much better, thanks! I've a few more tiny annoying nits.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
27

I recommend re-using this string because it's used by multiple checkers already.

We usually keep those in CommonBugCategories.h and CommonBugCategories.cpp.

82

I think it should be fairly easy to automatically figure out whether it's a raw pointer or a reference (?) I think we should get this polished in this patch if possible, because we're instantly turning the checker on by default.

clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
9–10

Oh my, i see what you did here!

I wonder if the second line actually tests the exact position of ^. Because by default {{xxx}} checks that xxx is a substring of the actual warning (in this case, notice). Might need regex matching to actually match the beginning and the end of the line.

16

So it points to the opening brace? I guess that's good enough with the name of the variable present in the warning text but there's room for experimentation - for instance, with some effort we could probably point to the first use of the variable; i'm not sure it's actually going to be much better but at least it'd point to the actual offending code.

35

Maybe let's put this comment inside so that nobody thought that it applies to the rest of the file :)

jkorous marked 2 inline comments as done.Jul 9 2020, 11:30 PM

Oh no - nits are welcome as always!

I agree with you on all of account and will work on it in the morning.

clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
9–10

Oh my, now I see it too :(

More in the response below.

16

Oh no ... [facepalm]

You just proved your point that I am matching just a substring here - in reality (all the handful of cases I've seen) the caret point to the start of the identifier.

I'd be inclined to say that now that we're printing the name of the variable the snippet isn't critical anymore and we might not necessarily have tests for it. But to be honest I am actually not sure if the current behavior (the caret points to the first occurrence of the implicitly captured variable) is defined or not.

I'll think about it - would prefer to not use multi-line regex magic if possible. Maybe I could somehow use serialized diagnostics here?

NoQ added inline comments.Jul 13 2020, 4:55 PM
clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
16

multi-line regex magic

Mmm, i'm thinking of [[ https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html | the usual -verify regex machinery ]], something like // expected-notice-re{{{{^}} ^{{$}}}}.

jkorous updated this revision to Diff 278287.Jul 15 2020, 1:03 PM

I didn't figure out how to match the code snippet with the caret by using -verify. IIUC it supports only {error,warning,remark,note} diagnostics while the code snippet is neither of these.
I took inspiration from clang/test/Misc/caret-diags-multiline.cpp and just use FileCheck %s --strict-whitespace.

jkorous marked 5 inline comments as done.Jul 15 2020, 4:38 PM
jkorous added inline comments.
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
27

I'll do this in a separate patch as I need to touch all the other WebKit checkers too.

jkorous updated this revision to Diff 278336.Jul 15 2020, 4:44 PM

distinguish between raw-ptr & reference

Friendly ping

NoQ accepted this revision.Aug 4 2020, 7:25 PM
NoQ added a reviewer: vsavchenko.

Whoops! Looks great now, i have no further comments, let's land :)

This revision is now accepted and ready to land.Aug 4 2020, 7:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 4:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript