[Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[vladimir.plyashkun]

Summary

To get properly integration Clang-Tidy with CLion IDE, next things were implemented:

1. Preserve Message, FileOffset, FilePath in the clang-tidy output.
2. Export all diagnostics, not just the ones with fixes
3. Test-cases

[vladimir.plyashkun]

updated CMakeLists.txt

[vladimir.plyashkun]

correct revision with all changes

[alexfh]

Please upload a diff with full context (http://llvm.org/docs/Phabricator.html).

include/clang/Tooling/DiagnosticsYaml.h

86–95

We should ensure clang-apply-fixes still works correctly in the presence of diagnostics with no fixes. It would be best to add a lit test for that.

unittests/Tooling/DiagnosticsYamlTest.cpp

174

Please add a newline.

[vladimir.plyashkun]

• fixed No newline at end of file problem
• provided test-case to check that diagnostics with no fixes will be applied correctly

[alexfh]

unittests/Tooling/DiagnosticsYamlTest.cpp

52

EXPECT_STREQ is more appropriate here. ASSERT_* family of macros terminates the test, and thus should only be used to verify the invariants that are absolutely required to continue the execution of the test, which is not the case here.

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

19

I'd remove const from DiagnosticName, since a StringRef doesn't allow to modify the string it points to (and marking by value arguments const is not common in LLVM). However, Message and Replacements allow to modify the referenced objects and thus should be marked const.

Also consider inlining this method, it doesn't buy you much.

24

Will {} work instead of EmptyNotes?

24

tooling:: is not needed due to the using directive above. Same below.

55–57

I'd remove the success variable and just call EXPECT_TRUE(merge...)

[vladimir.plyashkun]

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

24

Unfortunately, no.
Constructor expects non-const reference SmallVector<DiagnosticMessage, 1> &Notes, so i can't pass rvalue directly to it.

[vladimir.plyashkun]

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

19

Same for here.
Constructor for Diagnostic class requires non-const references for Message and Replacements arguments. So, it's invalid to mark them as const.

[vladimir.plyashkun]

• use EXPECT_* instead of ASSERT_* where it's possible
• Diagnostic constructor now takes const references for it's arguments
• removed extra namespace qualifiers

[alexfh]

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

24

I overlooked this when committing someone's patch. Fixed in r307143. Now you can add const and probably also use {}.

[vladimir.plyashkun]

• marked some arguments as const
• use {} instead of explicit variable declarations

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

24

Thanks, fixed it.

[alexfh]

A few more nits.

unittests/Tooling/DiagnosticsYamlTest.cpp

40

The assignment form of initialization is clearer for containers (and actually any type for which the initialization means "this variable is whatever is on the right side of the assignment" as opposed to "this variable is initialized by a constructor call taking these parameters").

61

Why not EXPECT_EQ (and remove .c_str() from the second argument)?

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

28

I believe, this can be expressed a bit less verbosely, e.g. TUs.push_back({ MainSourceFile, {Diagnostic(...)}});.

[vladimir.plyashkun]

Thanks, Alex for your suggestions.
I agree with your remarks, but i thought it's only personal style preferences.
I'll fix it soon.

[vladimir.plyashkun]

• made code less verbose
• used assignment initialization form
• removed extra .c_str() call

[alexfh]

Looks good. Thanks!

[alexfh]

I tried landing the patch for you, but it doesn't apply cleanly. One reason is that it contains changes to both cfe and clang-tools-extra repos. But even when I apply the patch to the two directories it breaks a bunch of clang-apply-replacements tests:

Failing Tests (4):
    Clang Tools :: clang-apply-replacements/basic.cpp
    Clang Tools :: clang-apply-replacements/conflict.cpp
    Clang Tools :: clang-apply-replacements/crlf.cpp
    Clang Tools :: clang-apply-replacements/format.cpp

Please rebase the patch and split it in two (upload a second differential revision for the clang-tools-extra part).

[vladimir.plyashkun]

1. split revision into two
2. fix failing tests

[vladimir.plyashkun]

I've splitted single revision into two different revisions:
https://reviews.llvm.org/D34404
https://reviews.llvm.org/D35349
Also, i fixed failing test-cases in clang-apply-replacements due to changes in output format (so they shouldn't be a problem anymore)

[alexfh]

unittests/Tooling/DiagnosticsYamlTest.cpp

36–46

Note: there's a bunch of trailing whitespace here and the formatting is off in a few places. git-clang-format fixed both issues, but it would be nice if you set up your editor properly or run clang-format yourself next time.

Same for the other CL, but there I had to also clean up trailing whitespace from yaml files as well.

include/clang/Tooling/DiagnosticsYaml.h

public:
     clang::tooling::Diagnostic::Level DiagLevel;
     std::string BuildDirectory;
   };
 
   static void mapping(IO &Io, clang::tooling::Diagnostic &D) {
     MappingNormalization<NormalizedDiagnostic, clang::tooling::Diagnostic> Keys(
         Io, D);
     Io.mapRequired("DiagnosticName", Keys->DiagnosticName);
+    Io.mapRequired("Message", Keys->Message.Message);
+    Io.mapRequired("FileOffset", Keys->Message.FileOffset);
+    Io.mapRequired("FilePath", Keys->Message.FilePath);
 
     // FIXME: Export properly all the different fields.
 
     std::vector<clang::tooling::Replacement> Fixes;
     for (auto &Replacements : Keys->Fix) {
       for (auto &Replacement : Replacements.second) {
         Fixes.push_back(Replacement);
       }
template <> struct MappingTraits<clang::tooling::Diagnostic> {
   }
 };
 
 /// \brief Specialized MappingTraits to describe how a
 /// TranslationUnitDiagnostics is (de)serialized.
 template <> struct MappingTraits<clang::tooling::TranslationUnitDiagnostics> {
   static void mapping(IO &Io, clang::tooling::TranslationUnitDiagnostics &Doc) {
     Io.mapRequired("MainSourceFile", Doc.MainSourceFile);
-
-    std::vector<clang::tooling::Diagnostic> Diagnostics;
-    for (auto &Diagnostic : Doc.Diagnostics) {
-      // FIXME: Export all diagnostics, not just the ones with fixes.
-      // Update MappingTraits<clang::tooling::Diagnostic>::mapping.
-      if (Diagnostic.Fix.size() > 0) {
-        Diagnostics.push_back(Diagnostic);
-      }
-    }
-    Io.mapRequired("Diagnostics", Diagnostics);
-    Doc.Diagnostics = Diagnostics;
+    Io.mapRequired("Diagnostics", Doc.Diagnostics);
   }
 };
 } // end namespace yaml
 } // end namespace llvm
 
 #endif // LLVM_CLANG_TOOLING_DIAGNOSTICSYAML_H

unittests/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   Support
   )
 
 # By default MSVC has a 2^16 limit on the number of sections in an object file,
 # and this needs more than that.
 if (MSVC)
   set_source_files_properties(RecursiveASTVisitorTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
   set_source_files_properties(RecursiveASTVisitorTestExprVisitor.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_unittest(ToolingTests
   CastExprTest.cpp
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  DiagnosticsYamlTest.cpp
   FixItTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
   RecursiveASTVisitorTest.cpp
   RecursiveASTVisitorTestCallVisitor.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestExprVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp

unittests/Tooling/DiagnosticsYamlTest.cpp

+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+static Diagnostic makeDiagnostic(StringRef DiagnosticName, 
+                                 const std::string &Message,
+                                 int FileOffset,
+                                 const std::string &FilePath,
+                                 const StringMap<Replacements> &Fix) {
+  DiagnosticMessage DiagMessage;
+  DiagMessage.Message = Message;
+  DiagMessage.FileOffset = FileOffset;
+  DiagMessage.FilePath = FilePath;
+  return Diagnostic(DiagnosticName, DiagMessage, Fix, {},
+                    Diagnostic::Warning, "path/to/build/directory");
+}
+
+TEST(DiagnosticsYamlTest, serializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+
+  StringMap<Replacements> Fix1 = { 
+    { 
+      "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));
+  
+  StringMap<Replacements> 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));
+
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72, 
+                                           "path/to/source2.cpp", {}));
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  EXPECT_EQ("---\n"
+    "MainSourceFile:  path/to/source.cpp\n"
+    "Diagnostics:     \n"
+    "  - DiagnosticName:  'diagnostic#1\'\n"
+    "    Message:         'message #1'\n"
+    "    FileOffset:      55\n"
+    "    FilePath:        path/to/source.cpp\n"
+    "    Replacements:    \n"
+    "      - FilePath:        path/to/source.cpp\n"
+    "        Offset:          100\n"
+    "        Length:          12\n"
+    "        ReplacementText: 'replacement #1'\n"
+    "  - DiagnosticName:  'diagnostic#2'\n"
+    "    Message:         'message #2'\n"
+    "    FileOffset:      60\n"
+    "    FilePath:        path/to/header.h\n"
+    "    Replacements:    \n"
+    "      - FilePath:        path/to/header.h\n"
+    "        Offset:          62\n"
+    "        Length:          2\n"
+    "        ReplacementText: 'replacement #2'\n"
+    "  - DiagnosticName:  'diagnostic#3'\n"
+    "    Message:         'message #3'\n"
+    "    FileOffset:      72\n"
+    "    FilePath:        path/to/source2.cpp\n"
+    "    Replacements:    \n"
+    "...\n",
+      YamlContentStream.str());
+}
+
+TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
+    std::string YamlContent = "---\n"
+      "MainSourceFile:  path/to/source.cpp\n"
+      "Diagnostics:     \n"
+      "  - DiagnosticName:  'diagnostic#1'\n"
+      "    Message:         'message #1'\n"
+      "    FileOffset:      55\n"
+      "    FilePath:        path/to/source.cpp\n"
+      "    Replacements:    \n"
+      "      - FilePath:        path/to/source.cpp\n"
+      "        Offset:          100\n"
+      "        Length:          12\n"
+      "        ReplacementText: 'replacement #1'\n"
+      "  - DiagnosticName:  'diagnostic#2'\n"
+      "    Message:         'message #2'\n"
+      "    FileOffset:      60\n"
+      "    FilePath:        path/to/header.h\n"
+      "    Replacements:    \n"
+      "      - FilePath:        path/to/header.h\n"
+      "        Offset:          62\n"
+      "        Length:          2\n"
+      "        ReplacementText: 'replacement #2'\n"
+      "  - DiagnosticName:  'diagnostic#3'\n"
+      "    Message:         'message #3'\n"
+      "    FileOffset:      98\n"
+      "    FilePath:        path/to/source.cpp\n"
+      "    Replacements:    \n"
+      "...\n";
+  TranslationUnitDiagnostics TUDActual;
+  yaml::Input YAML(YamlContent);
+  YAML >> TUDActual;
+
+  ASSERT_FALSE(YAML.error());
+  ASSERT_EQ(3u, TUDActual.Diagnostics.size());
+  EXPECT_EQ("path/to/source.cpp", TUDActual.MainSourceFile);
+
+  auto getFixes = [](const StringMap<Replacements> &Fix) {
+    std::vector<Replacement> Fixes;
+      for (auto &Replacements : Fix) {
+        for (auto &Replacement : Replacements.second) {
+          Fixes.push_back(Replacement);
+        }
+      }
+    return Fixes;
+  };
+
+  Diagnostic D1 = TUDActual.Diagnostics[0];
+  EXPECT_EQ("diagnostic#1", D1.DiagnosticName);
+  EXPECT_EQ("message #1", D1.Message.Message);
+  EXPECT_EQ(55u, D1.Message.FileOffset);
+  EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
+  std::vector<Replacement> Fixes1 = getFixes(D1.Fix);
+  ASSERT_EQ(1u, Fixes1.size());
+  EXPECT_EQ("path/to/source.cpp", Fixes1[0].getFilePath());
+  EXPECT_EQ(100, Fixes1[0].getOffset());
+  EXPECT_EQ(12, Fixes1[0].getLength());
+  EXPECT_EQ("replacement #1", Fixes1[0].getReplacementText());
+
+  Diagnostic D2 = TUDActual.Diagnostics[1];
+  EXPECT_EQ("diagnostic#2", D2.DiagnosticName);
+  EXPECT_EQ("message #2", D2.Message.Message);
+  EXPECT_EQ(60, D2.Message.FileOffset);
+  EXPECT_EQ("path/to/header.h", D2.Message.FilePath);
+  std::vector<Replacement> Fixes2 = getFixes(D2.Fix);
+  ASSERT_EQ(1u, Fixes2.size());
+  EXPECT_EQ("path/to/header.h", Fixes2[0].getFilePath());
+  EXPECT_EQ(62, Fixes2[0].getOffset());
+  EXPECT_EQ(2, Fixes2[0].getLength());
+  EXPECT_EQ("replacement #2", Fixes2[0].getReplacementText());
+
+  Diagnostic D3 = TUDActual.Diagnostics[2];
+  EXPECT_EQ("diagnostic#3", D3.DiagnosticName);
+  EXPECT_EQ("message #3", D3.Message.Message);
+  EXPECT_EQ(98, D3.Message.FileOffset);
+  EXPECT_EQ("path/to/source.cpp", D3.Message.FilePath);
+  std::vector<Replacement> Fixes3 = getFixes(D3.Fix);
+  EXPECT_TRUE(Fixes3.empty());
+}

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

