[analyzer] Add fix-it hint support.

[NoQ]

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.

[NoQ]

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.

[Szelethus]

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]

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

In D65182#1598614, @Szelethus wrote:

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

[baloghadamsoftware]

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]

In D65182#1604280, @baloghadamsoftware wrote:

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

[xazax.hun]

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]

• 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]

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.

[NoQ]

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.

[Charusso]

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]

Rebase!

[Szelethus]

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]

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 [[ https://clang.llvm.org/doxygen/classclang_1_1CXX17ElidedCopyConstructorInitializerConstructionContext.html | 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]

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

[Szelethus]

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]

In D65182#1629192, @Szelethus 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.

[Szelethus]

In D65182#1630540, @NoQ wrote:

In D65182#1629192, @Szelethus 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.

[a_sidorin]

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]

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]

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]

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]

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]

clang/test/Analysis/dead-stores.c

11 ↗

(On Diff #215236)

What's the eventual replacement?

[NoQ]

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]

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]

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]

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]

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

343 ↗

(On Diff #218575)

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

[NoQ]

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

343 ↗

(On Diff #218575)

Yup. I'll update the comment.

[NoQ]

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.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

CmdLineOption<Boolean,
                   InAlpha>
   ]>,
   Documentation<HasAlphaDocumentation>;
 
 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction/destruction">,
   CheckerOptions<[
     CmdLineOption<Boolean,
+                  "ShowFixIts",
+                  "Enable fix-it hints for this checker",
+                  "false",
+                  InAlpha>,
+    CmdLineOption<Boolean,
                   "PureOnly",
                   "Disables the checker. Keeps cplusplus.PureVirtualCall "
                   "enabled. This option is only provided for backwards "
                   "compatibility.",
                   "false",
                   InAlpha>
   ]>,
   Dependencies<[VirtualCallModeling]>,
def DeadStoresChecker : Checker<"DeadStores">,
   HelpText<"Check for values stored to variables that are never read "
            "afterwards">,
   CheckerOptions<[
     CmdLineOption<Boolean,
                   "WarnForDeadNestedAssignments",
                   "Warns for deadstores in nested assignments."
                   "E.g.: if ((P = f())) where P is unused.",
                   "true",
-                  Released>
+                  Released>,
+    CmdLineOption<Boolean,
+                  "ShowFixIts",
+                  "Enable fix-it hints for this checker",
+                  "false",
+                  InAlpha>
   ]>,
   Documentation<HasDocumentation>;
 
 } // end DeadCode
 
 let ParentPackage = DeadCodeAlpha in {
 
 def UnreachableCodeChecker : Checker<"UnreachableCode">,

cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

ANALYZER_OPTION(bool, ShouldTrackConditions, "track-conditions",
                 "Whether to track conditions that are a control dependency of "
                 "an already tracked variable.",
                 true)
 
 ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug",
                 "Whether to place an event at each tracked condition.",
                 false)
 
+ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks",
+                "Emit fix-it hints as remarks for testing purposes",
+                false)
+
+//===----------------------------------------------------------------------===//
+// Unsigned analyzer options.
+//===----------------------------------------------------------------------===//
+
 ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
                 "The maximal amount of translation units that is considered "
                 "for import when inlining functions during CTU analysis. "
                 "Lowering this threshold can alleviate the memory burder of "
                 "analysis with many interdependent definitions located in "
                 "various translation units.",
                 100u)
 
-//===----------------------------------------------------------------------===//
-// Unsinged analyzer options.
-//===----------------------------------------------------------------------===//
-
 ANALYZER_OPTION(
     unsigned, AlwaysInlineSize, "ipa-always-inline-size",
     "The size of the functions (in basic blocks), which should be considered "
     "to be small enough to always inline.",
     3)
 
 ANALYZER_OPTION(
     unsigned, GraphTrimInterval, "graph-trim-interval",

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

protected:
   PathDiagnosticLocation Location;
   PathDiagnosticLocation UniqueingLocation;
   const Decl *UniqueingDecl;
 
   const ExplodedNode *ErrorNode = nullptr;
   SmallVector<SourceRange, 4> Ranges;
   const SourceRange ErrorNodeRange;
   NoteList Notes;
+  SmallVector<FixItHint, 4> Fixits;
 
   /// A (stack of) a set of symbols that are registered with this
   /// report as being "interesting", and thus used to help decide which
   /// diagnostics to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
   llvm::DenseMap<SymbolRef, bugreporter::TrackingKind> InterestingSymbols;
 
public:
   }
 
   /// Add new item to the list of additional notes that need to be attached to
   /// this path-insensitive report. If you want to add extra notes to a
   /// path-sensitive report, you need to use a BugReporterVisitor because it
   /// allows you to specify where exactly in the auto-generated path diagnostic
   /// the extra note should appear.
   void addNote(StringRef Msg, const PathDiagnosticLocation &Pos,
-               ArrayRef<SourceRange> Ranges) {
+               ArrayRef<SourceRange> Ranges = {},
+               ArrayRef<FixItHint> Fixits = {}) {
     auto P = std::make_shared<PathDiagnosticNotePiece>(Pos, Msg);
 
     for (const auto &R : Ranges)
       P->addRange(R);
 
-    Notes.push_back(std::move(P));
-  }
+    for (const auto &F : Fixits)
+      P->addFixit(F);
 
