[clangd] Do not end inactiveRegions range at position 0 of line

This carries over the fix previously made for semantic highlighting
https://reviews.llvm.org/D92148, to the new inactiveRegions
protocol as well.

In addition, the directives at the beginning and end of an
inactive region are now excluded from the region.

Fixes https://github.com/clangd/clangd/issues/1631
Fixes https://github.com/clangd/clangd/issues/773

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

clang-tools-extra/clangd/ClangdServer.cpp

@@ -116,8 +116,8 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
         ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
                                             std::move(Diagnostics));
         if (CollectInactiveRegions) {
-          ServerCallbacks->onInactiveRegionsReady(
-              Path, std::move(AST.getMacros().SkippedRanges));
+          ServerCallbacks->onInactiveRegionsReady(Path,
+                                                  getInactiveRegions(AST));
         }
       });
   }

clang-tools-extra/clangd/SemanticHighlighting.cpp

@@ -39,6 +39,17 @@ namespace clang {
 namespace clangd {
 namespace {
 
+/// Get the last Position on a given line.
+llvm::Expected<Position> endOfLine(llvm::StringRef Code, int Line) {
+  auto StartOfLine = positionToOffset(Code, Position{Line, 0});
+  if (!StartOfLine)
+    return StartOfLine.takeError();
+  StringRef LineText = Code.drop_front(*StartOfLine).take_until([](char C) {
+    return C == '\n';
+  });
+  return Position{Line, static_cast<int>(lspLength(LineText))};
+}
+
 /// Some names are not written in the source code and cannot be highlighted,
 /// e.g. anonymous classes. This function detects those cases.
 bool canHighlightName(DeclarationName Name) {
@@ -516,38 +527,27 @@ public:
 
     // Merge token stream with "inactive line" markers.
     std::vector<HighlightingToken> WithInactiveLines;
-    auto SortedSkippedRanges = AST.getMacros().SkippedRanges;
-    llvm::sort(SortedSkippedRanges);
+    auto SortedInactiveRegions = getInactiveRegions(AST);
+    llvm::sort(SortedInactiveRegions);
     auto It = NonConflicting.begin();
-    for (const Range &R : SortedSkippedRanges) {
-      // Create one token for each line in the skipped range, so it works
+    for (const Range &R : SortedInactiveRegions) {
+      // Create one token for each line in the inactive range, so it works
       // with line-based diffing.
       assert(R.start.line <= R.end.line);
       for (int Line = R.start.line; Line <= R.end.line; ++Line) {
-        // If the end of the inactive range is at the beginning
-        // of a line, that line is not inactive.
-        if (Line == R.end.line && R.end.character == 0)
-          continue;
         // Copy tokens before the inactive line
         for (; It != NonConflicting.end() && It->R.start.line < Line; ++It)
           WithInactiveLines.push_back(std::move(*It));
         // Add a token for the inactive line itself.
-        auto StartOfLine = positionToOffset(MainCode, Position{Line, 0});
-        if (StartOfLine) {
-          StringRef LineText =
-              MainCode.drop_front(*StartOfLine).take_until([](char C) {
-                return C == '\n';
-              });
+        auto EndOfLine = endOfLine(MainCode, Line);
+        if (EndOfLine) {
           HighlightingToken HT;
           WithInactiveLines.emplace_back();
           WithInactiveLines.back().Kind = HighlightingKind::InactiveCode;
           WithInactiveLines.back().R.start.line = Line;
-          WithInactiveLines.back().R.end.line = Line;
-          WithInactiveLines.back().R.end.character =
-              static_cast<int>(lspLength(LineText));
+          WithInactiveLines.back().R.end = *EndOfLine;
         } else {
-          elog("Failed to convert position to offset: {0}",
-               StartOfLine.takeError());
+          elog("Failed to determine end of line: {0}", EndOfLine.takeError());
         }
 
         // Skip any other tokens on the inactive line. e.g.
@@ -1544,5 +1544,40 @@ diffTokens(llvm::ArrayRef<SemanticToken> Old,
   return {std::move(Edit)};
 }
 
+std::vector<Range> getInactiveRegions(ParsedAST &AST) {
+  std::vector<Range> SkippedRanges(std::move(AST.getMacros().SkippedRanges));
+  const auto &SM = AST.getSourceManager();
+  StringRef MainCode = SM.getBufferOrFake(SM.getMainFileID()).getBuffer();
+  std::vector<Range> InactiveRegions;
+  for (const Range &Skipped : SkippedRanges) {
+    Range Inactive = Skipped;
+    // Sometimes, SkippedRanges contains a range ending at position 0
+    // of a line. Clients that apply whole-line styles will treat that
+    // line as inactive which is not desirable, so adjust the ending
+    // position to be the end of the previous line.
+    if (Inactive.end.character == 0 && Inactive.end.line > 0) {
+      --Inactive.end.line;
+    }
+    // Exclude the directive lines themselves from the range.
+    if (Inactive.end.line >= Inactive.start.line + 2) {
+      ++Inactive.start.line;
+      --Inactive.end.line;
+    } else {
+      // range would be empty, e.g. #endif on next line after #ifdef
+      continue;
+    }
+    // Since we've adjusted the ending line, we need to recompute the
+    // column to reflect the end of that line.
+    if (auto EndOfLine = endOfLine(MainCode, Inactive.end.line)) {
+      Inactive.end = *EndOfLine;
+    } else {
+      elog("Failed to determine end of line: {0}", EndOfLine.takeError());
+      continue;
+    }
+    InactiveRegions.push_back(Inactive);
+  }
+  return InactiveRegions;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/SemanticHighlighting.h

@@ -120,6 +120,11 @@ llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier);
 std::vector<SemanticTokensEdit> diffTokens(llvm::ArrayRef<SemanticToken> Before,
                                            llvm::ArrayRef<SemanticToken> After);
 
+// Returns ranges of the file that are inside an inactive preprocessor branch.
+// The preprocessor directives at the beginning and end of a branch themselves
+// are not included.
+std::vector<Range> getInactiveRegions(ParsedAST &AST);
+
 } // namespace clangd
 } // namespace clang
 

clang-tools-extra/clangd/unittests/ClangdTests.cpp

@@ -1332,26 +1332,31 @@ TEST(ClangdServer, InactiveRegions) {
 #define PREAMBLEMACRO 42
 #if PREAMBLEMACRO > 40
   #define ACTIVE
-$inactive1[[#else
-  #define INACTIVE
-#endif]]
-int endPreamble;
-$inactive2[[#ifndef CMDMACRO
-    int inactiveInt;
-#endif]]
-#undef CMDMACRO
-$inactive3[[#ifdef CMDMACRO
-  int inactiveInt2;
-#else]]
-  int activeInt;
+#else
+$inactive1[[  #define INACTIVE]]
 #endif
+int endPreamble;
+#ifndef CMDMACRO
+$inactive2[[    int inactiveInt;]]
+#endif
+#undef CMDMACRO
+#ifdef CMDMACRO
+$inactive3[[  int inactiveInt2;]]
+#elif PREAMBLEMACRO > 0
+  int activeInt1;
+  int activeInt2;
+#else
+$inactive4[[  int inactiveInt3;]]
+#endif
+#ifdef CMDMACRO
+#endif  // empty inactive range, gets dropped
   )cpp");
   Server.addDocument(testPath("foo.cpp"), Source.code());
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   EXPECT_THAT(Callback.FoundInactiveRegions,
-              ElementsAre(ElementsAre(Source.range("inactive1"),
-                                      Source.range("inactive2"),
-                                      Source.range("inactive3"))));
+              ElementsAre(ElementsAre(
+                  Source.range("inactive1"), Source.range("inactive2"),
+                  Source.range("inactive3"), Source.range("inactive4"))));
 }
 
 } // namespace

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

@@ -451,11 +451,11 @@ TEST(SemanticHighlighting, GetsCorrectTokens) {
 
       #define $Macro_decl[[test]]
       #undef $Macro[[test]]
-$InactiveCode[[#ifdef test]]
-$InactiveCode[[#endif]]
+      #ifdef $Macro[[test]]
+      #endif
 
-$InactiveCode[[#if defined(test)]]
-$InactiveCode[[#endif]]
+      #if defined($Macro[[test]])
+      #endif
     )cpp",
       R"cpp(
       struct $Class_def[[S]] {
@@ -562,8 +562,9 @@ $InactiveCode[[#endif]]
       R"cpp(
       // Code in the preamble.
       // Inactive lines get an empty InactiveCode token at the beginning.
-$InactiveCode[[#ifdef test]]
-$InactiveCode[[#endif]]
+      #ifdef $Macro[[test]]
+$InactiveCode[[int Inactive1;]]
+      #endif
 
       // A declaration to cause the preamble to end.
       int $Variable_def[[EndPreamble]];
@@ -572,21 +573,21 @@ $InactiveCode[[#endif]]
       // Code inside inactive blocks does not get regular highlightings
       // because it's not part of the AST.
       #define $Macro_decl[[test2]]
-$InactiveCode[[#if defined(test)]]
+      #if defined($Macro[[test]])
 $InactiveCode[[int Inactive2;]]
-$InactiveCode[[#elif defined(test2)]]
+      #elif defined($Macro[[test2]])
       int $Variable_def[[Active1]];
-$InactiveCode[[#else]]
+      #else
 $InactiveCode[[int Inactive3;]]
-$InactiveCode[[#endif]]
+      #endif
 
       #ifndef $Macro[[test]]
       int $Variable_def[[Active2]];
       #endif
 
-$InactiveCode[[#ifdef test]]
+      #ifdef $Macro[[test]]
 $InactiveCode[[int Inactive4;]]
-$InactiveCode[[#else]]
+      #else
       int $Variable_def[[Active3]];
       #endif
     )cpp",