FoldingRanges: Handle LineFoldingsOnly clients.

[usaxena95]

Summary

Do not fold the endline which contains tokens after the end of range.

[usaxena95]

Do same for multi-line comments.

[usaxena95]

More tests.

[kadircet]

thanks, i think we should change the behaviour to not fold the last line in any case.

clang-tools-extra/clangd/ClangdLSPServer.cpp

949

normally this is an LSP specific behaviour, hence we should do the conversion of clangd internal representation here (in the LSP layer). but it feels like the line-only foldings are actually the sensible default for most of the clients so it makes sense to do it inside the clangdserver (also the information needed might not be present at this point.

but still rather than passing it with every request, let's make it an option of clangdserver and don't pass it here.

clang-tools-extra/clangd/ClangdLSPServer.h

263 ↗

(On Diff #449931)

LineFoldingOnly (as spelled in LSP)

clang-tools-extra/clangd/SemanticSelection.cpp

203

can we do this as part of the adjust logic at the bottom?

moreover, i don't think we actually need to care about these. spec is clear about these values being ignored by line-folding-only clients anyway.

224

i don't think the extra check for "if there are more tokens" is really useful here, in a scenario like:

if (foo) {
  ...
}

do we actually want to fold away the closing } as well ? (i don't think we do)

226

nit: early bail out

if(!LineFoldingsOnly) return;

231–233

similarly no consts here

248–252

let's not introduce the consts here (i know the clang-tidy check is complaining but the check itself should be disabled for LLVM instead, that's not what the codebase adheres to)

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

386

so IIUC outer-most folding here will imply:

void func(int a) {
...

which doesn't feel right, i think it should look like:

void func(int a) {
...
}

which implies not having the last line included in the range.

similarly for the folding near else i think we would get:

void func(int a) {
  if(a == 1) {
   a++;
  } else {
   ...
}

which again doesn't look right (as user can still see the opening brace, but not the closing brace), we shouldn't be folding the closing } in any case.

405

it'd be nice to have a test case like:

template <[[typename foo, class bar]]> struct baz {[[]]};

which isn't useful for line-only folding clients, but something we should be able to support going forward.

[hokein]

In D131154#3706062, @kadircet wrote:

thanks, i think we should change the behaviour to not fold the last line in any case.

+1, I think we should always do it for all bracket cases, but we probably don't want it for other cases (comment), right?

clang-tools-extra/clangd/Protocol.cpp

395

nit: remove the {} for the single-statement body.

clang-tools-extra/clangd/SemanticSelection.cpp

224

Yeah, it also introduces some inconsistencies (depending on whether there are trailing comments after the closed brackets).

We probably want the folding to work the same way for the following cases.

namespace foo {
 ...
} // namespace foo

namespace foo {
  ...
}

[kadircet]

clang-tools-extra/clangd/SemanticSelection.cpp

224

right. i think for comments we've got a bunch of different cases. for the block comments, i think we want to exclude the last line all the time again, e.g:

/*[[ foo
     bar
]]*/

i got 2 reasons:
1- this is somewhat the same as being able to see opening and closing brace.
2- when there's any token following the comment-block terminator, we actually mustn't hide them.

for back-to-back single-line comments, we should always fold the last line as well, eg:

//[[ foo
// bar]]

so i think the model here is always include the first line and don't include the last line if there are tokens after the folding range on the same line, as you've already proposed here, but we need to make sure that folding range doesn't include the terminators (like } and */). That way we'll ensure a line with a terminator is always visible.

[usaxena95]

Addressed comments.

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

405

I don't understand. We do not return any single-line ranges.
I think lines are never long enough to have helpful foldings (atleast in formatted code). It would only clutter IMO.

[kadircet]

clang-tools-extra/clangd/SemanticSelection.cpp

203

can you put it back to its previous location (i.e. right after startline)

261

i think this will end up trimming the foldings for cases like:

// foo\
bar\
baz

can we check the comment introducer instead? i.e. // vs /*. also let's amend the startposition to be Startoffset +2 similar to what we do with braces, to make sure starting sentinel is not included in the folding range.

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

405

I don't understand. We do not return any single-line ranges.

I think that's just a convenient solution we went with today because it's convenient and easier. i don't think that's the final UX we want to have.

I think lines are never long enough to have helpful foldings (atleast in formatted code). It would only clutter IMO.

Even if that was the case template arguments can get really long, especially in the presence of SFINAE, so there's definitely value in folding them.

Also just to be clear, i wasn't suggesting to have the implementation for them. I was just saying let's leave the testcases here, in disabled state, to remind us to have support for them in the future (and properly handle even for single line case).

[usaxena95]

Address comments.

[kadircet]

clang-tools-extra/clangd/SemanticSelection.cpp

260

this is actually not true, we might have // foo */, so we need to check if it has /* in the beginning again.

also can we have it in an explicit variable instead bool IsBlockComment

265

so it actually gets pretty annoying with mixture of block and line comments back to back, but also with multiple block comments back to back, e.g:

/* foo
  * bar*/

vs

/* foo */
/* bar */

vs

// foo
/* bar */

which is rare, so i don't think it's worth handling. but at least to make it consistent, rather than checking for the start/end lines of the lastcomment, we should actually be looking at the whole range and whether we've started the range with a blockcomment. (sorry for noticing it too late)

[usaxena95]

Addressed comments.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/ClangdLSPServer.cpp

if (Params.capabilities.WorkspaceSymbolKinds)
     SupportedSymbolKinds |= *Params.capabilities.WorkspaceSymbolKinds;
   if (Params.capabilities.CompletionItemKinds)
     SupportedCompletionItemKinds |= *Params.capabilities.CompletionItemKinds;
   SupportsCodeAction = Params.capabilities.CodeActionStructure;
   SupportsHierarchicalDocumentSymbol =
       Params.capabilities.HierarchicalDocumentSymbol;
   SupportFileStatus = Params.initializationOptions.FileStatus;
   HoverContentFormat = Params.capabilities.HoverContentFormat;
+  Opts.LineFoldingOnly = Params.capabilities.LineFoldingOnly;
   SupportsOffsetsInSignatureHelp = Params.capabilities.OffsetsInSignatureHelp;
   if (Params.capabilities.WorkDoneProgress)
     BackgroundIndexProgressState = BackgroundIndexProgress::Empty;
   BackgroundIndexSkipCreate = Params.capabilities.ImplicitProgressCreation;
   Opts.ImplicitCancellation = !Params.capabilities.CancelsStaleRequests;
 
   llvm::json::Object ServerCaps{
       {"textDocumentSync",
Server->documentSymbols(
           return Reply(std::move(*Items));
         return Reply(flattenSymbolHierarchy(*Items, FileURI));
       });
 }
 
 void ClangdLSPServer::onFoldingRange(
     const FoldingRangeParams &Params,
     Callback<std::vector<FoldingRange>> Reply) {
   Server->foldingRanges(Params.textDocument.uri.file(), std::move(Reply));
 }
 
 static llvm::Optional<Command> asCommand(const CodeAction &Action) {
   Command Cmd;
   if (Action.command && Action.edit)
     return None; // Not representable. (We never emit these anyway).
   if (Action.command) {
     Cmd = *Action.command;

clang-tools-extra/clangd/ClangdServer.h

struct Options {
 
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable preview of FoldingRanges feature.
     bool FoldingRanges = false;
 
+    // Whether the client supports folding only complete lines.
+    bool LineFoldingOnly = false;
+
     FeatureModuleSet *FeatureModules = nullptr;
     /// If true, use the dirty buffer contents when building Preambles.
     bool UseDirtyHeaders = false;
 
     // If true, parse emplace-like functions in the preamble.
     bool PreambleParseForwardingFunctions = false;
 
     explicit operator TUScheduler::Options() const;
private:
   // Storage for merged views of the various indexes.
   std::vector<std::unique_ptr<SymbolIndex>> MergedIdx;
 
   // When set, provides clang-tidy options for a specific file.
   TidyProviderRef ClangTidyProvider;
 
   bool UseDirtyHeaders = false;
 
+  // Whether the client supports folding only complete lines.
+  bool LineFoldingOnly = false;
+
   bool PreambleParseForwardingFunctions = false;
 
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   llvm::Optional<std::string> WorkspaceRoot;

clang-tools-extra/clangd/ClangdServer.cpp

 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const ThreadsafeFS &TFS, const Options &Opts,
                            Callbacks *Callbacks)
     : FeatureModules(Opts.FeatureModules), CDB(CDB), TFS(TFS),
       DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
       ClangTidyProvider(Opts.ClangTidyProvider),
       UseDirtyHeaders(Opts.UseDirtyHeaders),
+      LineFoldingOnly(Opts.LineFoldingOnly),
       PreambleParseForwardingFunctions(Opts.PreambleParseForwardingFunctions),
       WorkspaceRoot(Opts.WorkspaceRoot),
       Transient(Opts.ImplicitCancellation ? TUScheduler::InvalidateOnUpdate
                                           : TUScheduler::NoInvalidation),
       DirtyFS(std::make_unique<DraftStoreFS>(TFS, DraftMgr)) {
   if (Opts.AsyncThreadsCount != 0)
     IndexTasks.emplace();
   // Pass a callback into `WorkScheduler` to extract symbols from a newly

 
 void ClangdServer::foldingRanges(llvm::StringRef File,
                                  Callback<std::vector<FoldingRange>> CB) {
   auto Code = getDraft(File);
   if (!Code)
     return CB(llvm::make_error<LSPError>(
         "trying to compute folding ranges for non-added document",
         ErrorCode::InvalidParams));
-  auto Action = [CB = std::move(CB), Code = std::move(*Code)]() mutable {
-    CB(clangd::getFoldingRanges(Code));
+  auto Action = [LineFoldingOnly = LineFoldingOnly, CB = std::move(CB),
+                 Code = std::move(*Code)]() mutable {
+    CB(clangd::getFoldingRanges(Code, LineFoldingOnly));
   };
   // We want to make sure folding ranges are always available for all the open
   // files, hence prefer runQuick to not wait for operations on other files.
   WorkScheduler->runQuick("FoldingRanges", File, std::move(Action));
 }
 
 void ClangdServer::findType(llvm::StringRef File, Position Pos,
                             Callback<std::vector<LocatedSymbol>> CB) {

clang-tools-extra/clangd/Protocol.h

struct ClientCapabilities {
   /// Client supports hierarchical document symbols.
   /// textDocument.documentSymbol.hierarchicalDocumentSymbolSupport
   bool HierarchicalDocumentSymbol = false;
 
   /// Client supports signature help.
   /// textDocument.signatureHelp
   bool HasSignatureHelp = false;
 
+  /// Client signals that it only supports folding complete lines.
+  /// Client will ignore specified `startCharacter` and `endCharacter`
+  /// properties in a FoldingRange.
+  /// textDocument.foldingRange.lineFoldingOnly
+  bool LineFoldingOnly = false;
+
   /// Client supports processing label offsets instead of a simple label string.
   /// textDocument.signatureHelp.signatureInformation.parameterInformation.labelOffsetSupport
   bool OffsetsInSignatureHelp = false;
 
   /// The documentation format that should be used for
   /// textDocument/signatureHelp.
   /// textDocument.signatureHelp.signatureInformation.documentationFormat
   MarkupKind SignatureHelpDocumentationFormat = MarkupKind::PlainText;

clang-tools-extra/clangd/Protocol.cpp

if (auto *Help = TextDocument->getObject("signatureHelp")) {
                 Info->getArray("documentationFormat")) {
           for (const auto &Format : *DocumentationFormat) {
             if (fromJSON(Format, R.SignatureHelpDocumentationFormat, P))
               break;
           }
         }
       }
     }
+    if (auto *Folding = TextDocument->getObject("foldingRange")) {
+      if (auto LineFolding = Folding->getBoolean("lineFoldingOnly"))
+        R.LineFoldingOnly = *LineFolding;
+    }
     if (auto *Rename = TextDocument->getObject("rename")) {
       if (auto RenameSupport = Rename->getBoolean("prepareSupport"))
         R.RenamePrepareSupport = *RenameSupport;
     }
   }
   if (auto *Workspace = O->getObject("workspace")) {
     if (auto *Symbol = Workspace->getObject("symbol")) {
       if (auto *SymbolKind = Symbol->getObject("symbolKind")) {

clang-tools-extra/clangd/SemanticSelection.h

 
 /// Returns a list of ranges whose contents might be collapsible in an editor.
 /// This should include large scopes, preprocessor blocks etc.
 llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST);
 
 /// Returns a list of ranges whose contents might be collapsible in an editor.
 /// This version uses the pseudoparser which does not require the AST.
 llvm::Expected<std::vector<FoldingRange>>
-getFoldingRanges(const std::string &Code);
+getFoldingRanges(const std::string &Code, bool LineFoldingOnly);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICSELECTION_H

clang-tools-extra/clangd/SemanticSelection.cpp

llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST) {
   return collectFoldingRanges(SyntaxTree, TM);
 }
 
 // FIXME( usaxena95): Collect PP conditional regions, includes and other code
 // regions (e.g. public/private/protected sections of classes, control flow
 // statement bodies).
 // Related issue: https://github.com/clangd/clangd/issues/310
 llvm::Expected<std::vector<FoldingRange>>
-getFoldingRanges(const std::string &Code) {
+getFoldingRanges(const std::string &Code, bool LineFoldingOnly) {
   auto OrigStream = pseudo::lex(Code, clang::pseudo::genericLangOpts());
 
   auto DirectiveStructure = pseudo::DirectiveTree::parse(OrigStream);
   pseudo::chooseConditionalBranches(DirectiveStructure, OrigStream);
 
   // FIXME: Provide ranges in the disabled-PP regions as well.
   auto Preprocessed = DirectiveStructure.stripDirectives(OrigStream);
 
   auto ParseableStream = cook(Preprocessed, clang::pseudo::genericLangOpts());
   pseudo::pairBrackets(ParseableStream);
 
   std::vector<FoldingRange> Result;
-  auto ToFoldingRange = [](Position Start, Position End,
+  auto AddFoldingRange = [&](Position Start, Position End,
                              llvm::StringLiteral Kind) {
+    if (Start.line >= End.line)
+      return;
     FoldingRange FR;
     FR.startLine = Start.line;
     FR.startCharacter = Start.character;
     FR.endLine = End.line;
     FR.endCharacter = End.character;
     FR.kind = Kind.str();
-    return FR;
+    Result.push_back(FR);
   };
   auto OriginalToken = [&](const pseudo::Token &T) {
     return OrigStream.tokens()[T.OriginalIndex];
   };
   auto StartOffset = [&](const pseudo::Token &T) {
     return OriginalToken(T).text().data() - Code.data();
   };
   auto StartPosition = [&](const pseudo::Token &T) {
     return offsetToPosition(Code, StartOffset(T));
   };
+  auto EndOffset = [&](const pseudo::Token &T) {
+    return StartOffset(T) + OriginalToken(T).Length;
+  };
   auto EndPosition = [&](const pseudo::Token &T) {
-    return offsetToPosition(Code, StartOffset(T) + OriginalToken(T).Length);
+    return offsetToPosition(Code, EndOffset(T));
   };
   auto Tokens = ParseableStream.tokens();
   // Brackets.
   for (const auto &Tok : Tokens) {
     if (auto *Paired = Tok.pair()) {
       // Process only token at the start of the range. Avoid ranges on a single
       // line.
       if (Tok.Line < Paired->Line) {
         Position Start = offsetToPosition(Code, 1 + StartOffset(Tok));
         Position End = StartPosition(*Paired);
-        Result.push_back(ToFoldingRange(Start, End, FoldingRange::REGION_KIND));
+        if (LineFoldingOnly)
+          End.line--;
+        AddFoldingRange(Start, End, FoldingRange::REGION_KIND);
       }
     }
   }
+  auto IsBlockComment = [&](const pseudo::Token &T) {
+    assert(T.Kind == tok::comment);
+    return OriginalToken(T).Length >= 2 &&
+           Code.substr(StartOffset(T), 2) == "/*";
+  };
   // Multi-line comments.
-  for (const auto *T = Tokens.begin(); T != Tokens.end();) {
+  for (auto *T = Tokens.begin(); T != Tokens.end();) {
     if (T->Kind != tok::comment) {
       T++;
       continue;
     }
-    Position Start = StartPosition(*T);
-    Position LastCommentEnd = EndPosition(*T);
+    pseudo::Token *FirstComment = T;
+    // Show starting sentinals (// and /*) of the comment.
+    Position Start = offsetToPosition(Code, 2 + StartOffset(*FirstComment));
+    pseudo::Token *LastComment = T;
+    Position End = EndPosition(*T);
     while (T != Tokens.end() && T->Kind == tok::comment &&
-           StartPosition(*T).line <= LastCommentEnd.line + 1) {
-      LastCommentEnd = EndPosition(*T);
+           StartPosition(*T).line <= End.line + 1) {
+      End = EndPosition(*T);
+      LastComment = T;
       T++;
     }
-    if (Start.line < LastCommentEnd.line)
-      Result.push_back(
-          ToFoldingRange(Start, LastCommentEnd, FoldingRange::COMMENT_KIND));
+    if (IsBlockComment(*FirstComment)) {
+      if (LineFoldingOnly)
+        // Show last line of a block comment.
+        End.line--;
+      if (IsBlockComment(*LastComment))
+        // Show ending sentinal "*/" of the block comment.
+        End.character -= 2;
+    }
+    AddFoldingRange(Start, End, FoldingRange::COMMENT_KIND);
   }
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

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

ASSERT_TRUE(bool(Ranges))
       << "getSemanticRange returned an error: " << Ranges.takeError();
   ASSERT_EQ(Ranges->size(), SourceAnnotations.points().size());
   EXPECT_THAT(gatherRanges(Ranges->front()),
               ElementsAreArray(SourceAnnotations.ranges()));
   EXPECT_THAT(gatherRanges(Ranges->back()),
               ElementsAre(SourceAnnotations.range("empty")));
 }
 
-TEST(FoldingRanges, All) {
+TEST(FoldingRanges, ASTAll) {
   const char *Tests[] = {
       R"cpp(
         #define FOO int foo() {\
           int Variable = 42; \
         }
 
         // Do not generate folding range for braces within macro expansion.
         FOO
for (const char *Test : Tests) {
     auto T = Annotations(Test);
     auto AST = TestTU::withCode(T.code()).build();
     EXPECT_THAT(gatherFoldingRanges(llvm::cantFail(getFoldingRanges(AST))),
                 UnorderedElementsAreArray(T.ranges()))
         << Test;
   }
 }
 
-TEST(FoldingRangesPseudoParser, All) {
+TEST(FoldingRanges, PseudoParserWithoutLineFoldings) {
   const char *Tests[] = {
       R"cpp(
         #define FOO int foo() {\
           int Variable = 42; \
         }
 
         // Do not generate folding range for braces within macro expansion.
         FOO
const char *Tests[] = {
         {[[  \
         public:
           Foo() {[[\
             int X = 1;
           ]]}   \
         ]]};
       )cpp",
       R"cpp(
-        [[/* Multi 
+        /*[[ Multi 
           * line
           *  comment 
-          */]]
+          ]]*/
       )cpp",
       R"cpp(
-        [[// Comment
+        //[[ Comment
         // 1]]
         
-        [[// Comment
+        //[[ Comment
         // 2]]
         
         // No folding for single line comment.
 
-        [[/* comment 3
-        */]]
+        /*[[ comment 3
+        ]]*/
 
-        [[/* comment 4
-        */]]
+        /*[[ comment 4
+        ]]*/
+
+        /*[[ foo */
+        /* bar ]]*/
+
+        /*[[ foo */
+        // baz
+        /* bar ]]*/
+
+        /*[[ foo */
+        /* bar*/
+        // baz]]
+
+        //[[ foo
+        /* bar */]]
       )cpp",
   };
   for (const char *Test : Tests) {
     auto T = Annotations(Test);
-    EXPECT_THAT(
-        gatherFoldingRanges(llvm::cantFail(getFoldingRanges(T.code().str()))),
+    EXPECT_THAT(gatherFoldingRanges(llvm::cantFail(getFoldingRanges(
+                    T.code().str(), /*LineFoldingsOnly=*/false))),
                 UnorderedElementsAreArray(T.ranges()))
         << Test;
   }
 }
 
+TEST(FoldingRanges, PseudoParserLineFoldingsOnly) {
+  const char *Tests[] = {
+      R"cpp(
+        void func(int a) {[[
+            a++;]]
+        }
+      )cpp",
+      R"cpp(
+        // Always exclude last line for brackets.
+        void func(int a) {[[
+          if(a == 1) {[[
+            a++;]]
+          } else if (a == 2){[[
+            a--;]]
+          } else {  // No folding for 2 line bracketed ranges.
+          }]]
+        }
+      )cpp",
+      R"cpp(
+        /*[[ comment
+        * comment]]
+        */
+
+        /* No folding for this comment.
+        */
+
+        // No folding for this comment.
+
+        //[[ 2 single line comment.
+        // 2 single line comment.]]
+
+        //[[ >=2 line comments.
+        // >=2 line comments.
+        // >=2 line comments.]]
+
+        //[[ foo\
+        bar\
+        baz]]
+
+        /*[[ foo */
+        /* bar */]]
+        /* baz */
+
+        /*[[ foo */
+        /* bar]]
+        * This does not fold me */
+
+        //[[ foo
+        /* bar */]]
+      )cpp",
+      // FIXME: Support folding template arguments.
+      // R"cpp(
+      // template <[[typename foo, class bar]]> struct baz {};
+      // )cpp",
+
+  };
+  auto StripColumns = [](const std::vector<Range> &Ranges) {
+    std::vector<Range> Res;
+    for (Range R : Ranges) {
+      R.start.character = R.end.character = 0;
+      Res.push_back(R);
+    }
+    return Res;
+  };
+  for (const char *Test : Tests) {
+    auto T = Annotations(Test);
+    EXPECT_THAT(
+        StripColumns(gatherFoldingRanges(llvm::cantFail(
+            getFoldingRanges(T.code().str(), /*LineFoldingsOnly=*/true)))),
+        UnorderedElementsAreArray(StripColumns(T.ranges())))
+        << Test;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang