Page MenuHomePhabricator

[analyzer] Add fix-it hint support.
AcceptedPublic

Authored by NoQ on Jul 23 2019, 6:44 PM.

Details

Summary

That's a pretty wonky experiment, even if a fairly easy one. I'm trying to add support for fix-it hints to the bug reporter. In the current shape the patch does the following:

  • Allow attaching fixit hints to Static Analyzer BugReports.
  • Add support for fixits in text output (including the default text output that goes without notes, as the fixit "belongs" to the warning).
  • Add support for fixits in the plist output mode (not sure if i'm fully supporting all kinds of fixits).
  • Implement a fixit for the path-insensitive DeadStores checker (only one of the cases, and i'm still not sure it's a good fixit, but it was an obvious first thing to experiment with).
  • Implement a fixit for the path-sensitive VirtualCall checker when the virtual method is not pure virtual (in this case the "fix" is to suppress the warning by qualifying the call).
$ clang -cc1 -analyze -analyzer-checker=core,cplusplus,optin.cplusplus test.cpp
test.cpp:4:5: warning: Call to virtual method 'S::foo' during construction
    foo();
    ^~~~~
    S::
1 warning generated.

More TODOs:

  • We don't have a good way to test removal fixits (such as the one in the dead stores checker). Testing plist files is not a good way to test them (though we definitely need a few such tests). We can't test them via text output because clang itself generally doesn't display removal fixits in text output (it's kinda obvious if you look at how it prints them). In clang-tidy they use a more sophisticated facility for these tests, testing the fixed file instead, but it's deep within their custom testing scripts. We might need something similar.
  • HTML output still doesn't support fixits. Not sure if they are useful in there because fixits are generally not very useful when there's no button to apply them. But there should be no harm in displaying them anyway, so i hope i'll have time to take a look.
  • Need more tests with macros and such stuff.

Diff Detail

Event Timeline

NoQ created this revision.Jul 23 2019, 6:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 6:44 PM
NoQ edited the summary of this revision. (Show Details)Jul 23 2019, 6:46 PM
NoQ removed a subscriber: Charusso.
NoQ added a comment.Jul 23 2019, 8:22 PM

Also if we ever implement any fixits for path-sensitive reports that actually make sense, i suspect that it might as well make sense to attach fixits to individual path notes. For examples, if we report a double-free, we may attach a removal of the second free to the warning and a removal of the first free to the note that highlights the first free.

Woah, nice! How does an -analyzer-config fixits-as-warnings option sound like for more readable tests?

01 Obj obj6 = /**/; // expected-warning{{Value stored to 'obj6' during its initialization is never read}}
02                  // expected-warning@-1{{FixIt: Remove range (<filename>:1:1, <filename>:1:16)}}
NoQ updated this revision to Diff 211595.Jul 24 2019, 1:52 PM

Unforget a FileCheck-based test file for the virtual call checker that i already had.

How does an -analyzer-config fixits-as-warnings option sound like for more readable tests?

Yeah, that makes a lot of sense! I guess i'll do fixits-as-remarks instead :D

Hmm, I was thinking on the same for some time but I wonder how many checkers could find the correct fixits? Maybe the removal fixits of double frees or double file closes, but I am afraid that for most of our path-sensitive checks there are no obvious fixits. Even clang-tidy cannot provide a fixit for most of its findings. However, generally I like the idea, even for the few checkers it can be applied to.

NoQ added a comment.Mon, Jul 29, 7:27 PM

Hmm, I was thinking on the same for some time but I wonder how many checkers could find the correct fixits? Maybe the removal fixits of double frees or double file closes, but I am afraid that for most of our path-sensitive checks there are no obvious fixits. Even clang-tidy cannot provide a fixit for most of its findings. However, generally I like the idea, even for the few checkers it can be applied to.

I'd be pretty surprised if any path-sensitive checker would ever have a really good fixit, which is why i never was super excited about this idea when people were asking for it on the mailing lists. This is definitely mostly for syntactic checkers (think MallocSizeof or some of our Objective-C checkers). But at least it's one less artificial roadblock for people choosing where to put their checker :/

The direction looks good to me. I wonder if it is worth to add some warnings somewhere that it is really tricky to come up with reasonable fixits for most path sensitive checks.

NoQ updated this revision to Diff 214462.Fri, Aug 9, 3:18 PM
NoQ retitled this revision from [analyzer] WIP: Add fix-it hint support. to [analyzer] Add fix-it hint support..
  • Hide all current fixits behind a checker option, have them off by default. They're not tested enough yet.
  • Suppress the dead stores fixit when the initializer has a side effect.
  • Implement fixits-as-remarks for testing purposes. This is better than nothing, but i'd rather have something similar to clang-tidy where the test applies fixits and tests the resulting code.
  • Add documentation for BugReport::addFixItHint().

