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

[steakhal]

Summary

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.

[whisperity]

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

102

EXPECT_TRUE(Errors[0].Message.Ranges.empty());? But I'm not sure if the data structure used there supports this.

[steakhal]

• prefer empty() over size() == 0

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

auto ToCharRange = [this, &Loc](const CharSourceRange &SourceRange) {
       if (SourceRange.isCharRange())
         return SourceRange;
       assert(SourceRange.isTokenRange());
       SourceLocation End = Lexer::getLocForEndOfToken(
           SourceRange.getEnd(), 0, Loc.getManager(), LangOpts);
       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));
   }
 
   void emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
                          DiagnosticsEngine::Level Level,
                          ArrayRef<CharSourceRange> Ranges) override {}
 

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

 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t-input.cpp
 // RUN: not clang-tidy %t-input.cpp -checks='-*,google-explicit-constructor,clang-diagnostic-missing-prototypes,clang-diagnostic-zero-length-array' -export-fixes=%t.yaml -- -Wmissing-prototypes -Wzero-length-array > %t.msg 2>&1
 // RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s -implicit-check-not='{{warning|error|note}}:'
 // RUN: FileCheck -input-file=%t.yaml -check-prefix=CHECK-YAML %s
 #define X(n) void n ## n() {}
 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{{$}}
 // CHECK-MESSAGES: -input.cpp:2:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
 // 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'
 // CHECK-YAML-NEXT: Diagnostics:
 // CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-missing-prototypes
 // CHECK-YAML-NEXT:     DiagnosticMessage:
 // CHECK-YAML-NEXT:       Message:         'no previous prototype for function ''ff'''
 // CHECK-YAML-NEXT:       FilePath:        '{{.*}}-input.cpp'

 // CHECK-YAML-NEXT:       FileOffset:      52
 // CHECK-YAML-NEXT:       Replacements:    []
 // CHECK-YAML-NEXT:       Ranges:
 // CHECK-YAML-NEXT:        - FilePath:        '{{.*}}-input.cpp'
 // CHECK-YAML-NEXT:          FileOffset:      52
 // 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

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");
     diag(Var->getLocation(), "highlight range") << Var->getSourceRange();
   }
 };
 
+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) {
   std::vector<ClangTidyError> Errors;
   runCheckOnCode<TestCheck>("int a;", &Errors);
   EXPECT_EQ(3ul, Errors.size());
   EXPECT_EQ("DiagWithNoLoc", Errors[0].Message.Message);
   EXPECT_EQ("type specifier", Errors[1].Message.Message);
TEST(ClangTidyDiagnosticConsumer, HandlesSourceRangeHighlight) {
 
   // int abc
   // ~~~~~~~   -> Length 7. (0-length highlights are nonsensical.)
   EXPECT_EQ(1ul, Errors[0].Message.Ranges.size());
   EXPECT_EQ(0ul, Errors[0].Message.Ranges[0].FileOffset);
   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