diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -72,13 +72,14 @@ SmallString<64> CheckName(AnalyzerCheckNamePrefix); CheckName += PD->getCheckName(); Context.diag(CheckName, PD->getLocation().asLocation(), - PD->getShortDescription()) + PD->getShortDescription(), /*FixDescription*/ "") << PD->path.back()->getRanges(); for (const auto &DiagPiece : PD->path.flatten(/*ShouldFlattenMacros=*/true)) { Context.diag(CheckName, DiagPiece->getLocation().asLocation(), - DiagPiece->getString(), DiagnosticIDs::Note) + DiagPiece->getString(), /*FixDescription*/ "", + DiagnosticIDs::Note) << DiagPiece->getRanges(); } } diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.h b/clang-tools-extra/clang-tidy/ClangTidyCheck.h --- a/clang-tools-extra/clang-tidy/ClangTidyCheck.h +++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.h @@ -92,6 +92,22 @@ /// work in here. virtual void check(const ast_matchers::MatchFinder::MatchResult &Result) {} + /// A short human readable description about the check fix. It should + /// descripe what the check fix will do, so users can rely on it to know the + /// fix intention without viewing the actual code change. + /// + /// Example: "Convert to 'switch'"; "Removed unused using"; "Use make_unique". + /// + /// The description will be attached to ``ClangTidyError``. Returning empty + /// string indicates the check doesn't provide this information. + /// + /// Caveats: + /// - The interface may not fit well with a check that produces different + /// fixes for different cases. + /// - Clang compiler diagnostics and Clang Static Analyzer diagnostics are + /// not supported. + virtual llvm::StringRef fixDescription() { return ""; } + /// \brief Add a diagnostic with the check's name. DiagnosticBuilder diag(SourceLocation Loc, StringRef Description, DiagnosticIDs::Level Level = DiagnosticIDs::Warning); diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp @@ -20,7 +20,7 @@ DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message, DiagnosticIDs::Level Level) { - return Context->diag(CheckName, Loc, Message, Level); + return Context->diag(CheckName, Loc, Message, fixDescription(), Level); } void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) { diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -16,6 +16,7 @@ #include "clang/Tooling/Core/Diagnostic.h" #include "clang/Tooling/Refactoring.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseMapInfo.h" #include "llvm/ADT/StringMap.h" #include "llvm/Support/Regex.h" #include "llvm/Support/Timer.h" @@ -116,7 +117,7 @@ /// tablegen'd diagnostic IDs. /// FIXME: Figure out a way to manage ID spaces. DiagnosticBuilder diag(StringRef CheckName, SourceLocation Loc, - StringRef Message, + StringRef Message, StringRef CheckFixDescription, DiagnosticIDs::Level Level = DiagnosticIDs::Warning); /// \brief Sets the \c SourceManager of the used \c DiagnosticsEngine. @@ -140,6 +141,10 @@ /// diagnostic ID. std::string getCheckName(unsigned DiagnosticID) const; + /// \brief Returns the fix description of the clang-tidy check which produced + /// this diagnostic ID. + std::string getCheckFixDescription(unsigned DiagnosticID) const; + /// \brief Returns \c true if the check is enabled for the \c CurrentFile. /// /// The \c CurrentFile can be changed using \c setCurrentFile. @@ -209,7 +214,9 @@ std::string CurrentBuildDirectory; - llvm::DenseMap CheckNamesByDiagnosticID; + // A map from diagnostic id to . + llvm::DenseMap> + CheckNamesByDiagnosticID; bool Profile; std::string ProfilePrefix; diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -190,11 +190,12 @@ DiagnosticBuilder ClangTidyContext::diag( StringRef CheckName, SourceLocation Loc, StringRef Description, + StringRef CheckFixDescription, DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) { assert(Loc.isValid()); unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID( Level, (Description + " [" + CheckName + "]").str()); - CheckNamesByDiagnosticID.try_emplace(ID, CheckName); + CheckNamesByDiagnosticID[ID] = {CheckName, CheckFixDescription}; return DiagEngine->Report(Loc, ID); } @@ -259,10 +260,17 @@ DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID); if (!ClangWarningOption.empty()) return "clang-diagnostic-" + ClangWarningOption; - llvm::DenseMap::const_iterator I = - CheckNamesByDiagnosticID.find(DiagnosticID); + auto I = CheckNamesByDiagnosticID.find(DiagnosticID); if (I != CheckNamesByDiagnosticID.end()) - return I->second; + return I->second.first; + return ""; +} + +std::string +ClangTidyContext::getCheckFixDescription(unsigned DiagnosticID) const { + auto I = CheckNamesByDiagnosticID.find(DiagnosticID); + if (I != CheckNamesByDiagnosticID.end()) + return I->second.second; return ""; } @@ -449,6 +457,9 @@ Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager()); Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(), Info.getFixItHints()); + // Populate the fix description if the diagnostic has fixes. + if (!Errors.back().Fix.empty()) + Errors.back().FixDescription = Context.getCheckFixDescription(Info.getID()); if (Info.hasSourceManager()) checkFilters(Info.getLocation(), Info.getSourceManager()); diff --git a/clang-tools-extra/test/clang-tidy/export-diagnostics.cpp b/clang-tools-extra/test/clang-tidy/export-diagnostics.cpp --- a/clang-tools-extra/test/clang-tidy/export-diagnostics.cpp +++ b/clang-tools-extra/test/clang-tidy/export-diagnostics.cpp @@ -23,6 +23,7 @@ // CHECK-YAML-NEXT: - Message: expanded from here // CHECK-YAML-NEXT: FilePath: '' // CHECK-YAML-NEXT: FileOffset: 0 +// CHECK-YAML-NEXT: FixDescription: '' // CHECK-YAML-NEXT: Replacements: [] // CHECK-YAML-NEXT: ... diff --git a/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp b/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp --- a/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp +++ b/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp @@ -21,7 +21,7 @@ makeTUDiagnostics(const std::string &MainSourceFile, StringRef DiagnosticName, const DiagnosticMessage &Message, const StringMap &Replacements, - StringRef BuildDirectory) { + StringRef BuildDirectory, StringRef FixDescription) { TUDiagnostics TUs; TUs.push_back({MainSourceFile, {{DiagnosticName, @@ -29,7 +29,8 @@ Replacements, {}, Diagnostic::Warning, - BuildDirectory}}}); + BuildDirectory, + FixDescription}}}); return TUs; } @@ -42,8 +43,8 @@ FileManager Files((FileSystemOptions())); SourceManager SM(Diagnostics, Files); TUReplacements TURs; - TUDiagnostics TUs = - makeTUDiagnostics("path/to/source.cpp", "diagnostic", {}, {}, "path/to"); + TUDiagnostics TUs = makeTUDiagnostics("path/to/source.cpp", "diagnostic", {}, + {}, "path/to", ""); FileToChangesMap ReplacementsMap; EXPECT_TRUE(mergeAndDeduplicate(TURs, TUs, ReplacementsMap, SM)); diff --git a/clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp b/clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp --- a/clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp @@ -1,5 +1,6 @@ #include "ClangTidy.h" #include "ClangTidyTest.h" +#include "clang/Basic/Diagnostic.h" #include "gtest/gtest.h" namespace clang { @@ -13,10 +14,12 @@ void registerMatchers(ast_matchers::MatchFinder *Finder) override { Finder->addMatcher(ast_matchers::varDecl().bind("var"), this); } + llvm::StringRef fixDescription() override { return "test fix description"; } void check(const ast_matchers::MatchFinder::MatchResult &Result) override { const auto *Var = Result.Nodes.getNodeAs("var"); // Add diagnostics in the wrong order. - diag(Var->getLocation(), "variable"); + diag(Var->getLocation(), "variable") + << FixItHint::CreateInsertion(Var->getBeginLoc(), "/*empty*/"); diag(Var->getTypeSpecStartLoc(), "type specifier"); } }; @@ -27,6 +30,7 @@ EXPECT_EQ(2ul, Errors.size()); EXPECT_EQ("type specifier", Errors[0].Message.Message); EXPECT_EQ("variable", Errors[1].Message.Message); + EXPECT_EQ("test fix description", Errors[1].FixDescription); } TEST(GlobList, Empty) { diff --git a/clang/include/clang/Tooling/Core/Diagnostic.h b/clang/include/clang/Tooling/Core/Diagnostic.h --- a/clang/include/clang/Tooling/Core/Diagnostic.h +++ b/clang/include/clang/Tooling/Core/Diagnostic.h @@ -60,7 +60,7 @@ Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message, const llvm::StringMap &Fix, const SmallVector &Notes, Level DiagLevel, - llvm::StringRef BuildDirectory); + llvm::StringRef BuildDirectory, llvm::StringRef FixDescription); /// Name identifying the Diagnostic. std::string DiagnosticName; @@ -85,6 +85,10 @@ /// /// Note: it is empty in unittest. std::string BuildDirectory; + + // A short descrpition of the clang-tidy fix. + // Empty if the Diagnostic doesn't provide any fixes. + std::string FixDescription; }; /// Collection of Diagnostics generated from a single translation unit. diff --git a/clang/include/clang/Tooling/DiagnosticsYaml.h b/clang/include/clang/Tooling/DiagnosticsYaml.h --- a/clang/include/clang/Tooling/DiagnosticsYaml.h +++ b/clang/include/clang/Tooling/DiagnosticsYaml.h @@ -45,11 +45,12 @@ NormalizedDiagnostic(const IO &, const clang::tooling::Diagnostic &D) : DiagnosticName(D.DiagnosticName), Message(D.Message), Fix(D.Fix), Notes(D.Notes), DiagLevel(D.DiagLevel), - BuildDirectory(D.BuildDirectory) {} + BuildDirectory(D.BuildDirectory), FixDescription(D.FixDescription) {} clang::tooling::Diagnostic denormalize(const IO &) { return clang::tooling::Diagnostic(DiagnosticName, Message, Fix, Notes, - DiagLevel, BuildDirectory); + DiagLevel, BuildDirectory, + FixDescription); } std::string DiagnosticName; @@ -58,6 +59,7 @@ SmallVector Notes; clang::tooling::Diagnostic::Level DiagLevel; std::string BuildDirectory; + std::string FixDescription; }; static void mapping(IO &Io, clang::tooling::Diagnostic &D) { @@ -77,6 +79,7 @@ Fixes.push_back(Replacement); } } + Io.mapOptional("FixDescription", Keys->FixDescription); Io.mapRequired("Replacements", Fixes); for (auto &Fix : Fixes) { llvm::Error Err = Keys->Fix[Fix.getFilePath()].add(Fix); diff --git a/clang/lib/Tooling/Core/Diagnostic.cpp b/clang/lib/Tooling/Core/Diagnostic.cpp --- a/clang/lib/Tooling/Core/Diagnostic.cpp +++ b/clang/lib/Tooling/Core/Diagnostic.cpp @@ -42,9 +42,11 @@ const DiagnosticMessage &Message, const llvm::StringMap &Fix, const SmallVector &Notes, - Level DiagLevel, llvm::StringRef BuildDirectory) + Level DiagLevel, llvm::StringRef BuildDirectory, + llvm::StringRef FixDescription) : DiagnosticName(DiagnosticName), Message(Message), Fix(Fix), Notes(Notes), - DiagLevel(DiagLevel), BuildDirectory(BuildDirectory) {} + DiagLevel(DiagLevel), BuildDirectory(BuildDirectory), + FixDescription(FixDescription) {} } // end namespace tooling } // end namespace clang diff --git a/clang/unittests/Tooling/DiagnosticsYamlTest.cpp b/clang/unittests/Tooling/DiagnosticsYamlTest.cpp --- a/clang/unittests/Tooling/DiagnosticsYamlTest.cpp +++ b/clang/unittests/Tooling/DiagnosticsYamlTest.cpp @@ -31,9 +31,11 @@ static Diagnostic makeDiagnostic(StringRef DiagnosticName, const std::string &Message, int FileOffset, const std::string &FilePath, - const StringMap &Fix) { + const StringMap &Fix, + const std::string &FixDescription) { return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath), - Fix, {}, Diagnostic::Warning, "path/to/build/directory"); + Fix, {}, Diagnostic::Warning, "path/to/build/directory", + FixDescription); } TEST(DiagnosticsYamlTest, serializesDiagnostics) { @@ -44,16 +46,18 @@ {"path/to/source.cpp", Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}}; TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55, - "path/to/source.cpp", Fix1)); + "path/to/source.cpp", Fix1, + "fix1 description")); StringMap Fix2 = { {"path/to/header.h", Replacements({"path/to/header.h", 62, 2, "replacement #2"})}}; TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60, - "path/to/header.h", Fix2)); + "path/to/header.h", Fix2, + "fix2 description")); TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72, - "path/to/source2.cpp", {})); + "path/to/source2.cpp", {}, "")); TUD.Diagnostics.back().Notes.push_back( makeMessage("Note1", 88, "path/to/note1.cpp")); TUD.Diagnostics.back().Notes.push_back( @@ -72,6 +76,7 @@ " Message: 'message #1'\n" " FileOffset: 55\n" " FilePath: 'path/to/source.cpp'\n" + " FixDescription: fix1 description\n" " Replacements: \n" " - FilePath: 'path/to/source.cpp'\n" " Offset: 100\n" @@ -81,6 +86,7 @@ " Message: 'message #2'\n" " FileOffset: 60\n" " FilePath: 'path/to/header.h'\n" + " FixDescription: fix2 description\n" " Replacements: \n" " - FilePath: 'path/to/header.h'\n" " Offset: 62\n" @@ -97,6 +103,7 @@ " - Message: Note2\n" " FilePath: 'path/to/note2.cpp'\n" " FileOffset: 99\n" + " FixDescription: ''\n" " Replacements: []\n" "...\n", YamlContentStream.str()); @@ -110,6 +117,7 @@ " Message: 'message #1'\n" " FileOffset: 55\n" " FilePath: path/to/source.cpp\n" + " FixDescription: fix1 description\n" " Replacements: \n" " - FilePath: path/to/source.cpp\n" " Offset: 100\n" @@ -160,6 +168,7 @@ EXPECT_EQ("message #1", D1.Message.Message); EXPECT_EQ(55u, D1.Message.FileOffset); EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath); + EXPECT_EQ("fix1 description", D1.FixDescription); std::vector Fixes1 = getFixes(D1.Fix); ASSERT_EQ(1u, Fixes1.size()); EXPECT_EQ("path/to/source.cpp", Fixes1[0].getFilePath());