[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

171

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

18 ↗

(On Diff #105078)

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.

23 ↗

(On Diff #105078)

Will {} work instead of EmptyNotes?

23 ↗

(On Diff #105078)

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

54–56 ↗

(On Diff #105078)

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

[vladimir.plyashkun]

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

23 ↗

(On Diff #105078)

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

18 ↗

(On Diff #105078)

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

23 ↗

(On Diff #105078)

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

23 ↗

(On Diff #105078)

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

27 ↗

(On Diff #105260)

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

     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;

         Fixes.push_back(Replacement);
       }
     }
-    Io.mapRequired("Replacements", Fixes);
+    Io.mapOptional("Replacements", Fixes);
     for (auto &Fix : Fixes) {
       llvm::Error Err = Keys->Fix[Fix.getFilePath()].add(Fix);
       if (Err) {

 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

unittests/Tooling/CMakeLists.txt

 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  DiagnosticsYamlTest.cpp
   FixItTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.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;
+
+TEST(DiagnosticsYamlTest, SerializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+  DiagnosticMessage Message1;
+  Message1.Message = "message #1";
+  Message1.FileOffset = 55;
+  Message1.FilePath = "path/to/source.cpp";
+  StringMap<Replacements> Fix1{
+      {"path/to/source.cpp", Replacements(Replacement("path/to/source.cpp", 100,
+                                                      12, "replacement #1"))}};
+  DiagnosticMessage Message2;
+  Message2.Message = "message #2";
+  Message2.FileOffset = 60;
+  Message2.FilePath = "path/to/header.h";
+  StringMap<Replacements> Fix2{
+      {"path/to/header.h",
+       Replacements(Replacement("path/to/header.h", 62, 2, "replacement #2"))}};
+  SmallVector<DiagnosticMessage, 1> EmptyNotes;
+  TUD.Diagnostics.emplace_back("diagnostic#1", Message1, Fix1, EmptyNotes,
+                               Diagnostic::Warning, "path/to/build/directory");
+  TUD.Diagnostics.emplace_back("diagnostic#2", Message2, Fix2, EmptyNotes,
+                               Diagnostic::Error,
+                               "path/to/another/build/directory");
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  ASSERT_STREQ("---\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"
+               "...\n",
+               YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, SerializesDiagnosticWithNoFix) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+  DiagnosticMessage Message;
+  Message.Message = "message #1";
+  Message.FileOffset = 55;
+  Message.FilePath = "path/to/source.cpp";
+  StringMap<Replacements> EmptyReplacements;
+  SmallVector<DiagnosticMessage, 1> EmptyNotes;
+  TUD.Diagnostics.emplace_back("diagnostic#1", Message, EmptyReplacements,
+                               EmptyNotes, Diagnostic::Warning,
+                               "path/to/build/directory");
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  ASSERT_STREQ("---\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"
+               "...\n",
+               YamlContentStream.str().c_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"
+                            "...\n";
+  TranslationUnitDiagnostics TUDActual;
+  yaml::Input YAML(YamlContent);
+  YAML >> TUDActual;
+  ASSERT_FALSE(YAML.error());
+  ASSERT_EQ(2u, TUDActual.Diagnostics.size());
+  ASSERT_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];
+  ASSERT_EQ("diagnostic#1", D1.DiagnosticName);
+  ASSERT_EQ("message #1", D1.Message.Message);
+  ASSERT_EQ(55u, D1.Message.FileOffset);
+  ASSERT_EQ("path/to/source.cpp", D1.Message.FilePath);
+  std::vector<Replacement> Fixes1 = getFixes(D1.Fix);
+  ASSERT_EQ(1u, Fixes1.size());
+  ASSERT_EQ("path/to/source.cpp", Fixes1[0].getFilePath());
+  ASSERT_EQ(100, Fixes1[0].getOffset());
+  ASSERT_EQ(12, Fixes1[0].getLength());
+  ASSERT_EQ("replacement #1", Fixes1[0].getReplacementText());
+
+  Diagnostic D2 = TUDActual.Diagnostics[1];
+  ASSERT_EQ("diagnostic#2", D2.DiagnosticName);
+  ASSERT_EQ("message #2", D2.Message.Message);
+  ASSERT_EQ(60, D2.Message.FileOffset);
+  ASSERT_EQ("path/to/header.h", D2.Message.FilePath);
+  std::vector<Replacement> Fixes2 = getFixes(D2.Fix);
+  ASSERT_EQ(1u, Fixes2.size());
+  ASSERT_EQ("path/to/header.h", Fixes2[0].getFilePath());
+  ASSERT_EQ(62, Fixes2[0].getOffset());
+  ASSERT_EQ(2, Fixes2[0].getLength());
+  ASSERT_EQ("replacement #2", Fixes2[0].getReplacementText());
+}
 No newline at end of file