This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Implement conversion from Clang diagnostics to PathDiagnostics.
AcceptedPublic

Authored by NoQ on Jan 12 2021, 1:50 AM.

Details

Summary

This clang::DiagnosticConsumer can take normal clang diagnostics (eg., Sema errors or Clang-Tidy warnings) and turn them into static analyzer's PathDiagnostics which are then fed into PathDiagnosticConsumers. It's an essential component for my clang-tidy/static analyzer cross-integration efforts.

The consumer isn't magically aware of information that isn't present in Clang diagnostics but expected in path diagnostics. Such info includes checker name / bug type / category; the consumer expects an external provider to provide such information for every incoming diagnostic.

Additionally, PathDiagnostic's declaration-with-issue and uniqueing-declaration are currently set to nullptr. I'm slowly thinking about auto-detecting those based on the source location but technically all path diagnostic consumer facilities can function just fine without them (declaration-with-issue is there purely for displaying user-visible names and uniqueing declaration only makes sense for path-sensitive warnings that require exotic uniqueing which will probably never be fed into this consumer).

There are also a few features missing here and there, such as fix-it hint support, which i plan to add later.

Though technically possible, this consumer is never supposed to consume the output of TextPathDiagnosticConsumer; a lot of information is lost that way that can't be recovered. In particular, the proper way to teach clang-tidy to emit path diagnostics while keeping in mind that it already ships with static analyzer integration is to allow the integrated static analyzer to talk to path diagnostic consumers directly; static analyzer warnings should never go through this new consumer.

Diff Detail

Event Timeline

NoQ created this revision.Jan 12 2021, 1:50 AM
NoQ requested review of this revision.Jan 12 2021, 1:50 AM
NoQ updated this revision to Diff 316025.

Add a few comments here and there.

vsavchenko added inline comments.Jan 12 2021, 3:05 AM
clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h
24

I think that this class (or file?) needs to have some comments on how it is supposed to be used.

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
39

I think this type of stuff should be covered by a comment or a descriptive function name.

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
2

Looks like a copy-paste error :)

64

nit

64

nit: llvm::enumerate or llvm::zip?

84

TBH it is pretty hard to follow the test and from it's structure see what is the input and what is expected output. Maybe it can be reorganized a bit?

140

WE NEED MORE NOUNS!

150–151

Also doesn't seem relevant

155

It is also about the structure, I guess it would've been nice to have all the inputs and expected outputs to be specified here in the actual test, so the reader can figure out what is tested without diving deep into the classes. And it also seems that with the current structure you'll need a couple more classes for every new test.

Seems pretty straightforward and clean.

The cleanup of the report's message should be reworked.
Besides, that looks good to me.

I think these cases should be tested as well:

  • [Warning, Warning, Warning]
  • [Warning, Note, Note]
  • [Warning, Note, Note, Warning, Note]
  • [Warning, Note, Remark, Warning, Remark]

PS: clang-format; sorry for the nit-spam :D

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h
54–57

Overriding the virtual void clang::DiagnosticConsumer::finish() can't accomplish the same thing?

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
2

I've seen this a few times, and I still don't know why we use it.
The file extension should be enough to recognize that this is a C++ file.
Can someone clarify this?

15

I frequently see #include "clang/..."-ish includes but never #include <clang/...>.

24
38
39

Eh, I don't think it's gonna work this way.
We can not assume that the [ won't appear in the payload of the message.
Eg.:
NewDelete-checker-test.cpp:193

// newdelete-warning{{Argument to 'delete[]' is the address of the local variable 'i', which is not memory allocated by 'new[]'}}

The best you could do is to do a reverse search.
Do we emit the [mypackage.mychecker] suffix for all the reports? If not, then we have a problem.

46–53

So, Error and Fatalis treated in the same way as Warning.
What about the Ignored?

55–61

The ternary operator could simplify this inside the push_back function - emplace_back?

72

Should Desc and ShortDesc be the same?

75–78

Why not construct in place?

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
45

I would suggest removing the redundant virtual. The same applies to the other decls.

64

clang-format should format these files as well, am I right?

84

I can confirm.
What about passing the ExpectedDiags as a parameter?
Probably some shorter type alias could come handy for TestPathDiagnosticConsumer::ExpectedDiagTy and TestPathDiagnosticConsumer::ExpectedPieceTy.

140

What about calling this DiagnosticMock?

xazax.hun added inline comments.Jan 12 2021, 7:52 AM
clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h
39

A pointer pair might be small enough for a DenseMap?

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
21

Do we need this early return? We might get the same behavior by simply omitting this check. I have no strong preference about keeping or removing it.

NoQ added inline comments.Jan 12 2021, 1:50 PM
clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
2

https://llvm.org/docs/CodingStandards.html#file-headers

(with our file name it doesn't look like there's much space even for a short description)
(i should probably convert the long description into a doxygen comment as well)

15

Whoops thanks.

21

We need it. @steakhal figured this out correctly in the next comment.

39

Uh-oh, mmm, indeed. I should definitely make this optional as well.

55–61

Can't easily use ?: here because LHS and RHS are of different types and operator ?: doesn't do implicit casts.

72

They don't need to be the same but typically clang diagnostics don't provide two different wordings for the same warning.

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
45

Yup, should've said override instead.

155

We have to follow the libTooling architecture where we have to have a FrontendAction class and an ASTConsumer class with specific callback signatures. I'll try to think of something.

NoQ updated this revision to Diff 319513.Jan 27 2021, 3:01 AM
NoQ marked 22 inline comments as done.

Address review comments!

NoQ added a comment.Jan 27 2021, 3:02 AM

[Warning, Note, Remark, Warning, Remark]

I excluded remarks for now with an assertion. Other tests added :)

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h
54–57

*mind blown*

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
39

Do we emit the [mypackage.mychecker] suffix for all the reports? If not, then we have a problem.

This was supposed to be a workaround for clang-tidy doing this to us. I think it's actually better to teach clang-tidy not to do this when it's pumping output to us. PathDiagnosticConsumers will re-add the checker name when appropriate (eg., in text output but not in plist output).

75–78

With these classes typically there's a bunch of stuff in between, such as Piece->addRange(). Wait a sec, I can actually handle ranges pretty quickly. Let me add them so that this place wasn't empty :D

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
140

We still need to discriminate between FrontendAction mock, ASTConsumer mock, and PathDiagnosticConsumer mock. Dropped some of the nouns though.

alexfh set the repository for this revision to rG LLVM Github Monorepo.Jan 29 2021, 5:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2021, 5:59 AM
Szelethus accepted this revision.Feb 9 2021, 8:39 PM

LGTM! As a side note, are we aware of anyone that uses short messages instead of the full length one?

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
69–70

For later, consider testing and handling -werror and -analyzer-werror.

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
140

Do we need really need the postfix DiagnosticConsumer everywhere? Can't we convert it into a namespace? Maybe at least for PathDiagnosticConsumers? Its not a big deal, and it is certainly descriptive, but looks a bit funny.

Also, RIP ClangDiagPathDiagConsumer.

This revision is now accepted and ready to land.Feb 9 2021, 8:39 PM