-  // FIXME: Instead of making an override, we could have default-initialized
-  // Ranges with {}, however it crashes the MSVC 2013 compiler.
-  void addNote(StringRef Msg, const PathDiagnosticLocation &Pos) {
-    std::vector<SourceRange> Ranges;
-    addNote(Msg, Pos, Ranges);
+    Notes.push_back(std::move(P));
   }
 
   virtual const NoteList &getNotes() {
     return Notes;
   }
 
   /// Return the "definitive" location of the reported bug.
   ///
public:
 
   /// Get the declaration containing the uniqueing location.
   const Decl *getUniqueingDecl() const {
     return UniqueingDecl;
   }
 
   const Stmt *getStmt() const;
 
-  /// Add a range to a bug report.
+  /// Add a range to the bug report.
   ///
   /// Ranges are used to highlight regions of interest in the source code.
   /// They should be at the same source code line as the BugReport location.
   /// By default, the source range of the statement corresponding to the error
   /// node will be used; add a single invalid range to specify absence of
   /// ranges.
   void addRange(SourceRange R) {
     assert((R.isValid() || Ranges.empty()) && "Invalid range can only be used "
                            "to specify that the report does not have a range.");
     Ranges.push_back(R);
   }
 
   /// Get the SourceRanges associated with the report.
   virtual llvm::iterator_range<ranges_iterator> getRanges() const;
 
+  /// Add a fix-it hint to the warning message of the bug report.
+  ///
+  /// Fix-it hints are the suggested edits to the code that would resolve
+  /// the problem explained by the bug report. Fix-it hints should be
+  /// as conservative as possible because it is not uncommon for the user
+  /// to blindly apply all fixits to their project. It usually is very hard
+  /// to produce a good fix-it hint for most path-sensitive warnings.
+  /// Fix-it hints can also be added to notes through the addNote() interface.
+  void addFixItHint(const FixItHint &F) {
+    Fixits.push_back(F);
+  }
+
+  ArrayRef<FixItHint> getFixits() const { return Fixits; }
+
   /// Add custom or predefined bug report visitors to this report.
   ///
   /// The visitors should be used when the default trace is not sufficient.
   /// For example, they allow constructing a more elaborate trace.
   /// \sa registerConditionVisitor(), registerTrackNullOrUndefValue(),
   /// registerFindLastStore(), registerNilReceiverVisitor(), and
   /// registerVarDeclsLastStore().
   void addVisitor(std::unique_ptr<BugReporterVisitor> visitor);
public:
   /// The reports are usually generated by the checkers. Further, they are
   /// folded based on the profile value, which is done to coalesce similar
   /// reports.
   void emitReport(std::unique_ptr<BugReport> R);
 
   void EmitBasicReport(const Decl *DeclWithIssue, const CheckerBase *Checker,
                        StringRef BugName, StringRef BugCategory,
                        StringRef BugStr, PathDiagnosticLocation Loc,
-                       ArrayRef<SourceRange> Ranges = None);
+                       ArrayRef<SourceRange> Ranges = None,
+                       ArrayRef<FixItHint> Fixits = None);
 
   void EmitBasicReport(const Decl *DeclWithIssue, CheckName CheckName,
                        StringRef BugName, StringRef BugCategory,
                        StringRef BugStr, PathDiagnosticLocation Loc,
-                       ArrayRef<SourceRange> Ranges = None);
+                       ArrayRef<SourceRange> Ranges = None,
+                       ArrayRef<FixItHint> Fixits = None);
 
 private:
   llvm::StringMap<BugType *> StrBugTypes;
 
   /// Returns a BugType that is associated with the given name and
   /// category.
   BugType *getBugTypeForName(CheckName CheckName, StringRef name,
                              StringRef category);

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

private:
 
   /// A constant string that can be used to tag the PathDiagnosticPiece,
   /// typically with the identification of the creator.  The actual pointer
   /// value is meant to be an identifier; the string itself is useful for
   /// debugging.
   StringRef Tag;
 
   std::vector<SourceRange> ranges;
+  std::vector<FixItHint> fixits;
 
 protected:
   PathDiagnosticPiece(StringRef s, Kind k, DisplayHint hint = Below);
   PathDiagnosticPiece(Kind k, DisplayHint hint = Below);
 
 public:
   PathDiagnosticPiece() = delete;
   PathDiagnosticPiece(const PathDiagnosticPiece &) = delete;
public:
   }
 
   void addRange(SourceLocation B, SourceLocation E) {
     if (!B.isValid() || !E.isValid())
       return;
     ranges.push_back(SourceRange(B,E));
   }
 
+  void addFixit(FixItHint F) {
+    fixits.push_back(F);
+  }
+
   /// Return the SourceRanges associated with this PathDiagnosticPiece.
   ArrayRef<SourceRange> getRanges() const { return ranges; }
 
