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

[nridge]

Summary

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

[hokein]

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

1342–1343

While this patch is an improvement, I wonder we should move it further.

Has been thinking about it more, we seem to have some inconsistent behavior on highlighting the #if, #else, #endif lines:

• in $inactive1 case, the #endif is marked as inactive (IMO, the highlighting in the editor is confusing, it looks like we're missing a match endif, thus I prefer to mark it as active to correspond to the active #if branch);
• in $inactive3 case, the line #elif PREAMBLEMACRO > 0 is marked as inactive, this is inconsistent with $inactive1 case;

I think it would be nice to have a consistent model. One approach is to always consider #if, #elif, #endif, #endif lines active (in the implementation side, this would mean we always use the line range [skipp_range.start.line+1, skipp_range.end.line - 1]).

What do you think about this?

[daiyousei-qz]

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

1342–1343

+1. My perspective is that C++ source code is actually a meta-language (preprocessor) that describes generation of C++ language code. That is, #if, #else, #endif and .etc are always in a sense "executed" to generate the actual code. So they shouldn't be marked as inactive.

[nridge]

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

1342–1343

I agree, what you describe is a nice additional improvement.

I believe it would also resolve the long-standing issue https://github.com/clangd/clangd/issues/773.

I will update the patch.

[nridge]

Address review comments

[nridge]

slight simplification

[hokein]

clang-tools-extra/clangd/ClangdServer.cpp

119–120

this part of code becomes non-trivial now, I suggest pulling out a function and moving it to SemanticHighlighting.cpp. The old inactive-as-comment implementation can share it as well.

153

nit: std::move(InactiveRegions).

clang-tools-extra/clangd/SemanticHighlighting.cpp

477–478

I think we should do the same thing for the old implementation as well (or just delete it at all), otherwise, we will have inconsistent behavior.

[nridge]

Address review comments

[nridge]

Add comment

[hokein]

thanks, looks great.

clang-tools-extra/clangd/SourceCode.h

76

nit: no need to expose this API, it is only used in SemanticHighlighting.cpp.

clang-tools-extra/clangd/ClangdServer.cpp

