PList output for clang Tidy

Hello everyone,

The Clang Static analyzer can output the diagnostics in plist format. It is a useful feature, because it is easy to parse that format with 3rd party tools, hence integrating clang tools with others.

Unfortunately the plist reporting format is not supported by Clang Tidy. We would like to add plist support to it. This involves a lot of changes both to the format and the public API, so I want your opinion, how to do it.

In my opinion we need to extend the plist format to:

  • Support notes that are not events

  • Support fixits

What do you think, what would be the bast way to extend the format with those informations?

The plist reporting related functionality is not part of the Clang public API at the moment. The best would be, if the Static Analyzer and regular diagnostics could be reported to the same plist output file. To achieve this, the diagnostic consumer that outputs to the plist should support both PathDiagnostics and regular Diagnostics. It would be redundant to reimplement the whole functionality in Clang Tidy. To reduce the redundancy, we would like to refactor several plist related helper functions out from PlistDiagnostics and make it available in public headers. We would also like to make PlistDiagnostics class available so we can inherit from it. What do you think, what would be the best way to organize these changes?

Thanks,

Gábor Horváth

+chandler and daniel; I think that are all the chefs we need for this particular porridge

+chandler and daniel; I think that are all the chefs we need for this
particular porridge

Hello everyone,

The Clang Static analyzer can output the diagnostics in plist format. It
is a useful feature, because it is easy to parse that format with 3rd party
tools, hence integrating clang tools with others.

Unfortunately the plist reporting format is not supported by Clang Tidy.
We would like to add plist support to it. This involves a lot of changes
both to the format and the public API, so I want your opinion, how to do it.

In my opinion we need to extend the plist format to:
* Support notes that are not events
* Support fixits

What do you think, what would be the bast way to extend the format with
those informations?

The plist reporting related functionality is not part of the Clang public
API at the moment. The best would be, if the Static Analyzer and regular
diagnostics could be reported to the same plist output file. To achieve
this, the diagnostic consumer that outputs to the plist should support both
PathDiagnostics and regular Diagnostics. It would be redundant to
reimplement the whole functionality in Clang Tidy. To reduce the
redundancy, we would like to refactor several plist related helper
functions out from PlistDiagnostics and make it available in public
headers. We would also like to make PlistDiagnostics class available so
we can inherit from it. What do you think, what would be the best way to
organize these changes?

If I remember correctly, Chandler has long ago proposed to merge the
different diagnostic types we have into one central clang diagnostic type
that supports all our use cases.

I personally would need to see a CL to judge whether it makes sense, but
generally, I think what you say sounds like it's the right direction (if
you agree with the sentence above: make clang's basic diagnostic system
powerful enough to support the analyzer's use cases, and then switch the
analyzer and clang-tidy to use it).

In my proposal it would be possible to create a Diag Consumer that can
consume both PathDiagnostics and regular Diagnostics. It would be possible,
to make the same object to consume both Clang and Static Analyzer diags. It
may not be as clean as merging the two kinds of diagnostics, but I suspect
this approach is faster to implement. In the long term I do agree that, it
would be desirable to use the same mechanisms for both Static Analyzer and
Clang. But I am not sure that, I have enough time for a refactoring like
that.

I'd vote for renaming PList to something non-horrible in the process, (I
still have no idea what the "P" stands for), but that's bikeshedding.

I don't know if we could rename that. PList is not Clang specific, Apple
uses this format in other projects as well.

Cheers,
/Manuel

Thanks,
Gábor

  • Ted

+chandler and daniel; I think that are all the chefs we need for this particular porridge

Hello everyone,

The Clang Static analyzer can output the diagnostics in plist format. It is a useful feature, because it is easy to parse that format with 3rd party tools, hence integrating clang tools with others.

Unfortunately the plist reporting format is not supported by Clang Tidy. We would like to add plist support to it. This involves a lot of changes both to the format and the public API, so I want your opinion, how to do it.

In my opinion we need to extend the plist format to:

  • Support notes that are not events

  • Support fixits

What do you think, what would be the bast way to extend the format with those informations?

The plist reporting related functionality is not part of the Clang public API at the moment. The best would be, if the Static Analyzer and regular diagnostics could be reported to the same plist output file. To achieve this, the diagnostic consumer that outputs to the plist should support both PathDiagnostics and regular Diagnostics. It would be redundant to reimplement the whole functionality in Clang Tidy. To reduce the redundancy, we would like to refactor several plist related helper functions out from PlistDiagnostics and make it available in public headers. We would also like to make PlistDiagnostics class available so we can inherit from it. What do you think, what would be the best way to organize these changes?

If I remember correctly, Chandler has long ago proposed to merge the different diagnostic types we have into one central clang diagnostic type that supports all our use cases.

I personally would need to see a CL to judge whether it makes sense, but generally, I think what you say sounds like it’s the right direction (if you agree with the sentence above: make clang’s basic diagnostic system powerful enough to support the analyzer’s use cases, and then switch the analyzer and clang-tidy to use it).

I’d vote for renaming Plist to something non-horrible in the process, (I still have no idea what the “P” stands for), but that’s bikeshedding.

http://en.wikipedia.org/wiki/Property_list
(plist is the specific format widely used at Apple)

  • Ted

+chandler and daniel; I think that are all the chefs we need for this particular porridge

Hello everyone,

The Clang Static analyzer can output the diagnostics in plist format. It is a useful feature, because it is easy to parse that format with 3rd party tools, hence integrating clang tools with others.

Unfortunately the plist reporting format is not supported by Clang Tidy. We would like to add plist support to it. This involves a lot of changes both to the format and the public API, so I want your opinion, how to do it.

In my opinion we need to extend the plist format to:

  • Support notes that are not events

  • Support fixits

What do you think, what would be the bast way to extend the format with those informations?

The plist reporting related functionality is not part of the Clang public API at the moment. The best would be, if the Static Analyzer and regular diagnostics could be reported to the same plist output file. To achieve this, the diagnostic consumer that outputs to the plist should support both PathDiagnostics and regular Diagnostics. It would be redundant to reimplement the whole functionality in Clang Tidy. To reduce the redundancy, we would like to refactor several plist related helper functions out from PlistDiagnostics and make it available in public headers. We would also like to make PlistDiagnostics class available so we can inherit from it. What do you think, what would be the best way to organize these changes?

If I remember correctly, Chandler has long ago proposed to merge the different diagnostic types we have into one central clang diagnostic type that supports all our use cases.

I personally would need to see a CL to judge whether it makes sense, but generally, I think what you say sounds like it’s the right direction (if you agree with the sentence above: make clang’s basic diagnostic system powerful enough to support the analyzer’s use cases, and then switch the analyzer and clang-tidy to use it).

I’d vote for renaming Plist to something non-horrible in the process, (I still have no idea what the “P” stands for), but that’s bikeshedding.

http://en.wikipedia.org/wiki/Property_list

(plist is the specific format widely used at Apple)

Showing my ignorance - thanks for the link, I want aware, that explains a lot…

Yes, Plist goes back to 1986 and in its modern form is just a straight up xml file.

It is cross-platform supported on FreeBSD, Linux and elsewhere. Adding these features, and or making it the same level quality from OS X, iOS, FreeBSD, Linux, etc., via Clang/Clang-Tidy makes a lot of sound sense.

  • Marc

Hi Gábor,

+chandler and daniel; I think that are all the chefs we need for this
particular porridge

Hello everyone,

The Clang Static analyzer can output the diagnostics in plist format.
It is a useful feature, because it is easy to parse that format with 3rd
party tools, hence integrating clang tools with others.

Unfortunately the plist reporting format is not supported by Clang Tidy.
We would like to add plist support to it. This involves a lot of changes
both to the format and the public API, so I want your opinion, how to do it.

In my opinion we need to extend the plist format to:
* Support notes that are not events
* Support fixits

What do you think, what would be the bast way to extend the format with
those informations?

Ted or Anna can say whether it's possible to extend the plist Static
Analyzer output format and what's the best way to do this. Also, as you
noted, most of PlistDiagnostics should be factored out and exposed in a
form of some API to be reusable. This can be done completely independent of
the ClangTidy part of the changes. Ted, Anna and Jordan should be the right
people to send relevant patches to.

The plist reporting related functionality is not part of the Clang public

API at the moment. The best would be, if the Static Analyzer and regular
diagnostics could be reported to the same plist output file. To achieve
this, the diagnostic consumer that outputs to the plist should support both
PathDiagnostics and regular Diagnostics. It would be redundant to
reimplement the whole functionality in Clang Tidy. To reduce the
redundancy, we would like to refactor several plist related helper
functions out from PlistDiagnostics and make it available in public
headers. We would also like to make PlistDiagnostics class available so
we can inherit from it. What do you think, what would be the best way to
organize these changes?

If I remember correctly, Chandler has long ago proposed to merge the
different diagnostic types we have into one central clang diagnostic type
that supports all our use cases.

Chandler, do you have any thoughts on this?

I personally would need to see a CL to judge whether it makes sense, but

generally, I think what you say sounds like it's the right direction (if
you agree with the sentence above: make clang's basic diagnostic system
powerful enough to support the analyzer's use cases, and then switch the
analyzer and clang-tidy to use it).

In my proposal it would be possible to create a Diag Consumer that can
consume both PathDiagnostics and regular Diagnostics. It would be possible,
to make the same object to consume both Clang and Static Analyzer diags.

The problem is not consuming messages from the Static Analyzer in a
different format (ClangTidy already subscribes to PathDiagnostics), but the
fact that ClangTidy uses Clang DiagnosticsEngine internally to handle
diagnostics from the Static Analyzer, from all checks, and from the
compiler. It means that to handle all information from PathDiagnostic we'd
need to either extend Clang's diagnostic subsystem or rewrite a significant
part of ClangTidy to introduce an alternative way to handle PathDiagnostics
inside ClangTidy. It seems to me that the former is a much superior
solution.

In any case we'd also need to extend the ClangTidyError structure (used to
return ClangTidy results to clients) to accommodate additional information
from PathDiagnostic.

It may not be as clean as merging the two kinds of diagnostics, but I

suspect this approach is faster to implement. In the long term I do agree
that, it would be desirable to use the same mechanisms for both Static
Analyzer and Clang. But I am not sure that, I have enough time for a
refactoring like that.

I'd vote for renaming PList to something non-horrible in the process, (I
still have no idea what the "P" stands for), but that's bikeshedding.

I don't know if we could rename that. PList is not Clang specific, Apple
uses this format in other projects as well.

Cheers,
/Manuel

Thanks,
Gábor

Regards,
Alex

Hi,

I had some free time this week and I started to implement a prototype for clang tidy plist output (patches are attached to this mail). In this first attempt I tried to keep clang changes minimal. To achieve this, running clang tidy generates two separate plist output: one for the reports that are generated by the clang static analyzer and one for the reports generated by tidy checkers. I try to make these two kinds of plists as similar as possible. I did not add note and fixit information to the plists yet and the plists which contains reports from tidy checkers do not contain a file list. I choose this separate plist design because the PathDiagnosticConsumers and DiagnosticConsumers have different life times. Supporting one output would add too much extra complexity in my opinion (unless the clang diagnostic engine learns to handle path diagnostics).

What is your opinion about the design? Should I continue the implementation or should I change the design?

Regards,

Gábor

TidyPlist_ClangPart.patch (3.22 KB)

TidyPlist.patch (15.7 KB)

Hi Gåbor,

The problem with this approach is that it results in a significant code duplication:

  • the whole error filtering logic will need to be duplicated for the AnalyzerDiagnosticConsumer that handles plist output (note that diagnostic filtering won’t work for plist output of analyzer results in your patch). And it seems that it will require making significantly more changes to ento::PlistDiagnostics than just making it public and inheriting from it.
  • plist-formatting the output of non-analyzer diagnostics to a certain degree duplicates what is being done in PlistDiagnostics (and duplication will likely increase as the output formats for these two kinds of diagnostics are brought closer together).

As I’ve noted earlier, extending DiagnosticsEngine would be one possible solution (probably not the easiest one, though). Maybe pulling out some reusable parts will allow to reduce code duplication while basically having the same design as you propose. I suspect however that the former approach can result in a more maintainable code.

P.S. I would appreciate if you sent patches via Phabricator for a more convenient review. Thanks!

Hi Alex,

I agree that this involves significant amount of code duplication. I also agree that extending DiagnosticsEngine would be a cleaner solution (however that would require massive amount of modification through the clang and static analyzer codebase). If you think a patch with similar design may be accepted I will start sending patches via Phabricator.

I see the following options:

  • Clean up this patch and attempt to make it accepted to support plists and extend the DiagnosticsEngine later.

  • Clean up this patch and attempt to make it accepted to support plists and do not bother with DiagnosticEngine for now.

  • Drop this design and start to extend DiagnosticEngine. Add plist support later.

Which option do you suggest?

Thanks,

Gábor