+  /// Return the fix-it hints associated with this PathDiagnosticPiece.
+  ArrayRef<FixItHint> getFixits() const { return fixits; }
+
   virtual void Profile(llvm::FoldingSetNodeID &ID) const;
 
   void setAsLastInMainSourceFile() {
     LastInMainSourceFile = true;
   }
 
   bool isLastInMainSourceFile() const {
     return LastInMainSourceFile;

cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

 //==- DeadStoresChecker.cpp - Check for stores to dead variables -*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines a DeadStores, a flow-sensitive checker that looks for
 //  stores to variables that are no longer live.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Lex/Lexer.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
if (BO->getOpcode() == BO_Comma) {
       continue;
     }
     break;
   }
   return Ex;
 }
 
 namespace {
+class DeadStoresChecker : public Checker<check::ASTCodeBody> {
+public:
+  bool ShowFixIts = false;
+  bool WarnForDeadNestedAssignments = true;
+
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const;
+};
+
 class DeadStoreObs : public LiveVariables::Observer {
   const CFG &cfg;
   ASTContext &Ctx;
   BugReporter& BR;
-  const CheckerBase *Checker;
+  const DeadStoresChecker *Checker;
   AnalysisDeclContext* AC;
   ParentMap& Parents;
   llvm::SmallPtrSet<const VarDecl*, 20> Escaped;
   std::unique_ptr<ReachableCode> reachableCode;
   const CFGBlock *currentBlock;
   std::unique_ptr<llvm::DenseSet<const VarDecl *>> InEH;
-  const bool WarnForDeadNestedAssignments;
 
   enum DeadStoreKind { Standard, Enclosing, DeadIncrement, DeadInit };
 
 public:
   DeadStoreObs(const CFG &cfg, ASTContext &ctx, BugReporter &br,
-               const CheckerBase *checker, AnalysisDeclContext *ac,
+               const DeadStoresChecker *checker, AnalysisDeclContext *ac,
                ParentMap &parents,
                llvm::SmallPtrSet<const VarDecl *, 20> &escaped,
                bool warnForDeadNestedAssignments)
       : cfg(cfg), Ctx(ctx), BR(br), Checker(checker), AC(ac), Parents(parents),
-        Escaped(escaped), currentBlock(nullptr),
-        WarnForDeadNestedAssignments(warnForDeadNestedAssignments) {}
+        Escaped(escaped), currentBlock(nullptr) {}
 
   ~DeadStoreObs() override {}
 
   bool isLive(const LiveVariables::LivenessValues &Live, const VarDecl *D) {
     if (Live.isLive(D))
       return true;
     // Lazily construct the set that records which VarDecls are in
     // EH code.
void Report(const VarDecl *V, DeadStoreKind dsk,
 
     if (isSuppressed(R))
       return;
 
     SmallString<64> buf;
     llvm::raw_svector_ostream os(buf);
     const char *BugType = nullptr;
 
+    SmallVector<FixItHint, 1> Fixits;
+
     switch (dsk) {
-      case DeadInit:
+      case DeadInit: {
         BugType = "Dead initialization";
         os << "Value stored to '" << *V
            << "' during its initialization is never read";
+
+        ASTContext &ACtx = V->getASTContext();
+        if (Checker->ShowFixIts) {
+          if (V->getInit()->HasSideEffects(ACtx,
+                                           /*IncludePossibleEffects=*/true)) {
             break;
+          }
+          SourceManager &SM = ACtx.getSourceManager();
+          const LangOptions &LO = ACtx.getLangOpts();
+          SourceLocation L1 =
+              Lexer::findNextToken(
+                  V->getTypeSourceInfo()->getTypeLoc().getEndLoc(),
+                  SM, LO)->getEndLoc();
+          SourceLocation L2 =
+              Lexer::getLocForEndOfToken(V->getInit()->getEndLoc(), 1, SM, LO);
+          Fixits.push_back(FixItHint::CreateRemoval({L1, L2}));
+        }
+        break;
+      }
 
       case DeadIncrement:
         BugType = "Dead increment";
         LLVM_FALLTHROUGH;
       case Standard:
         if (!BugType) BugType = "Dead assignment";
         os << "Value stored to '" << *V << "' is never read";
         break;
 
       // eg.: f((x = foo()))
       case Enclosing:
-        if (!WarnForDeadNestedAssignments)
+        if (!Checker->WarnForDeadNestedAssignments)
           return;
         BugType = "Dead nested assignment";
         os << "Although the value stored to '" << *V
            << "' is used in the enclosing expression, the value is never "
               "actually read from '"
            << *V << "'";
         break;
     }
 
     BR.EmitBasicReport(AC->getDecl(), Checker, BugType, "Dead store", os.str(),
-                       L, R);
+                       L, R, Fixits);
   }
 
   void CheckVarDecl(const VarDecl *VD, const Expr *Ex, const Expr *Val,
                     DeadStoreKind dsk,
                     const LiveVariables::LivenessValues &Live) {
 
     if (!VD->hasLocalStorage())
       return;

 };
 } // end anonymous namespace
 
 
 //===----------------------------------------------------------------------===//
 // DeadStoresChecker
 //===----------------------------------------------------------------------===//
 
-namespace {
-class DeadStoresChecker : public Checker<check::ASTCodeBody> {
-public:
-  bool WarnForDeadNestedAssignments = true;
-
-  void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
+void DeadStoresChecker::checkASTCodeBody(const Decl *D, AnalysisManager &mgr,
                                          BugReporter &BR) const {
 
-    // Don't do anything for template instantiations.
-    // Proving that code in a template instantiation is "dead"
-    // means proving that it is dead in all instantiations.
-    // This same problem exists with -Wunreachable-code.
-    if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
-      if (FD->isTemplateInstantiation())
-        return;
 
-    if (LiveVariables *L = mgr.getAnalysis<LiveVariables>(D)) {
-      CFG &cfg = *mgr.getCFG(D);
-      AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D);
-      ParentMap &pmap = mgr.getParentMap(D);
-      FindEscaped FS;
-      cfg.VisitBlockStmts(FS);
-      DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped,
-                     WarnForDeadNestedAssignments);
-      L->runOnAllBlocks(A);
-    }
-  }
-};
-}
 
 void ento::registerDeadStoresChecker(CheckerManager &Mgr) {
-  auto Chk = Mgr.registerChecker<DeadStoresChecker>();
+  auto *Chk = Mgr.registerChecker<DeadStoresChecker>();
 
   const AnalyzerOptions &AnOpts = Mgr.getAnalyzerOptions();
   Chk->WarnForDeadNestedAssignments =
       AnOpts.getCheckerBooleanOption(Chk, "WarnForDeadNestedAssignments");
+  Chk->ShowFixIts =
+      AnOpts.getCheckerBooleanOption(Chk, "ShowFixIts");
 }
 
 bool ento::shouldRegisterDeadStoresChecker(const LangOptions &LO) {
   return true;
 }

cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

 } // end namespace llvm
 
 namespace {
 class VirtualCallChecker
     : public Checker<check::BeginFunction, check::EndFunction, check::PreCall> {
 public:
   // These are going to be null if the respective check is disabled.
   mutable std::unique_ptr<BugType> BT_Pure, BT_Impure;
+  bool ShowFixIts = false;
 
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
 private:
   void registerCtorDtorCallInState(bool IsBeginFunction,
                                    CheckerContext &C) const;
void VirtualCallChecker::checkPreCall(const CallEvent &Call,
 
   const std::unique_ptr<BugType> &BT = IsPure ? BT_Pure : BT_Impure;
   if (!BT) {
     // The respective check is disabled.
     return;
   }
 
   auto Report = std::make_unique<BugReport>(*BT, OS.str(), N);
+
+  if (ShowFixIts && !IsPure) {
+    // FIXME: These hints are valid only when the virtual call is made
+    // directly from the constructor/destructor. Otherwise the dispatch
+    // will work just fine from other callees, and the fix may break
+    // the otherwise correct program.
+    FixItHint Fixit = FixItHint::CreateInsertion(
+        CE->getBeginLoc(), MD->getParent()->getNameAsString() + "::");
+    Report->addFixItHint(Fixit);
+  }
+
   C.emitReport(std::move(Report));
 }
 
 void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction,
                                                      CheckerContext &C) const {
   const auto *LCtx = C.getLocationContext();
   const auto *MD = dyn_cast_or_null<CXXMethodDecl>(LCtx->getDecl());
   if (!MD)

 
 void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
   auto *Chk = Mgr.getChecker<VirtualCallChecker>();
   if (!Mgr.getAnalyzerOptions().getCheckerBooleanOption(
           Mgr.getCurrentCheckName(), "PureOnly")) {
     Chk->BT_Impure = std::make_unique<BugType>(
         Mgr.getCurrentCheckName(), "Unexpected loss of virtual dispatch",
         categories::CXXObjectLifecycle);
+    Chk->ShowFixIts = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+        Mgr.getCurrentCheckName(), "ShowFixIts");
   }
 }
 
 bool ento::shouldRegisterVirtualCallModeling(const LangOptions &LO) {
   return LO.CPlusPlus;
 }
 
 bool ento::shouldRegisterPureVirtualCallChecker(const LangOptions &LO) {
   return LO.CPlusPlus;
 }
 
 bool ento::shouldRegisterVirtualCallChecker(const LangOptions &LO) {
   return LO.CPlusPlus;
 }

cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp

if (getAnalyzerOptions().ShouldDisplayNotesAsEvents) {
         Pieces.push_front(std::move(ConvertedPiece));
       }
     } else {
       for (auto I = report->getNotes().rbegin(),
            E = report->getNotes().rend(); I != E; ++I)
         Pieces.push_front(*I);
     }
 
+    for (const auto &I : report->getFixits())
+      Pieces.back()->addFixit(I);
+
     updateExecutedLinesWithDiagnosticPieces(*PD);
     Consumer->HandlePathDiagnostic(std::move(PD));
   }
 }
 
 /// Insert all lines participating in the function signature \p Signature
 /// into \p ExecutedLines.
 static void populateExecutedLinesWithFunctionSignature(
BugReporter::generateDiagnosticForConsumerMap(
   for (auto const &P : *Out)
     if (Opts.ShouldReportIssuesInMainSourceFile && !Opts.AnalyzeAll)
       P.second->resetDiagnosticLocationToMainFile();
 
   return Out;
 }
 
 void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
-                                  const CheckerBase *Checker,
-                                  StringRef Name, StringRef Category,
-                                  StringRef Str, PathDiagnosticLocation Loc,
-                                  ArrayRef<SourceRange> Ranges) {
+                                  const CheckerBase *Checker, StringRef Name,
+                                  StringRef Category, StringRef Str,
+                                  PathDiagnosticLocation Loc,
+                                  ArrayRef<SourceRange> Ranges,
+                                  ArrayRef<FixItHint> Fixits) {
   EmitBasicReport(DeclWithIssue, Checker->getCheckName(), Name, Category, Str,
-                  Loc, Ranges);
+                  Loc, Ranges, Fixits);
 }
 
 void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
                                   CheckName CheckName,
                                   StringRef name, StringRef category,
                                   StringRef str, PathDiagnosticLocation Loc,
-                                  ArrayRef<SourceRange> Ranges) {
+                                  ArrayRef<SourceRange> Ranges,
+                                  ArrayRef<FixItHint> Fixits) {
   // 'BT' is owned by BugReporter.
   BugType *BT = getBugTypeForName(CheckName, name, category);
   auto R = std::make_unique<BugReport>(*BT, str, Loc);
   R->setDeclWithIssue(DeclWithIssue);
-  for (ArrayRef<SourceRange>::iterator I = Ranges.begin(), E = Ranges.end();
-       I != E; ++I)
-    R->addRange(*I);
+  for (const auto &SR : Ranges)
+    R->addRange(SR);
+  for (const auto &FH : Fixits)
+    R->addFixItHint(FH);
   emitReport(std::move(R));
 }
 
 BugType *BugReporter::getBugTypeForName(CheckName CheckName, StringRef name,
                                         StringRef category) {
   SmallString<136> fullDesc;
   llvm::raw_svector_ostream(fullDesc) << CheckName.getName() << ":" << name
                                       << ":" << category;
   BugType *&BT = StrBugTypes[fullDesc];
   if (!BT)
     BT = new BugType(CheckName, name, category);
   return BT;
 }

cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

switch (P.getKind()) {
         ReportPopUp(o, cast<PathDiagnosticPopUpPiece>(P), indent);
         break;
     }
   }
 
   void EmitRanges(raw_ostream &o, const ArrayRef<SourceRange> Ranges,
                   unsigned indent);
   void EmitMessage(raw_ostream &o, StringRef Message, unsigned indent);
+  void EmitFixits(raw_ostream &o, ArrayRef<FixItHint> fixits, unsigned indent);
 
   void ReportControlFlow(raw_ostream &o,
                          const PathDiagnosticControlFlowPiece& P,
                          unsigned indent);
   void ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
                    unsigned indent, unsigned depth, bool isKeyEvent = false);
   void ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P,
                   unsigned indent, unsigned depth);
void PlistPrinter::EmitMessage(raw_ostream &o, StringRef Message,
 
   // Output the short text.
   // FIXME: Really use a short string.
   Indent(o, indent) << "<key>message</key>\n";
   Indent(o, indent);
   EmitString(o, Message) << '\n';
 }
 
+void PlistPrinter::EmitFixits(raw_ostream &o, ArrayRef<FixItHint> fixits,
+                              unsigned indent) {
+  if (fixits.size() == 0)
+    return;
+
+  const SourceManager &SM = PP.getSourceManager();
+  const LangOptions &LangOpts = PP.getLangOpts();
+
+  Indent(o, indent) << "<key>fixits</key>\n";
+  Indent(o, indent) << "<array>\n";
+  for (const auto &fixit : fixits) {
+    assert(!fixit.isNull());
+    // FIXME: Add support for InsertFromRange and BeforePreviousInsertion.
+    assert(!fixit.InsertFromRange.isValid() && "Not implemented yet!");
+    assert(!fixit.BeforePreviousInsertions && "Not implemented yet!");
+    Indent(o, indent) << " <dict>\n";
+    Indent(o, indent) << "  <key>remove_range</key>\n";
+    EmitRange(o, SM, Lexer::getAsCharRange(fixit.RemoveRange, SM, LangOpts),
+              FM, indent + 2);
+    Indent(o, indent) << "  <key>insert_string</key>";
+    EmitString(o, fixit.CodeToInsert);
+    o << "\n";
+    Indent(o, indent) << " </dict>\n";
+  }
+  Indent(o, indent) << "</array>\n";
+}
+
 void PlistPrinter::ReportControlFlow(raw_ostream &o,
                                      const PathDiagnosticControlFlowPiece& P,
                                      unsigned indent) {
 
   const SourceManager &SM = PP.getSourceManager();
   const LangOptions &LangOpts = PP.getLangOpts();
 
   Indent(o, indent) << "<dict>\n";
void PlistPrinter::ReportControlFlow(raw_ostream &o,
 
   // Output any helper text.
   const auto &s = P.getString();
   if (!s.empty()) {
     Indent(o, indent) << "<key>alternate</key>";
     EmitString(o, s) << '\n';
   }
 
+  assert(P.getFixits().size() == 0 &&
+         "Fixits on constrol flow pieces are not implemented yet!");
+
   --indent;
   Indent(o, indent) << "</dict>\n";
 }
 
 void PlistPrinter::ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
                                unsigned indent, unsigned depth,
                                bool isKeyEvent) {
 
void PlistPrinter::ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
 
   // Output the call depth.
   Indent(o, indent) << "<key>depth</key>";
   EmitInteger(o, depth) << '\n';
 
   // Output the text.
   EmitMessage(o, P.getString(), indent);
 
+  // Output the fixits.
+  EmitFixits(o, P.getFixits(), indent);
+
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
 void PlistPrinter::ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P,
                               unsigned indent,
                               unsigned depth) {
void PlistPrinter::ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P,
 
   for (PathPieces::const_iterator I = P.path.begin(), E = P.path.end();I!=E;++I)
     ReportPiece(o, **I, indent, depth, /*includeControlFlow*/ true);
 
   --depth;
 
   if (auto callExit = P.getCallExitEvent())
     ReportPiece(o, *callExit, indent, depth, /*includeControlFlow*/ true);
+
+  assert(P.getFixits().size() == 0 &&
+         "Fixits on call pieces are not implemented yet!");
 }
 
 void PlistPrinter::ReportMacroSubPieces(raw_ostream &o,
                                         const PathDiagnosticMacroPiece& P,
                                         unsigned indent, unsigned depth) {
   MacroPieces.push_back(&P);
 
   for (PathPieces::const_iterator I = P.subPieces.begin(),
                                   E = P.subPieces.end();
        I != E; ++I) {
     ReportPiece(o, **I, indent, depth, /*includeControlFlow*/ false);
   }
+
+  assert(P.getFixits().size() == 0 &&
+         "Fixits on constrol flow pieces are not implemented yet!");
 }
 
 void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
 
   for (const PathDiagnosticMacroPiece *P : MacroPieces) {
     const SourceManager &SM = PP.getSourceManager();
     ExpansionInfo EI = getExpandedMacro(P->getLocation().asLocation(), PP, CTU);
 
void PlistPrinter::ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
 
   // Output the ranges (if any).
   ArrayRef<SourceRange> Ranges = P.getRanges();
   EmitRanges(o, Ranges, indent);
 
   // Output the text.
   EmitMessage(o, P.getString(), indent);
 
+  // Output the fixits.
+  EmitFixits(o, P.getFixits(), indent);
+
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
 void PlistPrinter::ReportPopUp(raw_ostream &o,
                                const PathDiagnosticPopUpPiece &P,
                                unsigned indent) {
void PlistPrinter::ReportPopUp(raw_ostream &o,
 
   // Output the ranges (if any).
   ArrayRef<SourceRange> Ranges = P.getRanges();
   EmitRanges(o, Ranges, indent);
 
   // Output the text.
   EmitMessage(o, P.getString(), indent);
 
+  assert(P.getFixits().size() == 0 &&
+         "Fixits on pop-up pieces are not implemented yet!");
+
   // Finish up.
   --indent;
   Indent(o, indent) << "</dict>\n";
 }
 
 //===----------------------------------------------------------------------===//
 // Static function definitions.
 //===----------------------------------------------------------------------===//

cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

void ento::createTextPathDiagnosticConsumer(
     const std::string &Prefix, const clang::Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
   llvm_unreachable("'text' consumer should be enabled on ClangDiags");
 }
 
 namespace {
 class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
   DiagnosticsEngine &Diag;
-  bool IncludePath, ShouldEmitAsError;
+  bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false;
 
 public:
   ClangDiagPathDiagConsumer(DiagnosticsEngine &Diag)
-      : Diag(Diag), IncludePath(false), ShouldEmitAsError(false) {}
+      : Diag(Diag) {}
   ~ClangDiagPathDiagConsumer() override {}
   StringRef getName() const override { return "ClangDiags"; }
 
   bool supportsLogicalOpControlFlow() const override { return true; }
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
     return IncludePath ? Minimal : None;
   }
 
-  void enablePaths() {
-    IncludePath = true;
-  }
-
+  void enablePaths() { IncludePath = true; }
   void enableWerror() { ShouldEmitAsError = true; }
+  void enableFixitsAsRemarks() { FixitsAsRemarks = true; }
 
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
     unsigned WarnID =
         ShouldEmitAsError
             ? Diag.getCustomDiagID(DiagnosticsEngine::Error, "%0")
             : Diag.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
     unsigned NoteID = Diag.getCustomDiagID(DiagnosticsEngine::Note, "%0");
+    unsigned RemarkID = Diag.getCustomDiagID(DiagnosticsEngine::Remark, "%0");
+
+    auto reportPiece =
+        [&](unsigned ID, SourceLocation Loc, StringRef String,
+            ArrayRef<SourceRange> Ranges, ArrayRef<FixItHint> Fixits) {
+          if (!FixitsAsRemarks) {
+            Diag.Report(Loc, ID) << String << Ranges << Fixits;
+          } else {
+            Diag.Report(Loc, ID) << String << Ranges;
+            for (const FixItHint &Hint : Fixits) {
+              SourceManager &SM = Diag.getSourceManager();
+              llvm::SmallString<128> Str;
+              llvm::raw_svector_ostream OS(Str);
+              // FIXME: Add support for InsertFromRange and
+              // BeforePreviousInsertion.
+              assert(!Hint.InsertFromRange.isValid() && "Not implemented yet!");
+              assert(!Hint.BeforePreviousInsertions && "Not implemented yet!");
+              OS << SM.getSpellingColumnNumber(Hint.RemoveRange.getBegin())
+                 << "-" << SM.getSpellingColumnNumber(Hint.RemoveRange.getEnd())
+                 << ": '" << Hint.CodeToInsert << "'";
+              Diag.Report(Loc, RemarkID) << OS.str();
+            }
+          }
+        };
 
-    for (std::vector<const PathDiagnostic*>::iterator I = Diags.begin(),
-         E = Diags.end(); I != E; ++I) {
+    for (std::vector<const PathDiagnostic *>::iterator I = Diags.begin(),
+         E = Diags.end();
+         I != E; ++I) {
       const PathDiagnostic *PD = *I;
-      SourceLocation WarnLoc = PD->getLocation().asLocation();
-      Diag.Report(WarnLoc, WarnID) << PD->getShortDescription()
-                                   << PD->path.back()->getRanges();
+      reportPiece(WarnID, PD->getLocation().asLocation(),
+                  PD->getShortDescription(), PD->path.back()->getRanges(),
+                  PD->path.back()->getFixits());
 
       // First, add extra notes, even if paths should not be included.
       for (const auto &Piece : PD->path) {
         if (!isa<PathDiagnosticNotePiece>(Piece.get()))
           continue;
 
-        SourceLocation NoteLoc = Piece->getLocation().asLocation();
-        Diag.Report(NoteLoc, NoteID) << Piece->getString()
-                                     << Piece->getRanges();
+        reportPiece(NoteID, Piece->getLocation().asLocation(),
+                    Piece->getString(), Piece->getRanges(), Piece->getFixits());
       }
 
       if (!IncludePath)
         continue;
 
       // Then, add the path notes if necessary.
       PathPieces FlatPath = PD->path.flatten(/*ShouldFlattenMacros=*/true);
       for (const auto &Piece : FlatPath) {
         if (isa<PathDiagnosticNotePiece>(Piece.get()))
           continue;
 
-        SourceLocation NoteLoc = Piece->getLocation().asLocation();
-        Diag.Report(NoteLoc, NoteID) << Piece->getString()
-                                     << Piece->getRanges();
+        reportPiece(NoteID, Piece->getLocation().asLocation(),
+                    Piece->getString(), Piece->getRanges(), Piece->getFixits());
       }
     }
   }
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // AnalysisConsumer declaration.
if (Opts->AnalysisDiagOpt != PD_NONE) {
       // Create the PathDiagnosticConsumer.
       ClangDiagPathDiagConsumer *clangDiags =
           new ClangDiagPathDiagConsumer(PP.getDiagnostics());
       PathConsumers.push_back(clangDiags);
 
       if (Opts->AnalyzerWerror)
         clangDiags->enableWerror();
 
+      if (Opts->ShouldEmitFixItHintsAsRemarks)
+        clangDiags->enableFixitsAsRemarks();
+
       if (Opts->AnalysisDiagOpt == PD_TEXT) {
         clangDiags->enablePaths();
 
       } else if (!OutDir.empty()) {
         switch (Opts->AnalysisDiagOpt) {
         default:
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)                    \
   case PD_##NAME:                                                              \

cfe/trunk/test/Analysis/analyzer-config.c

 // CHECK-NEXT: cfg-rich-constructors = true
 // CHECK-NEXT: cfg-scopes = false
 // CHECK-NEXT: cfg-temporary-dtors = true
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-import-threshold = 100
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
+// CHECK-NEXT: deadcode.DeadStores:ShowFixIts = false
 // CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = true
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCastExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtOffsetOfExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtCXXNewExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtCastExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtOffsetOfExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:RegionChanges = false
 // CHECK-NEXT: display-ctu-progress = false
 // CHECK-NEXT: eagerly-assume = true
 // CHECK-NEXT: elide-constructors = true
 // CHECK-NEXT: expand-macros = false
 // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
+// CHECK-NEXT: fixits-as-remarks = false
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-symbol-complexity = 35
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: model-path = ""
 // CHECK-NEXT: notes-as-events = false
 // CHECK-NEXT: nullability:NoDiagnoseCallsToSystemHeaders = false
 // CHECK-NEXT: objc-inlining = true
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:CheckPointeeInitialization = false
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreGuardedFields = false
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreRecordsWithField = ""
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:NotesAsWarnings = false
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:Pedantic = false
 // CHECK-NEXT: optin.cplusplus.VirtualCall:PureOnly = false
+// CHECK-NEXT: optin.cplusplus.VirtualCall:ShowFixIts = false
 // CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false
 // CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
 // CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
 // CHECK-NEXT: osx.cocoa.RetainCount:CheckOSObject = true
 // CHECK-NEXT: osx.cocoa.RetainCount:TrackNSCFStartParam = false
 // CHECK-NEXT: prune-paths = true
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: silence-checkers = ""
 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 91
+// CHECK-NEXT: num-entries = 94

cfe/trunk/test/Analysis/dead-stores.c

-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code     \
-// RUN:  -analyzer-checker=core,deadcode.DeadStores                             \
-// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
-// RUN:  -analyzer-opt-analyze-nested-blocks -verify=non-nested %s
-//
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code     \
-// RUN:  -analyzer-checker=core,deadcode.DeadStores                             \
-// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
-// RUN:  -analyzer-opt-analyze-nested-blocks -verify=non-nested                 \
-// RUN:  -analyzer-store=region %s
-//
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code     \
-// RUN:  -analyzer-checker=core,deadcode.DeadStores                             \
-// RUN:  -analyzer-opt-analyze-nested-blocks -verify=non-nested,nested %s
+// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN:   -analyzer-checker=core,deadcode.DeadStores \
+// RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
+// RUN:   -analyzer-config fixits-as-remarks=true \
+// RUN:   -analyzer-config \
+// RUN:       deadcode.DeadStores:WarnForDeadNestedAssignments=false \
+// RUN:   -verify=non-nested %s
+
+// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN:   -analyzer-checker=core,deadcode.DeadStores \
+// RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
+// RUN:   -analyzer-config fixits-as-remarks=true \
+// RUN:   -verify=non-nested,nested %s
 
 void f1() {
   int k, y; // non-nested-warning {{unused variable 'k'}}
             // non-nested-warning@-1 {{unused variable 'y'}}
   int abc = 1;
   long idx = abc + 3 * 5; // non-nested-warning {{never read}}
                           // non-nested-warning@-1 {{unused variable 'idx'}}
+                          // non-nested-remark@-2 {{11-24: ''}}
 }
 
 void f2(void *b) {
   char *c = (char *)b; // no-warning
   char *d = b + 1;     // non-nested-warning {{never read}}
                        // non-nested-warning@-1 {{unused variable 'd'}}
+                       // non-nested-remark@-2 {{10-17: ''}}
   printf("%s", c);
   // non-nested-warning@-1 {{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}}
   // non-nested-note@-2 {{include the header <stdio.h> or explicitly provide a declaration for 'printf'}}
 }
 
 int f();
 void f3() {
   int r;
if (k)
     f1();
   k = 2; // non-nested-warning {{never read}}
 }
 
 void f5() {
   int x = 4;   // no-warning
   int *p = &x; // non-nested-warning {{never read}}
                // non-nested-warning@-1 {{unused variable 'p'}}
+               // non-nested-remark@-2 {{9-13: ''}}
 }
 
 int f6() {
   int x = 4;
   ++x; // no-warning
   return 1;
 }
 
else
       f23_aux("I shouldn't log.  Wait.. d'oh!\n");
   }();
 }
 
 void f23_pos(int argc, char **argv) {
   int shouldLog = (argc > 1);
   // non-nested-warning@-1 {{Value stored to 'shouldLog' during its initialization is never read}}
   // non-nested-warning@-2 {{unused variable 'shouldLog'}}
+  // non-nested-remark@-3 {{16-28: ''}}
   ^{
     f23_aux("I did too use it!\n");
   }();
 }
 
 void f24_A(int y) {
   // FIXME: One day this should be reported as dead since 'z = x + y' is dead.
   int x = (y > 2); // no-warning
   ^{
     int z = x + y;
     // non-nested-warning@-1 {{Value stored to 'z' during its initialization is never read}}
     // non-nested-warning@-2 {{unused variable 'z'}}
+    // non-nested-remark@-3 {{10-17: ''}}
   }();
 }
 
 void f24_B(int y) {
   // FIXME: One day this should be reported as dead since 'x' is just overwritten.
   __block int x = (y > 2); // no-warning
   ^{
     // FIXME: This should eventually be a dead store since it is never read either.

cfe/trunk/test/Analysis/edges-new.mm

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,deadcode.DeadStores,osx.cocoa.RetainCount,unix.Malloc,unix.MismatchedDeallocator -analyzer-output=plist -o %t -w %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,deadcode.DeadStores,osx.cocoa.RetainCount,unix.Malloc,unix.MismatchedDeallocator -analyzer-output=plist -analyzer-config deadcode.DeadStores:ShowFixIts=true -o %t -w %s
 // RUN: %normalize_plist <%t | diff -ub %S/Inputs/expected-plists/edges-new.mm.plist -
 
 //===----------------------------------------------------------------------===//
 // Forward declarations (from headers).
 //===----------------------------------------------------------------------===//
 
 typedef const struct __CFNumber * CFNumberRef;
 typedef const struct __CFAllocator * CFAllocatorRef;

cfe/trunk/test/Analysis/objc-arc.m

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.cocoa.RetainCount,deadcode -verify -fblocks -analyzer-opt-analyze-nested-blocks -fobjc-arc -analyzer-output=plist-multi-file -o %t.plist %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.cocoa.RetainCount,deadcode -verify -fblocks -analyzer-opt-analyze-nested-blocks -fobjc-arc -analyzer-output=plist-multi-file -analyzer-config deadcode.DeadStores:ShowFixIts=true -o %t.plist %s
 // RUN: %normalize_plist <%t.plist | diff -ub %S/Inputs/expected-plists/objc-arc.m.plist -
 
 typedef signed char BOOL;
 typedef struct _NSZone NSZone;
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 typedef unsigned long NSUInteger;
 
 @protocol NSObject

cfe/trunk/test/Analysis/plist-output.m

-// RUN: %clang_analyze_cc1 -analyzer-config eagerly-assume=false %s -analyzer-checker=osx.cocoa.RetainCount,deadcode.DeadStores,core -analyzer-output=plist -o %t.plist
+// RUN: %clang_analyze_cc1 -analyzer-config eagerly-assume=false %s -analyzer-checker=osx.cocoa.RetainCount,deadcode.DeadStores,core -analyzer-output=plist -analyzer-config deadcode.DeadStores:ShowFixIts=true -o %t.plist
 // RUN: %normalize_plist <%t.plist | diff -ub %S/Inputs/expected-plists/plist-output.m.plist -
 
 void test_null_init(void) {
   int *p = 0;
   *p = 0xDEADBEEF;
 }
 
 void test_null_assign(void) {

cfe/trunk/test/Analysis/virtualcall-fixits.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:     -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
+// RUN:     %s 2>&1 | FileCheck -check-prefix=TEXT %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:     -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
+// RUN:     -analyzer-config fixits-as-remarks=true \
+// RUN:     -analyzer-output=plist -o %t.plist -verify %s
+// RUN: cat %t.plist | FileCheck -check-prefix=PLIST %s
+
+struct S {
+  virtual void foo();
+  S() {
+    foo();
+    // expected-warning@-1{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+    // expected-remark@-2{{5-5: 'S::'}}
+  }
+  ~S();
+};
+
+// TEXT: warning: Call to virtual method 'S::foo' during construction
+// TEXT-SAME: bypasses virtual dispatch
+// TEXT-NEXT: foo();
+// TEXT-NEXT: ^~~~~
+// TEXT-NEXT: S::
+// TEXT-NEXT: 1 warning generated.
+
+// PLIST:  <key>fixits</key>
+// PLIST-NEXT:  <array>
+// PLIST-NEXT:   <dict>
+// PLIST-NEXT:    <key>remove_range</key>
+// PLIST-NEXT:    <array>
+// PLIST-NEXT:     <dict>
+// PLIST-NEXT:      <key>line</key><integer>13</integer>
+// PLIST-NEXT:      <key>col</key><integer>5</integer>
+// PLIST-NEXT:      <key>file</key><integer>0</integer>
+// PLIST-NEXT:     </dict>
+// PLIST-NEXT:     <dict>
+// PLIST-NEXT:      <key>line</key><integer>13</integer>
+// PLIST-NEXT:      <key>col</key><integer>4</integer>
+// PLIST-NEXT:      <key>file</key><integer>0</integer>
+// PLIST-NEXT:     </dict>
+// PLIST-NEXT:    </array>
+// PLIST-NEXT:    <key>insert_string</key><string>S::</string>
+// PLIST-NEXT:   </dict>
+// PLIST-NEXT:  </array>