+//===- clang-apply-replacements/ApplyReplacementsTest.cpp ----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang-apply-replacements/Tooling/ApplyReplacements.h"
+#include "gtest/gtest.h"
+
+using namespace clang::replace;
+using namespace llvm;
+
+namespace clang { 
+namespace tooling {
+
+static TUDiagnostics makeTUDiagnostics(const std::string &MainSourceFile,
+                                       StringRef DiagnosticName,
+                                       const DiagnosticMessage &Message,
+                                       const StringMap<Replacements> &Replacements,
+                                       StringRef BuildDirectory) {
+  TUDiagnostics TUs;
+  TUs.push_back({ MainSourceFile, 
+                  { { DiagnosticName, Message, Replacements, {}, 
+                      Diagnostic::Warning, BuildDirectory } } 
+  });
+  return TUs;
+}
+
+// Test to ensure diagnostics with no fixes, will be merged correctly 
+// before applying.
+TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
+  DiagnosticsEngine Diagnostics(
+    IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts.get());
+  FileManager Files((FileSystemOptions()));
+  SourceManager SM(Diagnostics, Files);
+  TUDiagnostics TUs = makeTUDiagnostics("path/to/source.cpp", "diagnostic",
+                                        {}, {}, "path/to");
+  FileToReplacementsMap ReplacementsMap;
+
+  EXPECT_TRUE(mergeAndDeduplicate(TUs, ReplacementsMap, SM));
+  EXPECT_TRUE(ReplacementsMap.empty());
+}
+
+} // end namespace tooling
+} // end namespace clang

unittests/clang-apply-replacements/CMakeLists.txt

 get_filename_component(ClangApplyReplacementsLocation
   "${CMAKE_CURRENT_SOURCE_DIR}/../../clang-apply-replacements/include" REALPATH)
 get_filename_component(CommonIncLocation
   "${CMAKE_CURRENT_SOURCE_DIR}/../include" REALPATH)
 include_directories(
   ${ClangApplyReplacementsLocation}
   ${CommonIncLocation}
   )
 
 add_extra_unittest(ClangApplyReplacementsTests
+  ApplyReplacementsTest.cpp
   ReformattingTest.cpp
   )
 
 target_link_libraries(ClangApplyReplacementsTests
   clangApplyReplacements
   clangToolingCore
   )