[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–155

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

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));
+          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;
+            }
+            // If we've adjusted the ending line, recompute the column
+            // which should be at the end of the line.
+            if (Inactive.end.line != Skipped.end.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);
+          }
+          ServerCallbacks->onInactiveRegionsReady(Path, InactiveRegions);
         }
       });
   }
 
   void onFailedAST(PathRef Path, llvm::StringRef Version,
                    std::vector<Diag> Diags, PublishFn Publish) override {
     if (ServerCallbacks)
       Publish(

clang-tools-extra/clangd/SemanticHighlighting.cpp

std::vector<HighlightingToken> collect(ParsedAST &AST) && {
     auto SortedSkippedRanges = AST.getMacros().SkippedRanges;
     llvm::sort(SortedSkippedRanges);
     auto It = NonConflicting.begin();
     for (const Range &R : SortedSkippedRanges) {
       // Create one token for each line in the skipped 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)

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