Page MenuHomePhabricator

[analyzer] Add fix-it hint support.
ClosedPublic

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

Repository
rL LLVM

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.Jul 29 2019, 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.Aug 9 2019, 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.Aug 9 2019, 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
561 ↗(On Diff #214462)

Show -> Enable?

652 ↗(On Diff #214462)

Same as above.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
308 ↗(On Diff #214462)

Unsinged -> Unsigned

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
103 ↗(On Diff #214462)

FixIt with capital i.

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

Fix un-singed options.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
561 ↗(On Diff #214462)

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 ↗(On Diff #214462)

:D

unsinged, adj., "not popular among singers"

Fxd.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
103 ↗(On Diff #214462)

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.Aug 9 2019, 7:03 PM
Charusso accepted this revision.Aug 9 2019, 7:20 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
103 ↗(On Diff #214462)

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.Aug 13 2019, 7:07 PM

Rebase!

Szelethus accepted this revision.EditedAug 14 2019, 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 ↗(On Diff #215011)

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

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
743 ↗(On Diff #215011)

Shouldn't we assert this?

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
84 ↗(On Diff #215011)

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 ↗(On Diff #215011)

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.Aug 14 2019, 1:21 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
879 ↗(On Diff #215011)

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 ↗(On Diff #215011)

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

clang/test/Analysis/objc-arc.m
1 ↗(On Diff #215011)

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

NoQ updated this revision to Diff 215236.Aug 14 2019, 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.Aug 14 2019, 1:58 PM
Szelethus added inline comments.Aug 14 2019, 2:02 PM
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
879 ↗(On Diff #215011)

Consider my inline void then :)

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
84 ↗(On Diff #215011)

That's at least descriptive, but I agree.

clang/test/Analysis/objc-arc.m
1 ↗(On Diff #215011)

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

NoQ added a comment.Aug 14 2019, 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.Aug 14 2019, 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 ↗(On Diff #215236)

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

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
3100 ↗(On Diff #215236)

const auto&/auto&&?

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
741 ↗(On Diff #215236)

const auto&?

gribozavr added inline comments.Aug 20 2019, 6:09 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
347 ↗(On Diff #215236)

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

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
751 ↗(On Diff #215236)

Escaping?

clang/test/Analysis/dead-stores.c
11 ↗(On Diff #215236)

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

NoQ marked an inline comment as done.Aug 20 2019, 7:27 AM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
347 ↗(On Diff #215236)

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 :)

NoQ updated this revision to Diff 217982.Aug 29 2019, 3:04 PM
NoQ marked 8 inline comments as done.

Fixits ♫ Fix the Fixits ♫
Yabba dabba doo ♫

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
751 ↗(On Diff #215236)

Oh #@&%! Thanks.

clang/test/Analysis/dead-stores.c
11 ↗(On Diff #215236)

I anyway hope that this facility is temporary(c).

gribozavr added inline comments.Sep 2 2019, 4:15 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
350 ↗(On Diff #217982)

No need to qualify with "llvm::".

491 ↗(On Diff #217982)

I'm not sure if this is the right model for fixits. Fixits are usually associated with a message that explains what the fixit does. Only in the unusual case where Clang or ClangTidy is very confident that the fixit is correct, it is attached to the warning. Most commonly, fixits are attached to notes.

Also, for IDE support, it would be really nice if we could provide short descriptions of edits themselves (e.g., "replace 'virtual' with 'override") that can be displayed to the user instead of the diff when possible -- right now we don't and tools using ClangTidy have to use a subpar UI because of that. For example, when we show UI for fixing a warning, displaying the complete diff is too much; a concise description would be a lot better.

gribozavr added inline comments.Sep 2 2019, 4:58 AM
clang/test/Analysis/dead-stores.c
11 ↗(On Diff #215236)

What's the eventual replacement?

NoQ marked an inline comment as done.Sep 3 2019, 2:31 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
491 ↗(On Diff #217982)

Fixits are usually associated with a message that explains what the fixit does. Only in the unusual case where Clang or ClangTidy is very confident that the fixit is correct, it is attached to the warning.

Wait, you guys already have fixits attached to notes? Then, yeah, i need to support this.

Also, for IDE support, it would be really nice if we could provide short descriptions of edits themselves (e.g., "replace 'virtual' with 'override") that can be displayed to the user instead of the diff when possible -- right now we don't and tools using ClangTidy have to use a subpar UI because of that.

Mmm, interesting. I've seen this IDE of ours autogenerate such messages as "replace '$code_in_removed_range' with '$inserted_code'" (while also combining multiple parts of the fixit into a single replacement under the hood) and it looked quite bearable most of the time and i silently assumed that everybody does the same thing.

I agree that a high-level description of a fixit is nice to have. But given that you're attaching fixits to notes, isn't the text of the note text itself sufficient? E.g.:

  • warning: variable may be used uninitialized here
  • note: initialize 'x' here to suppress the warning
    • fixit: add ' = 0' after 'x'
NoQ marked an inline comment as done.Sep 3 2019, 2:34 PM
NoQ added inline comments.
clang/test/Analysis/dead-stores.c
11 ↗(On Diff #215236)

I hope we eventually learn how to apply fixits automatically, and then FileCheck fixed files.

NoQ updated this revision to Diff 218575.Sep 3 2019, 6:40 PM

Make fix-it hints attachable to any PathDiagnosticPiece, rather than to PathDiagnostic as a whole. This basically allows attaching a fixit to any note in the report. For now the plist consumer only supports attaching fixits to the warning itself, to path event pieces, and to extra note pieces; it's completely unclear what's the benefit of attaching fixits to other kinds of pieces.

Rebase after D66733.
Fix escaping again.
Address an inline comment.

gribozavr added inline comments.Sep 6 2019, 6:24 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
491 ↗(On Diff #217982)

Wait, you guys already have fixits attached to notes? Then, yeah, i need to support this.

Yes, fixits are attached to notes in Clang and ClangTidy.

Mmm, interesting. I've seen this IDE of ours autogenerate such messages as "replace '$code_in_removed_range' with '$inserted_code'" (while also combining multiple parts of the fixit into a single replacement under the hood) and it looked quite bearable most of the time and i silently assumed that everybody does the same thing.

We are doing the same thing, however, it only works well when the edit text is meaningful. For example, a hypothetical "replace const with constexpr" edit looks readable. However, "insert '(*' and insert ')'" does not read well, "dereference the pointer" would be better.

I agree that a high-level description of a fixit is nice to have. But given that you're attaching fixits to notes, isn't the text of the note text itself sufficient? E.g.:

Often yes, however not always. Also, fixits can be attached to warnings themselves. For example:

warning: #includes are not sorted according to the style guide
attached fixit: <remove all #includes>, <insert the same #includes in different order>

gribozavr accepted this revision.Sep 6 2019, 6:39 AM
gribozavr added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
343 ↗(On Diff #218575)

Are these fix-its notionally attached to the primary diagnostic itself?

NoQ marked an inline comment as done.Sep 6 2019, 1:36 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
343 ↗(On Diff #218575)

Yup. I'll update the comment.

NoQ marked an inline comment as done.Sep 6 2019, 1:47 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
491 ↗(On Diff #217982)

I see, thanks! It sounds like we might want to extend the FixItHint class itself to include an optional note.

Closed by commit rL371257: [analyzer] Add minimal support for fix-it hints. (authored by dergachev, committed by ). · Explain WhySep 6 2019, 1:54 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 1:54 PM