[libtooling][clang-tidy] Fix crashing on rendering invalid SourceRanges

Invalid SourceRanges can occur generally if the code does not compile,
thus we expect clang error diagnostics.
Unlike `clang`, `clang-tidy` did not swallow invalid source ranges, but
tried to highlight them, and blow various assertions.

The following two examples produce invalid source ranges, but this is
not a complete list:

  void test(x); // error: unknown type name 'x'
  struct Foo {
    member; // error: C++ requires a type specifier for all declarations
  };

Thanks @whisperity helping me fix this.

Reviewed-By: xazax.hun

Differential Revision: https://reviews.llvm.org/D114254

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

@@ -79,16 +79,22 @@ protected:
       return CharSourceRange::getCharRange(SourceRange.getBegin(), End);
     };
 
+    // We are only interested in valid ranges.
+    auto ValidRanges =
+        llvm::make_filter_range(Ranges, [](const CharSourceRange &R) {
+          return R.getAsRange().isValid();
+        });
+
     if (Level == DiagnosticsEngine::Note) {
       Error.Notes.push_back(TidyMessage);
-      for (const CharSourceRange &SourceRange : Ranges)
+      for (const CharSourceRange &SourceRange : ValidRanges)
         Error.Notes.back().Ranges.emplace_back(Loc.getManager(),
                                                ToCharRange(SourceRange));
       return;
     }
     assert(Error.Message.Message.empty() && "Overwriting a diagnostic message");
     Error.Message = TidyMessage;
-    for (const CharSourceRange &SourceRange : Ranges)
+    for (const CharSourceRange &SourceRange : ValidRanges)
       Error.Message.Ranges.emplace_back(Loc.getManager(),
                                         ToCharRange(SourceRange));
   }

clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp

@@ -7,6 +7,11 @@ X(f)
 int a[-1];
 int b[0];
 
+void test(x);
+struct Foo {
+  member;
+};
+
 // CHECK-MESSAGES: -input.cpp:2:1: warning: no previous prototype for function 'ff' [clang-diagnostic-missing-prototypes]
 // CHECK-MESSAGES: -input.cpp:1:19: note: expanded from macro 'X'
 // CHECK-MESSAGES: {{^}}note: expanded from here{{$}}
@@ -14,6 +19,8 @@ int b[0];
 // CHECK-MESSAGES: -input.cpp:1:14: note: expanded from macro 'X'
 // CHECK-MESSAGES: -input.cpp:3:7: error: 'a' declared as an array with a negative size [clang-diagnostic-error]
 // CHECK-MESSAGES: -input.cpp:4:7: warning: zero size arrays are an extension [clang-diagnostic-zero-length-array]
+// CHECK-MESSAGES: -input.cpp:6:11: error: unknown type name 'x' [clang-diagnostic-error]
+// CHECK-MESSAGES: -input.cpp:8:3: error: C++ requires a type specifier for all declarations [clang-diagnostic-error]
 
 // CHECK-YAML: ---
 // CHECK-YAML-NEXT: MainSourceFile:  '{{.*}}-input.cpp'
@@ -71,4 +78,20 @@ int b[0];
 // CHECK-YAML-NEXT:          Length:          1
 // CHECK-YAML-NEXT:     Level:           Warning
 // CHECK-YAML-NEXT:     BuildDirectory:  '{{.*}}'
+// CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-error
+// CHECK-YAML-NEXT:     DiagnosticMessage:
+// CHECK-YAML-NEXT:       Message:         'unknown type name ''x'''
+// CHECK-YAML-NEXT:       FilePath:        '{{.*}}-input.cpp'
+// CHECK-YAML-NEXT:       FileOffset:      67
+// CHECK-YAML-NEXT:       Replacements:    []
+// CHECK-YAML-NEXT:     Level:           Error
+// CHECK-YAML-NEXT:     BuildDirectory:  '{{.*}}'
+// CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-error
+// CHECK-YAML-NEXT:     DiagnosticMessage:
+// CHECK-YAML-NEXT:       Message:         'C++ requires a type specifier for all declarations'
+// CHECK-YAML-NEXT:       FilePath:        '{{.*}}-input.cpp'
+// CHECK-YAML-NEXT:       FileOffset:      86
+// CHECK-YAML-NEXT:       Replacements:    []
+// CHECK-YAML-NEXT:     Level:           Error
+// CHECK-YAML-NEXT:     BuildDirectory:  '{{.*}}'
 // CHECK-YAML-NEXT: ...

clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp

@@ -37,6 +37,33 @@ public:
   }
 };
 
+class InvalidRangeTestCheck : public ClangTidyCheck {
+public:
+  InvalidRangeTestCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override {
+    Finder->addMatcher(ast_matchers::varDecl().bind("var"), this);
+  }
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override {
+    const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+    SourceLocation ValidBeginLoc = Var->getBeginLoc();
+    SourceLocation ValidEndLoc = Var->getEndLoc();
+    SourceLocation InvalidLoc;
+    ASSERT_TRUE(ValidBeginLoc.isValid());
+    ASSERT_TRUE(ValidEndLoc.isValid());
+    ASSERT_TRUE(InvalidLoc.isInvalid());
+
+    diag(ValidBeginLoc, "valid->valid")
+        << SourceRange(ValidBeginLoc, ValidEndLoc);
+    diag(ValidBeginLoc, "valid->invalid")
+        << SourceRange(ValidBeginLoc, InvalidLoc);
+    diag(ValidBeginLoc, "invalid->valid")
+        << SourceRange(InvalidLoc, ValidEndLoc);
+    diag(ValidBeginLoc, "invalid->invalid")
+        << SourceRange(InvalidLoc, InvalidLoc);
+  }
+};
+
 } // namespace
 
 TEST(ClangTidyDiagnosticConsumer, SortsErrors) {
@@ -66,6 +93,24 @@ TEST(ClangTidyDiagnosticConsumer, HandlesSourceRangeHighlight) {
   EXPECT_EQ(7ul, Errors[0].Message.Ranges[0].Length);
 }
 
+TEST(ClangTidyDiagnosticConsumer, InvalidSourceLocationRangesIgnored) {
+  std::vector<ClangTidyError> Errors;
+  runCheckOnCode<InvalidRangeTestCheck>("int x;", &Errors);
+  EXPECT_EQ(4ul, Errors.size());
+
+  EXPECT_EQ("invalid->invalid", Errors[0].Message.Message);
+  EXPECT_TRUE(Errors[0].Message.Ranges.empty());
+
+  EXPECT_EQ("invalid->valid", Errors[1].Message.Message);
+  EXPECT_TRUE(Errors[1].Message.Ranges.empty());
+
+  EXPECT_EQ("valid->invalid", Errors[2].Message.Message);
+  EXPECT_TRUE(Errors[2].Message.Ranges.empty());
+
+  EXPECT_EQ("valid->valid", Errors[3].Message.Message);
+  EXPECT_EQ(1ul, Errors[3].Message.Ranges.size());
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang