[clangd] Plan features for FoldingRanges

[kbobyrev]

Summary

This is not an actual patch, this is an RFC + TDD plan for future RAV
implementation. Hopefully, the implementation would gradually make all of the
tests from this snippet pass.

[kbobyrev]

Fix some typos.

[kbobyrev]

Add some missing ranges.

[sammccall]

A few different types of feedback mixed up here :-)

Some nitpicks about behavior: these are probably worth discussing.

Places where the coverage is incomplete: obviously until we get closer to implementation, how complete this is is up to you.

Looking at examples mostly reinforced to me that foldability should have some threshold for what's inside as well as rules about grammatically what's foldable. I just can't see editors doing a good job of discarding uninteresting ranges - they're going to consider that our job.
If "only things spanning newlines" is too strict, we could consider maybe other complexity heuristics.

At least for the AST-based folds, I'd like some formalism like:

• we build a tree of potentially-foldable regions based on the AST
• starting at the bottom, we decide whether each potentially-foldable region is actually folded
  • any region containing a non-folded newline is folded
  • any bracket-delimited region containing a non-folded comma or semicolon token is folded

Regarding implementation and RAV: we talked about the possibility of using syntax trees, is that still on the table?

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

237

nit: the subexpression tests are mixed in with the if cases

246

confused about [[++I]] - why is this foldable but not I > 42 etc - is this because of the containing parens?

FWIW, Personally, I'd want the whole if condition to be foldable (maybe only if it spans lines), and only paren subexpressions, arg lists etc that span lines to be foldable. It's hard to imagine a UX that could expose folding small parens in a way that's not annoying. (e.g. shows lots of extra UI elements, or prevents folding the outer expression)

277

this is a really nice model, but I worry that having multiple folding ranges starting at the same point within a line is going to be a UX nightmare.
I'd suggest picking one.

(Note that other control flow statements can look like for loops too these days, with initializer statements)

329

missing lambdas, blocks, objc methods.
cover bodies in the same tests?
capture lists of lambdas?

342

linkage specs probably belong here too

414

why is the template argument list not foldable here? (it is for the class example above)

552

I think clang-format messed some of these up for you

578

I think consistent with how you handle braces and comments, the range should end just before the #endif

580

I don't think you want the condition as part of the same of the same fold as the content, it might be important

What about

#if [[some_condition]][[
body
]]#elif [[some_condition]][[
body
]]#else[[
body
]]#endif

Rendering (partially folded) as:
#if some_condition ... #elif some_condition ... #else ... #endif

583

here I'd expect folding before # so you can see it's a directive when folded

605

I think YAGNI here - not sure how this could help, and it's an obscure feature to start with

613

Similarly.
*Maybe* generically folding directives that span multiple lines with \. But I think they're rare enough nobody cares.

628

would be lovely to cover objc @interface/@implementation too...

656

You're missing the case where there's stuff in the implicit initial section, and then another accessor section.