void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
     assert(AST.getDiagnostics() &&
            "We issue callback only with fresh preambles");
     std::vector<Diag> Diagnostics = *AST.getDiagnostics();
     if (ServerCallbacks)
       Publish([&]() {
         ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
                                             std::move(Diagnostics));
         if (CollectInactiveRegions) {
-          ServerCallbacks->onInactiveRegionsReady(
-              Path, std::move(AST.getMacros().SkippedRanges));
+          ServerCallbacks->onInactiveRegionsReady(Path,
+                                                  getInactiveRegions(AST));
         }
       });
   }
 
   void onFailedAST(PathRef Path, llvm::StringRef Version,
                    std::vector<Diag> Diags, PublishFn Publish) override {
     if (ServerCallbacks)
       Publish(
private:
   const ThreadsafeFS &TFS;
   std::shared_ptr<StdLibSet> Stdlib;
   AsyncTaskRunner *Tasks;
   bool CollectInactiveRegions;
 };
 
 class DraftStoreFS : public ThreadsafeFS {
 public:
   DraftStoreFS(const ThreadsafeFS &Base, const DraftStore &Drafts)
       : Base(Base), DirtyFiles(Drafts) {}
 
 private:
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override {
     auto OFS = llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(
         Base.view(std::nullopt));
     OFS->pushOverlay(DirtyFiles.asVFS());
     return OFS;

clang-tools-extra/clangd/SemanticHighlighting.h

 
 std::vector<SemanticToken> toSemanticTokens(llvm::ArrayRef<HighlightingToken>,
                                             llvm::StringRef Code);
 llvm::StringRef toSemanticTokenType(HighlightingKind Kind);
 llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier);
 std::vector<SemanticTokensEdit> diffTokens(llvm::ArrayRef<SemanticToken> Before,
                                            llvm::ArrayRef<SemanticToken> After);
 
+std::vector<Range> getInactiveRegions(ParsedAST &AST);
+
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/SemanticHighlighting.cpp

std::vector<HighlightingToken> collect(ParsedAST &AST) && {
     if (!IncludeInactiveRegionTokens)
       return NonConflicting;
 
     const auto &SM = AST.getSourceManager();
     StringRef MainCode = SM.getBufferOrFake(SM.getMainFileID()).getBuffer();
 
     // 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.
         // `#ifndef Foo` is considered as part of an inactive region when Foo is
         // defined, and there is a Foo macro token.
         // FIXME: we should reduce the scope of the inactive region to not
         // include the directive itself.
         while (It != NonConflicting.end() && It->R.start.line == Line)
if (Old.empty() && New.empty())
     return {};
   SemanticTokensEdit Edit;
   Edit.startToken = Offset;
   Edit.deleteTokens = Old.size();
   Edit.tokens = New;
   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/SourceCode.h

 /// Turn an offset in Code into a [line, column] pair.
 /// The offset must be in range [0, Code.size()].
 Position offsetToPosition(llvm::StringRef Code, size_t Offset);
 
 /// Turn a SourceLocation into a [line, column] pair.
 /// FIXME: This should return an error if the location is invalid.
 Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
 
+/// Get the last Position on a given line.
+llvm::Expected<Position> endOfLine(llvm::StringRef Code, int Line);
+
 /// Return the file location, corresponding to \p P. Note that one should take
 /// care to avoid comparing the result with expansion locations.
 llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM,
                                                         Position P);
 
 /// Returns true iff \p Loc is inside the main file. This function handles
 /// file & macro locations. For macro locations, returns iff the macro is being
 /// expanded inside the main file.

clang-tools-extra/clangd/SourceCode.cpp

Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc) {
   if (!Invalid) {
     auto ColumnInBytes = SM.getColumnNumber(FID, Offset) - 1;
     auto LineSoFar = Code.substr(Offset - ColumnInBytes, ColumnInBytes);
     P.character = lspLength(LineSoFar);
   }
   return P;
 }
 
+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))};
+}
+
 bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
   if (Loc.isFileID())
     return true;
   auto Spelling = SM.getDecomposedSpellingLoc(Loc);
   StringRef SpellingFile = SM.getSLocEntry(Spelling.first).getFile().getName();
   if (SpellingFile == "<scratch space>")
     return false;
   if (SpellingFile == "<built-in>")

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

TEST(ClangdServer, InactiveRegions) {
   auto Opts = ClangdServer::optsForTest();
   Opts.PublishInactiveRegions = true;
   InactiveRegionsCallback Callback;
   ClangdServer Server(CDB, FS, Opts, &Callback);
   Annotations Source(R"cpp(
 #define PREAMBLEMACRO 42
 #if PREAMBLEMACRO > 40
   #define ACTIVE
-$inactive1[[#else
-  #define INACTIVE
-#endif]]
+#else
+$inactive1[[  #define INACTIVE]]
+#endif
 int endPreamble;
-$inactive2[[#ifndef CMDMACRO
-    int inactiveInt;
-#endif]]
+#ifndef CMDMACRO
+$inactive2[[    int inactiveInt;]]
+#endif
 #undef CMDMACRO
-$inactive3[[#ifdef CMDMACRO
-  int inactiveInt2;
-#else]]
-  int activeInt;
+#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
 } // namespace clangd
 } // namespace clang

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

)cpp",
       // highlighting all macro references
       R"cpp(
       #ifndef $Macro[[name]]
       #define $Macro_decl[[name]]
       #endif
 
       #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]] {
         float $Field_decl[[Value]];
         $Class[[S]] *$Field_decl[[Next]];
       };
       $Class[[S]] $Variable_def[[Global]][2] = {$Class[[S]](), $Class[[S]]()};
       auto [$Variable_decl[[G1]], $Variable_decl[[G2]]] = $Variable[[Global]];
)cpp",
         $Class[[Foo]]$Bracket[[<]]$TemplateParameter[[TT]], $TemplateParameter[[TTs]]...$Bracket[[>]]
           *$Field_decl[[t]];
       };
     )cpp",
       // Inactive code highlighting
       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]];
 
       // Code after the preamble.
       // 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",
       // Argument to 'sizeof...'
       R"cpp(
       template $Bracket[[<]]typename... $TemplateParameter_def[[Elements]]$Bracket[[>]]
       struct $Class_def[[TupleSize]] {
         static const int $StaticField_decl_readonly_static[[size]] =