I plan to commit this in the current shape, hence no longer "WIP".

Charusso accepted this revision.Fri, Aug 9, 3:34 PM

I really like that patch, as no human would rewrite 600 dyn_cast and getAs to cast and castAs. Thanks you!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
567

Show -> Enable?

658

Same as above.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
308

Unsinged -> Unsigned

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
103

FixIt with capital i.

This revision is now accepted and ready to land.Fri, Aug 9, 3:34 PM
NoQ updated this revision to Diff 214495.Fri, Aug 9, 7:03 PM
NoQ marked 3 inline comments as done.

Fix un-singed options.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
567

recursion, n., see "recursion"

I think it's valuable when the object and the documentation describe the same idea in different words.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
308

:D

unsinged, adj., "not popular among singers"

Fxd.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
103

It looks as if people write FixIt when they mean "fix-it" as an adjective (eg., "fix-it hint", "fix-it location") but Fixit when they mean "a fixit" as a noun.

NoQ marked an inline comment as done.Fri, Aug 9, 7:03 PM
Charusso accepted this revision.Fri, Aug 9, 7:20 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
103

The name of the stuff is fix-it as properly named in the title of the review and fixit is a total different noun. I do not mind, if we pick fixit, but the tooling will remain named as fix-it.

NoQ updated this revision to Diff 215011.Tue, Aug 13, 7:07 PM

Rebase!

Szelethus accepted this revision.EditedWed, Aug 14, 6:26 AM
Szelethus added subscribers: gribozavr, aaron.ballman, alexfh.

Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config. But I don't insist :)

Now that we proposed that clang-tidy should also use the BugReporter, maybe we should let some clang-tidy folks chip in on this.

In any case, a direction is set and looks great.

clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
879

Hmm, will this return an immutable container? If not, can we make it so?

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
743

Shouldn't we assert this?

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
84

Oh my god. This must be a runner up for "Worst class name in the Clang Static Analyzer", and might even all the trophies home. I wanted to make a joke about honorable mentions, but nothing compares :^)

clang/test/Analysis/objc-arc.m
1

Just thinking aloud, but maybe it'd be time to create a RUN: line formatter for test files, WDYT?

NoQ marked 3 inline comments as done.Wed, Aug 14, 1:21 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
879

ArrayRef is like a view/slice (like StringRef), it's lightweight and you can't mutate the original container through it and it abstracts out various implementation details of the underlying vector.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
84

I'm pretty sure CXX17ElidedCopyConstructorInitializerConstructionContext deserves at least some recognition.

clang/test/Analysis/objc-arc.m
1

Will this formatter be smart enough to update line numbers in expected-plists/objc-arc.m.plist? (:

NoQ updated this revision to Diff 215236.Wed, Aug 14, 1:57 PM

Add a "not implemented yet" assertion for kinds of fixits that are not supported as of that patch (but will definitely need to be supported in the future).

NoQ marked an inline comment as done.Wed, Aug 14, 1:58 PM
Szelethus added inline comments.Wed, Aug 14, 2:02 PM
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
879

Consider my inline void then :)

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
84

That's at least descriptive, but I agree.

clang/test/Analysis/objc-arc.m
1

Why not put that on the wish list, eh? ;)

NoQ added a comment.Wed, Aug 14, 4:05 PM

Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config. But I don't insist :)

I did this because fixits have a much higher quality standard that warnings (because people often apply them blindly). This means that fixit support is likely to be lagging behind in meeting the quality standards and therefore it's important to allow some (but not necessarily all) on-by-default checkers have alpha fixits.

Szelethus accepted this revision.Wed, Aug 14, 4:11 PM
In D65182#1630540, @NoQ wrote:

Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config. But I don't insist :)

I did this because fixits have a much higher quality standard that warnings (because people often apply them blindly). This means that fixit support is likely to be lagging behind in meeting the quality standards and therefore it's important to allow some (but not necessarily all) on-by-default checkers have alpha fixits.

I'm sold.

Hi Artem!
The patch looks very promising for CSA, thanks for working on this!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
343

As I see, FixItHint is a pretty expensive class to copy. Should we consider const ref/move?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
3100

const auto&/auto&&?

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
741

const auto&?

gribozavr added inline comments.Tue, Aug 20, 6:09 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
347

Why is it virtual? In fact, why is BugReporter subclassable at all?

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
751

Escaping?

clang/test/Analysis/dead-stores.c
11

If the tests ignore the line number anyway, why even print it?

NoQ marked an inline comment as done.Tue, Aug 20, 7:27 AM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
347

This clearly doesn't need to be virtual. I copy-pasted it from nearby functions without applying any critical thinking.

However, BugReport(er) most likely needs to be sub-classable because D66473#inline-596021, so i promise to clean those up in the process :)