It's an interesting one because if you allow the implicit section to be folded, and also the whole class to be folded, then you have two folds triggered at the same location. (Currently there's only one [[[[ in this file, which i've flagged)

718

nit: these are "member initializer lists", not to be confused with "initializer lists" which are the braced things (and should also be covered, I guess!)

[kbobyrev]

Will resolve the comments and add some missing examples.

[kbobyrev]

Resolve most comments, make sure everything builds.

[kbobyrev]

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

246

Sounds good! Added a separate expressions test to Misc, too.

329

What kind of blocks? Do you mean statement groups? What do you mean by "cover bodies in the same tests"? What kind of bodies?

[sammccall]

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

329

What kind of blocks?

Parallel-universe lambdas

https://clang.llvm.org/doxygen/classclang_1_1BlockDecl.html#details

(Commonly used in the apple world I think, though it's not actually an extension to objc but rather c)

What do you mean by "cover bodies in the same tests"?

You have to go through the different ways to write functions here, I was just suggesting making sure they are definitions so you can test their body ranges too...

[hokein]

just some random comments from my side.

At least for the AST-based folds, I'd like some formalism like:

+1. I think we need some general rules (mental model) to guide us how the folding works.

Some editors (e.g. VSCode) have their builtin folding-range, it is worth to investigate how the built folding-range interacts with one provided by the language server.

From my experience, VSCode's built folding-range kind of works though this is not perfect, would be nice to have some concrete examples see the improvements, we may want to prioritize those during the implementation.

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

261

this example seems vague, it is folded because

1. it is cross-line
2. it is wrapped by ()

or both?

284

nit: for completeness, add a for-range case.

326

not sure about this case.

if we decide to support this (btw, the test on L242 should be adjusted too), would be nice to support the following usages.

// forward decls
[[class A;
class B;
class C;]]

// using decls;
[[using ns::A;
using ns::B;
using ns::C;]]

556

this [[ seems mismatched, if I read the code correctly.

btw, this example contains too many brackets, considering split into simpler ones.

598

another option: we could just fold the whole preamble region, it seems easier, and provides a visible to see preamble range :), the downside is that any comments/macros will be folded together.

601

These macros are quite simple, I'm not sure this is useful to fold them. I'd just not fold it, but if the macro body is complicated (multi-lines), it is worth folding it.

679

looks like we're missing case of inheritance, e.g. a class inherits multiple classes

class B : public A,
  private C {};

[kbobyrev]

Need to resolve the comments.

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

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(FoldingRanges, ControlFlow) {
+  const char *Tests[] = {
+      // If.
+      R"cpp(
+        int main() {[[
+          bool B = true;
+          int I = 0;
+          char C = 'z';
+
+          if (B && I > 42 || ~(++I)) {[[
+            ++I;
+          ]]} else {[[
+            B = false;
+          ]]}
+
+          if (B && (!B) || !((C == 'a'))) {[[
+            ++I;
+          ]]} else if ([[!B && true != false &&
+                       2 * 2 == 4]]) {[[
+            --I;
+          ]]} else {[[
+            C = 'a';
+          ]]}
+
+          if (B && ([["subexpression" != "multi-line" &&
+                    1 > 0]]))
+            B = false;
+
+          if ([[int Init = 0, Variable = 9000;
+              [[Init == 0 && Init > 0 ||
+              Init < 5000]]]])
+            Init = 42;
+        ]]}
+      )cpp",
+      // While.
+      R"cpp(
+        int main() {[[
+          bool B;
+          while ([[B]]) {[[
+            B = !B;
+          ]]}
+
+          do {[[
+            B = !B;
+          ]]} while ([[B]]);
+        ]]}
+      )cpp",
+      // For.
+      R"cpp(
+        int main() {[[
+          for ([[int I = 0;I < 42; ++I]]) {[[
+            --I;
+          ]]}
+        ]]}
+      )cpp",
+      // Switch.
+      R"cpp(
+        void noop();
+
+        int main() {[[
+          int i = 2;
+          switch ([[i]]) {[[
+              case 1:[[ noop();]]
+              case 2:[[ noop(); //[[execution starts at this case label]]]]
+              case 3:[[ noop();]]
+              case 4:
+              case 5:[[ noop();
+                      break;              //[[execution of subsequent statements is terminated]]]]
+              case 6:[[ noop();]]
+          ]]}
+        ]]}
+      )cpp",
+  };
+  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(FoldingRanges, Misc) {
+  const char *Tests[] = {
+      // Statement groups.
+      R"cpp(
+        int main() {[[
+          [[int X = 5;
+          int Y = 42;
+          bool B = 15;]]
+          if ([[B]]) {[[ ++X; ]]}
+          unsigned U = 9000;
+        ]]}
+      )cpp",
+      // Expressions (fold if they span multiple lines).
+      R"cpp(
+        int main() {[[
+          bool B = 42 > 0;
+          B = (42 > 0);
+          B = (42 > 0 && 9000 > 2000);
+          B = 42 > 0 && (9000 > 2000 && 2000 > 1000);
+          B = [[42 > 0 && ([[9000 > 2000 &&
+                         2000 > 1000]])]];
+        ]]}
+      )cpp",
+      // Enum.
+      R"cpp(
+        enum Color {[[
+          Green = 0,
+          YInMn,
+          Orange,
+        ]]};
+      )cpp",
+      // Argument lists.
+      R"cpp(
+        int foo([[char C, bool B]]) {[[ return static_cast<int>(C); ]]}
+
+        foo([['z', true]]);
+
+        struct Foo {
+          Foo([[int I, unsigned U, bool B=true]]) {}
+        };
+
+        Foo F = Foo([[/*[[I=]]*/1, /*[[U=]]*/2, /*[[B=]]false]]);
+        F = Foo([[1, 2]]);
+      )cpp",
+      // Namespace.
+      R"cpp(
+        namespace ns {[[
+        int Variable = 42;
+        namespace nested {[[
+        int NestedVariable = 50;
+        ]]}
+        ]]}
+
+        namespace a {[[
+        namespace b {[[
+        namespace c {[[
+
+        ]]} //[[ namespacee c]]
+        ]]} //[[ namespacee b]]
+        ]]} //[[ namespacee a]]
+
+        namespace modern::ns::syntax {[[
+        ]]} //[[ namespace modern::ns::syntax]]
+
+        namespace {[[
+        ]]} //[[ namespace]]
+
+        extern "C" {[[
+          void Foo();
+        ]]}
+      )cpp",
+      // Strings.
+      R"cpp(
+        std::string String = "[[ShortString]]";
+        String = "[[Suuuuuuuuuuuuuuuuuper Looooooooooooooooong String]]";
+        String = u8"[[Suuuuuuuuuuuuuuuuuper Looooooooooooooooong String]]";
+
+        String = R"raw([[Suuuuuuuuuuuuuuuuuper Looooooooooooooong String]])raw";
+
+        const char *text =
+          "[[This"
+          "is a multiline"
+          "string]]";
+
+        const char *text2 =
+          "[[Here is another \
+        string \
+        also \
+        multilineeeeee]]";
+      )cpp",
+      // Arrays.
+      R"cpp(
+        char Array[] = {[[ 'F', 'o', 'o', 'b', 'a', 'r', '\0' ]]};
+
+        Array = {[[ 'F', 'o', 'o', 'b', 'a', 'r', 'F', 'o', 'o', 'b',
+                 'a', 'r', '\0' ]]};
+
+        int Nested[3][4] = {[[{[[0,1,2,3]]}, {[[4,5,6,7]]}, {[[8,9,10,11]]}]]};
+      )cpp",
+      // Templates.
+      R"cpp(
+        template <[[typename T, typename U, typename V, int X, char C, bool B]]>
+        class Foo {};
+
+        template <[[bool B]]>
+        class Foo<[[unsigned, unsigned, unsigned, -42, 'z', B]]> {
+          int Specialization;
+        };
+
+        template <>
+        class Foo<[[unsigned, unsigned, unsigned, -42, 'z', false]]> {
+          int Specialization;
+        };
+
+        Foo<[[char, int, bool, 42, 'x', false]]> F;
+
+        template <[[typename T, unsigned U]]>
+        void foo([[T t, unsigned u]]) {[[ return; ]]}
+
+        template <[[unsigned U]]>
+        void foo<[U, 50]>([[char t, U u]]) {}
+
+        template <>
+        void foo<[char, 42]>([[char t, U u]]) {}
+      )cpp",
+      // Lambdas.
+      R"cpp(
+        int main() {[[
+          bool B = true;
+          int I = 42;
+          auto Lambda = [=]([[auto a, auto&& b]]) {[[ return a < b; ]]};
+          auto vglambda = [[[=, &B, &I]]]([[auto printer]]) {
+              return [=](auto&&... ts)
+              {
+                  return [=] { printer(ts...); };
+              };
+          };
+        ]]}
+      )cpp",
+  };
+  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(FoldingRanges, Comments) {
+  const char *Tests[] = {
+      R"cpp(
+        //[[ This is a one-line comment.]]
+      )cpp",
+      R"cpp(
+        //[[ Continuous
+        // Comment
+        // ...]]
+
+        //[[ And another one
+        // But doesn't belong to the one above]]
+
+        //[[ This time
+        //
+        // It's connected!]]
+
+        ///[[ Doxygen]]
+
+        ///[[ Doxygen
+        /// With more multiple lines.
+        ///
+        ///
+        /// The End]]
+      )cpp",
+      R"cpp(
+        /*[[ C-Style comment. ]]*/
+
+        [[/* Spanning
+         * multiple
+         * lines */]]
+      )cpp",
+      R"cpp(
+        [[/* C-Style
+         * multiple
+         * lines */]]
+        //[[ Followed by C++ Style
+        // Are still different comments!]]
+      )cpp",
+      R"cpp(
+        /*[[!
+         * ... text ...
+         ]]*/
+
+        /*[[*
+         * ... text ...
+         ]]*/
+
+        /*[[!
+         ... text ...
+        ]]*/
+
+        ///[[
+        /// ... text ...
+        ///]]
+
+        //[[!
+        //!... text ...
+        //!]]
+
+        /*[[*******************************************//**
+        *  ... text
+        **********************************************]]*/
+
+        //[[///////////////////////////////////////////////
+        /// ... text ...
+        /////////////////////////////////////////////////]]
+
+        /*[[***********************************************
+         *  ... text
+         **********************************************]]*/
+      )cpp",
+      R"cpp(
+        /*[[
+          Tricky C-Style
+
+          void Foo(int I);
+          void Foo(int I, char C);
+        /*]]*/
+      )cpp",
+      R"cpp(
+        int Foo([[int I, bool Boolean=false]]) {[[
+          return 42; //[[ Do nothing, just return!]]
+        ]]}
+
+        const auto Variable = Foo([[42, /*[[Boolean=]]*/true]]);
+      )cpp",
+      R"cpp(
+        //[[ Some documentation.
+        // @param
+        int Foo([[int I, bool Boolean=false]]) {[[ //[[ With a comment]]
+          return 42; //[[ Do nothing, just return!]]
+        ]]}
+
+        const auto Variable = Foo([[42, /*[[Boolean=]]*/]]true]]);
+      )cpp",
+      R"cpp(
+        //[[ Empty comments don't generate folding ranges!]]
+
+        //
+        /**/
+        ///
+      )cpp",
+  };
+  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(FoldingRanges, Preprocessor) {
+  const char *Tests[] = {
+      R"cpp(
+        #include [[<cmath>
+        #include <cstdio>
+        #include <iostream>]]
+
+        #include [[<vector>]]
+
+        #include [["external/Logger.h"
+        #include "external/Vector.h"
+        #include "project/DataStructures/Trie.h"]]
+
+        #include [["project/File.h"
+        #include <map>
+        #include <set>]]
+
+        #include [["math.h"]]
+      )cpp",
+      R"cpp(
+        #define true [[false]]
+
+        #define getmax(a,b) [[((a)>(b)?(a):(b))]]
+        #define PI [[3.14159]]
+      )cpp",
+      R"cpp(
+        #ifndef [[FOO_H_INCLUDED]][[ /*[[ any name uniquely mapped to file name ]]*/
+        #define FOO_H_INCLUDED
+        //[[ ^ Don't fold simple definition?]]
+
+        //[[ contents of the file are here]]
+        ]]#endif
+
+        #ifndef [[LIBRARY_FILENAME_H]][[
+        #define LIBRARY_FILENAME_H
+        //[[ contents of the header]]
+        ]]#endif /*[[ LIBRARY_FILENAME_H ]]*/
+
+        #if [[__has_include(<optional>)]][[
+        #  include <optional>
+        #  define have_optional 1
+        ]]#elif [[__has_include(<experimental/optional>)]][[
+        #  include <experimental/optional>
+        #  define have_optional 1
+        #  define experimental_optional 1
+        ]]#else[[
+        #  define have_optional 0
+        ]]#endif
+
+        #if [[OK]][[
+        # define NICE
+        ]]#else[[
+        #  define true [[false]]
+        ]]#endif
+
+        #ifdef [[SOMETHING]][[
+        //[[ Oh no!]]
+        #undef SOMETHING
+        ]]#else[[
+        #define OKAY_GOOD
+        ]]#endif
+      )cpp",
+  };
+  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(FoldingRanges, Classes) {
+  const char *Tests[] = {
+      // Basics.
+      R"cpp(
+        class Foo {[[
+          int I;
+        ]]};
+
+        struct Bar {[[
+          int I;
+        ]]};
+
+        class Baz {[[
+        public:[[
+          Baz();
+
+          int I;]]
+        ]]};
+
+        class Chewbacca {};
+        class Vader;
+
+        struct FooStruct {[[
+          int Field;
+        ]]};
+        struct BarStruct {[[]]};
+        struct FwdDeclaration;
+      )cpp",
+      // Accessor secions.
+      R"cpp(
+        class Foo {[[
+        public:
+        private:
+        protected:
+        public:
+        ]]};
+
+        class Bar {[[
+        public:[[
+          Bar();
+          Bar([[int X]]);
+]]
+        private:[[
+          int Field;]]
+        protected:[[
+          bool ProtectThis;]]
+        public:[[
+          int getField() {[[ return Field; ]]}]]
+        ]]};
+
+        class ImplicitFirstSection {[[[[
+          ImplicitFirstSection() {}
+
+          int Field;]]
+        public:[[
+
+          int getField();]]
+        ]]};
+      )cpp",
+      // Nested classes with accessor sections.
+      R"cpp(
+        class Nested {[[
+        public:
+        private:[[
+          class Inner {[[
+          public:[[
+            Inner();
+            Inner(int X);
+  ]]
+          private:[[
+            int Field;]]
+          protected:[[
+            bool ProtectThis;]]
+          public:[[
+            int getField() {[[ return Field; ]]}]]
+          ]]};]]
+        protected:
+        public:
+        ]]};
+
+        struct NestedStruct {[[
+        public:
+        private:[[
+          class Inner {[[
+          public:[[
+            Inner();
+            Inner(int X);
+  ]]
+          private:[[
+            int Field;]]
+          protected:[[
+            bool ProtectThis;]]
+          public:[[
+            int getField() {[[ return Field; ]]}]]
+          ]]};]]
+        protected:
+        public:
+        ]]};
+      )cpp",
+      // Member initializer lists.
+      R"cpp(
+        class Foo {[[
+        public:[[
+          Foo([[int Integer, char Char, bool Boolean]]) :[[ I(Integer), C(Char), B(Boolean) ]] {[[
+            int X;
+          ]]}]]
+        private:[[
+          int I;
+          char C;
+          bool B;]]
+        ]]};
+      )cpp",
+  };
+  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;
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang