[clangd] Add BlockEnd comments for control flow statements

[sammccall]

Summary

These mark the end of CompoundStmts bodies of if/while/for/switch.
To identify which statement is being ended, we include abbreviated
text of the condition/loop variable.

Looks like this:

[hokein]

Thanks, this looks like in a good shape. I left comments with some thoughts and nits, but they're not blockers, feel free to land it.

clang-tools-extra/clangd/InlayHints.cpp

252

This looks like a complicated implementation to get an abbreviated-form of cond expression for the inlay label text.

A different idea would be if the user can just click the inlay label text, then the editor jumps to the corresponding for statement (looks like it is supported with the InlayHintLabelPart::Location) with that we probably don't need to emit an abbreviated-form text (// for should be clear enough).

583

my opinion for this case would be (the marking result is also consistent with the brackets)

if (cond1) {
} // mark as cond1
else if (cond2) {
} // mark as cond2.

604

it looks like we will mark the block end for single-statement CompoundStmt without {}, it doesn't seem to add much value to this case (the body is short enough to spot the for statement).

for (int i = 0; i < 10; ++i)
  foo();

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

1786

nit: add a case if (int i = 0; i > 10) { ... }.

1859

nit: I guess showing foo... is better.

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

252

A different idea would be if the user can just click the inlay label text, then the editor jumps to the corresponding for statement (looks like it is supported with the InlayHintLabelPart::Location) with that we probably don't need to emit an abbreviated-form text (// for should be clear enough).

Agree we should have this too (also for other kinds of hints).

But I don't think it's a replacement:

• BlockEnd is most useful to connect blockend + blockstart context when the block start is far away, and navigating to it destroys your blockend context. (Hover is better in this regard but still isn't available *in-line*).
• inlayhintlabelpart isn't available in most editors that support inlayhint, and realistically may never be

583

If the first if is } is on a separate line we could do this.

The styles we work the most place } else on one line though, so this isn't an option.
In those styles we see if/elseif/else as a single construct (and format it as such) - and marking as cond2 seems strange to me...

604

As discussed offline, CompoundStmt exists exactly where braces exist.

clang-tools-extra/clangd/InlayHints.cpp

 //===--- InlayHints.cpp ------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "InlayHints.h"
 #include "AST.h"
 #include "Config.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/raw_ostream.h"
 #include <optional>
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // For now, inlay hints are always anchored at the left or right of their range.
 enum class HintSide { Left, Right };
 
getDesignators(const InitListExpr *Syn) {
   // We use their SourceLocation to correlate with the syntactic form later.
   llvm::DenseMap<SourceLocation, std::string> Designators;
   std::string EmptyPrefix;
   collectDesignators(Syn->isSemanticForm() ? Syn : Syn->getSemanticForm(),
                      Designators, NestedBraces, EmptyPrefix);
   return Designators;
 }
 
+void stripLeadingUnderscores(StringRef &Name) { Name = Name.ltrim('_'); }
+
+// getDeclForType() returns the decl responsible for Type's spelling.
+// This is the inverse of ASTContext::getTypeDeclType().
+template <typename Ty, typename = decltype(((Ty *)nullptr)->getDecl())>
+const NamedDecl *getDeclForTypeImpl(const Ty *T) {
+  return T->getDecl();
+}
+const NamedDecl *getDeclForTypeImpl(const void *T) { return nullptr; }
+const NamedDecl *getDeclForType(const Type *T) {
+  switch (T->getTypeClass()) {
+#define ABSTRACT_TYPE(TY, BASE)
+#define TYPE(TY, BASE)                                                         \
+  case Type::TY:                                                               \
+    return getDeclForTypeImpl(llvm::cast<TY##Type>(T));
+#include "clang/AST/TypeNodes.inc"
+  }
+}
+
+// getSimpleName() returns the plain identifier for an entity, if any.
+llvm::StringRef getSimpleName(const DeclarationName &DN) {
+  if (IdentifierInfo *Ident = DN.getAsIdentifierInfo())
+    return Ident->getName();
+  return "";
+}
+llvm::StringRef getSimpleName(const NamedDecl &D) {
+  return getSimpleName(D.getDeclName());
+}
+llvm::StringRef getSimpleName(QualType T) {
+  if (const auto *ET = llvm::dyn_cast<ElaboratedType>(T))
+    return getSimpleName(ET->getNamedType());
+  if (const auto *BT = llvm::dyn_cast<BuiltinType>(T)) {
+    PrintingPolicy PP(LangOptions{});
+    PP.adjustForCPlusPlus();
+    return BT->getName(PP);
+  }
+  if (const auto *D = getDeclForType(T.getTypePtr()))
+    return getSimpleName(D->getDeclName());
+  return "";
+}
+
+// Returns a very abbreviated form of an expression, or "" if it's too complex.
+// For example: `foo->bar()` would produce "bar".
+// This is used to summarize e.g. the condition of a while loop.
+std::string summarizeExpr(const Expr *E) {
+  struct Namer : ConstStmtVisitor<Namer, std::string> {
+    std::string Visit(const Expr *E) {
+      if (E == nullptr)
+        return "";
+      return ConstStmtVisitor::Visit(E->IgnoreImplicit());
+    }
+
+    // Any sort of decl reference, we just use the unqualified name.
+    std::string VisitMemberExpr(const MemberExpr *E) {
+      return getSimpleName(*E->getMemberDecl()).str();
+    }
+    std::string VisitDeclRefExpr(const DeclRefExpr *E) {
+      return getSimpleName(*E->getFoundDecl()).str();
+    }
+    std::string VisitCallExpr(const CallExpr *E) {
+      return Visit(E->getCallee());
+    }
+    std::string
+    VisitCXXDependentScopeMemberExpr(const CXXDependentScopeMemberExpr *E) {
+      return getSimpleName(E->getMember()).str();
+    }
+    std::string
+    VisitDependentScopeMemberExpr(const DependentScopeDeclRefExpr *E) {
+      return getSimpleName(E->getDeclName()).str();
+    }
+    std::string VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *E) {
+      return getSimpleName(E->getType()).str();
+    }
+    std::string VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *E) {
+      return getSimpleName(E->getType()).str();
+    }
+
+    // Step through implicit nodes that clang doesn't classify as such.
+    std::string VisitCXXMemberCallExpr(const CXXMemberCallExpr *E) {
+      // Call to operator bool() inside if (X): dispatch to X.
+      if (E->getNumArgs() == 0 &&
+          E->getMethodDecl()->getDeclName().getNameKind() ==
+              DeclarationName::CXXConversionFunctionName &&
+          E->getSourceRange() ==
+              E->getImplicitObjectArgument()->getSourceRange())
+        return Visit(E->getImplicitObjectArgument());
+      return ConstStmtVisitor::VisitCXXMemberCallExpr(E);
+    }
+    std::string VisitCXXConstructExpr(const CXXConstructExpr *E) {
+      if (E->getNumArgs() == 1)
+        return Visit(E->getArg(0));
+      return "";
+    }
+
+    // Literals are just printed
+    std::string VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E) {
+      return E->getValue() ? "true" : "false";
+    }
+    std::string VisitIntegerLiteral(const IntegerLiteral *E) {
+      return llvm::to_string(E->getValue());
+    }
+    std::string VisitFloatingLiteral(const FloatingLiteral *E) {
+      std::string Result;
+      llvm::raw_string_ostream OS(Result);
+      E->getValue().print(OS);
+      // Printer adds newlines?!
+      Result.resize(llvm::StringRef(Result).rtrim().size());
+      return Result;
+    }
+    std::string VisitStringLiteral(const StringLiteral *E) {
+      std::string Result = "\"";
+      if (E->containsNonAscii()) {
+        Result += "...";
+      } else if (E->getLength() > 10) {
+        Result += E->getString().take_front(7);
+        Result += "...";
+      } else {
+        llvm::raw_string_ostream OS(Result);
+        llvm::printEscapedString(E->getString(), OS);
+      }
+      Result.push_back('"');
+      return Result;
+    }
+
+    // Simple operators. Motivating cases are `!x` and `I < Length`.
+    std::string printUnary(llvm::StringRef Spelling, const Expr *Operand,
+                           bool Prefix) {
+      std::string Sub = Visit(Operand);
+      if (Sub.empty())
+        return "";
+      if (Prefix)
+        return (Spelling + Sub).str();
+      Sub += Spelling;
+      return Sub;
+    }
+    bool InsideBinary = false; // No recursing into binary expressions.
+    std::string printBinary(llvm::StringRef Spelling, const Expr *LHSOp,
+                            const Expr *RHSOp) {
+      if (InsideBinary)
+        return "";
+      llvm::SaveAndRestore InBinary(InsideBinary, true);
+
+      std::string LHS = Visit(LHSOp);
+      std::string RHS = Visit(RHSOp);
+      if (LHS.empty() && RHS.empty())
+        return "";
+
+      if (LHS.empty())
+        LHS = "...";
+      LHS.push_back(' ');
+      LHS += Spelling;
+      LHS.push_back(' ');
+      if (RHS.empty())
+        LHS += "...";
+      else
+        LHS += RHS;
+      return LHS;
+    }
+    std::string VisitUnaryOperator(const UnaryOperator *E) {
+      return printUnary(E->getOpcodeStr(E->getOpcode()), E->getSubExpr(),
+                        !E->isPostfix());
+    }
+    std::string VisitBinaryOperator(const BinaryOperator *E) {
+      return printBinary(E->getOpcodeStr(E->getOpcode()), E->getLHS(),
+                         E->getRHS());
+    }
+    std::string VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *E) {
+      const char *Spelling = getOperatorSpelling(E->getOperator());
+      // Handle weird unary-that-look-like-binary postfix operators.
+      if ((E->getOperator() == OO_PlusPlus ||
+           E->getOperator() == OO_MinusMinus) &&
+          E->getNumArgs() == 2)
+        return printUnary(Spelling, E->getArg(0), false);
+      if (E->isInfixBinaryOp())
+        return printBinary(Spelling, E->getArg(0), E->getArg(1));
+      if (E->getNumArgs() == 1) {
+        switch (E->getOperator()) {
+        case OO_Plus:
+        case OO_Minus:
+        case OO_Star:
+        case OO_Amp:
+        case OO_Tilde:
+        case OO_Exclaim:
+        case OO_PlusPlus:
+        case OO_MinusMinus:
+          return printUnary(Spelling, E->getArg(0), true);
+        default:
+          break;
+        }
+      }
+      return "";
+    }
+  };
+  return Namer{}.Visit(E);
+}
+
 // Determines if any intermediate type in desugaring QualType QT is of
 // substituted template parameter type. Ignore pointer or reference wrappers.
 bool isSugaredTemplateParameter(QualType QT) {
   static auto PeelWrappers = [](QualType QT) {
     // Neither `PointerType` nor `ReferenceType` is considered as sugared
     // type. Peel it.
     QualType Next;
     while (!(Next = QT->getPointeeType()).isNull())
if (Cfg.InlayHints.BlockEnd && D->isThisDeclarationADefinition()) {
       // We use `printName` here to properly print name of ctor/dtor/operator
       // overload.
       if (const Stmt *Body = D->getBody())
         addBlockEndHint(Body->getSourceRange(), "", printName(AST, *D), "");
     }
     return true;
   }
 
+  bool VisitForStmt(ForStmt *S) {
+    if (Cfg.InlayHints.BlockEnd) {
+      std::string Name;
+      // Common case: for (int I = 0; I < N; I++). Use "I" as the name.
+      if (auto *DS = llvm::dyn_cast_or_null<DeclStmt>(S->getInit());
+          DS && DS->isSingleDecl())
+        Name = getSimpleName(llvm::cast<NamedDecl>(*DS->getSingleDecl()));
+      else
+        Name = summarizeExpr(S->getCond());
+      markBlockEnd(S->getBody(), "for", Name);
+    }
+    return true;
+  }
+
+  bool VisitCXXForRangeStmt(CXXForRangeStmt *S) {
+    if (Cfg.InlayHints.BlockEnd)
+      markBlockEnd(S->getBody(), "for", getSimpleName(*S->getLoopVariable()));
+    return true;
+  }
+
+  bool VisitWhileStmt(WhileStmt *S) {
+    if (Cfg.InlayHints.BlockEnd)
+      markBlockEnd(S->getBody(), "while", summarizeExpr(S->getCond()));
+    return true;
+  }
+
+  bool VisitSwitchStmt(SwitchStmt *S) {
+    if (Cfg.InlayHints.BlockEnd)
+      markBlockEnd(S->getBody(), "switch", summarizeExpr(S->getCond()));
+    return true;
+  }
+
+  // If/else chains are tricky.
+  //   if (cond1) {
+  //   } else if (cond2) {
+  //   } // mark as "cond1" or "cond2"?
+  // For now, the answer is neither, just mark as "if".
+  // The ElseIf is a different IfStmt that doesn't know about the outer one.
+  llvm::DenseSet<const IfStmt *> ElseIfs; // not eligible for names
+  bool VisitIfStmt(IfStmt *S) {
+    if (Cfg.InlayHints.BlockEnd) {
+      if (const auto *ElseIf = llvm::dyn_cast_or_null<IfStmt>(S->getElse()))
+        ElseIfs.insert(ElseIf);
+      // Don't use markBlockEnd: the relevant range is [then.begin, else.end].
+      if (const auto *EndCS = llvm::dyn_cast<CompoundStmt>(
+              S->getElse() ? S->getElse() : S->getThen())) {
+        addBlockEndHint(
+            {S->getThen()->getBeginLoc(), EndCS->getRBracLoc()}, "if",
+            ElseIfs.contains(S) ? "" : summarizeExpr(S->getCond()), "");
+      }
+    }
+    return true;
+  }
+
+  void markBlockEnd(const Stmt *Body, llvm::StringRef Label,
+                    llvm::StringRef Name = "") {
+    if (const auto *CS = llvm::dyn_cast_or_null<CompoundStmt>(Body))
+      addBlockEndHint(CS->getSourceRange(), Label, Name, "");
+  }
+
   bool VisitTagDecl(TagDecl *D) {
     if (Cfg.InlayHints.BlockEnd && D->isThisDeclarationADefinition()) {
       std::string DeclPrefix = D->getKindName().str();
       if (const auto *ED = dyn_cast<EnumDecl>(D)) {
         if (ED->isScoped())
           DeclPrefix += ED->isScopedUsingClassTag() ? " class" : " struct";
       };
       addBlockEndHint(D->getBraceRange(), DeclPrefix, getSimpleName(*D), ";");
if (auto *Callee = dyn_cast<FunctionDecl>(P->getDeclContext())) {
         if (I < Callee->getNumParams()) {
           return Def->getParamDecl(I);
         }
       }
     }
     return nullptr;
   }
 
-  static void stripLeadingUnderscores(StringRef &Name) {
-    Name = Name.ltrim('_');
-  }
-
-  static StringRef getSimpleName(const NamedDecl &D) {
-    if (IdentifierInfo *Ident = D.getDeclName().getAsIdentifierInfo()) {
-      return Ident->getName();
-    }
-
-    return StringRef();
-  }
-
   // We pass HintSide rather than SourceLocation because we want to ensure
   // it is in the same file as the common file range.
   void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind,
                     llvm::StringRef Prefix, llvm::StringRef Label,
                     llvm::StringRef Suffix) {
     auto LSPRange = getHintRange(R);
     if (!LSPRange)
       return;

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

 #include "TestTU.h"
 #include "TestWorkspace.h"
 #include "XRefs.h"
 #include "support/Context.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <string>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
                               const InlayHint &Hint) {
   return Stream << Hint.label << "@" << Hint.range;
 }
assertBlockEndHints(
     enum class E2 {
     $E2[[};]]
   )cpp",
       ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
       ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
       ExpectedHint{" // enum class E2", "E2"});
 }
 
+TEST(BlockEndHints, If) {
+  assertBlockEndHints(
+      R"cpp(
+    void foo(bool cond) {
+       if (cond)
+          ;
+
+       if (cond) {
+       $simple[[}]]
+
+       if (cond) {
+       } else {
+       $ifelse[[}]]
+
+       if (cond) {
+       } else if (!cond) {
+       $elseif[[}]]
+
+       if (cond) {
+       } else {
+         if (!cond) {
+         $inner[[}]]
+       $outer[[}]]
+
+       if (auto X = cond) {
+       $init[[}]]
+
+       if (int i = 0; i > 10) {
+       $init_cond[[}]]
+    } // suppress
+  )cpp",
+      ExpectedHint{" // if cond", "simple"},
+      ExpectedHint{" // if cond", "ifelse"}, ExpectedHint{" // if", "elseif"},
+      ExpectedHint{" // if !cond", "inner"},
+      ExpectedHint{" // if cond", "outer"}, ExpectedHint{" // if X", "init"},
+      ExpectedHint{" // if i > 10", "init_cond"});
+}
+
+TEST(BlockEndHints, Loops) {
+  assertBlockEndHints(
+      R"cpp(
+    void foo() {
+       while (true)
+          ;
+
+       while (true) {
+       $while[[}]]
+
+       do {
+       } while (true);
+
+       for (;true;) {
+       $forcond[[}]]
+
+       for (int I = 0; I < 10; ++I) {
+       $forvar[[}]]
+
+       int Vs[] = {1,2,3};
+       for (auto V : Vs) {
+       $foreach[[}]]
+    } // suppress
+  )cpp",
+      ExpectedHint{" // while true", "while"},
+      ExpectedHint{" // for true", "forcond"},
+      ExpectedHint{" // for I", "forvar"},
+      ExpectedHint{" // for V", "foreach"});
+}
+
+TEST(BlockEndHints, Switch) {
+  assertBlockEndHints(
+      R"cpp(
+    void foo(int I) {
+      switch (I) {
+        case 0: break;
+      $switch[[}]]
+    } // suppress
+  )cpp",
+      ExpectedHint{" // switch I", "switch"});
+}
+
+TEST(BlockEndHints, PrintLiterals) {
+  assertBlockEndHints(
+      R"cpp(
+    void foo() {
+      while ("foo") {
+      $string[[}]]
+
+      while ("foo but this time it is very long") {
+      $string_long[[}]]
+
+      while (true) {
+      $boolean[[}]]
+
+      while (1) {
+      $integer[[}]]
+
+      while (1.5) {
+      $float[[}]]
+    } // suppress
+  )cpp",
+      ExpectedHint{" // while \"foo\"", "string"},
+      ExpectedHint{" // while \"foo but...\"", "string_long"},
+      ExpectedHint{" // while true", "boolean"},
+      ExpectedHint{" // while 1", "integer"},
+      ExpectedHint{" // while 1.5", "float"});
+}
+
+TEST(BlockEndHints, PrintRefs) {
+  assertBlockEndHints(
+      R"cpp(
+    namespace ns {
+      int Var;
+      int func();
+      struct S {
+        int Field;
+        int method() const;
+      }; // suppress
+    } // suppress
+    void foo() {
+      while (ns::Var) {
+      $var[[}]]
+
+      while (ns::func()) {
+      $func[[}]]
+
+      while (ns::S{}.Field) {
+      $field[[}]]
+
+      while (ns::S{}.method()) {
+      $method[[}]]
+    } // suppress
+  )cpp",
+      ExpectedHint{" // while Var", "var"},
+      ExpectedHint{" // while func", "func"},
+      ExpectedHint{" // while Field", "field"},
+      ExpectedHint{" // while method", "method"});
+}
+
+TEST(BlockEndHints, PrintConversions) {
+  assertBlockEndHints(
+      R"cpp(
+    struct S {
+      S(int);
+      S(int, int);
+      explicit operator bool();
+    }; // suppress
+    void foo(int I) {
+      while (float(I)) {
+      $convert_primitive[[}]]
+
+      while (S(I)) {
+      $convert_class[[}]]
+
+      while (S(I, I)) {
+      $construct_class[[}]]
+    } // suppress
+  )cpp",
+      ExpectedHint{" // while float", "convert_primitive"},
+      ExpectedHint{" // while S", "convert_class"},
+      ExpectedHint{" // while S", "construct_class"});
+}
+
+TEST(BlockEndHints, PrintOperators) {
+  std::string AnnotatedCode = R"cpp(
+    void foo(Integer I) {
+      while(++I){
+      $preinc[[}]]
+
+      while(I++){
+      $postinc[[}]]
+
+      while(+(I + I)){
+      $unary_complex[[}]]
+
+      while(I < 0){
+      $compare[[}]]
+
+      while((I + I) < I){
+      $lhs_complex[[}]]
+
+      while(I < (I + I)){
+      $rhs_complex[[}]]
+
+      while((I + I) < (I + I)){
+      $binary_complex[[}]]
+    } // suppress
+  )cpp";
+
+  // We can't store shared expectations in a vector, assertHints uses varargs.
+  auto AssertExpectedHints = [&](llvm::StringRef Code) {
+    assertBlockEndHints(Code, ExpectedHint{" // while ++I", "preinc"},
+                        ExpectedHint{" // while I++", "postinc"},
+                        ExpectedHint{" // while", "unary_complex"},
+                        ExpectedHint{" // while I < 0", "compare"},
+                        ExpectedHint{" // while ... < I", "lhs_complex"},
+                        ExpectedHint{" // while I < ...", "rhs_complex"},
+                        ExpectedHint{" // while", "binary_complex"});
+  };
+
+  // First with built-in operators.
+  AssertExpectedHints("using Integer = int;" + AnnotatedCode);
+  // And now with overloading!
+  AssertExpectedHints(R"cpp(
+    struct Integer {
+      explicit operator bool();
+      Integer operator++();
+      Integer operator++(int);
+      Integer operator+(Integer);
+      Integer operator+();
+      bool operator<(Integer);
+      bool operator<(int);
+    }; // suppress
+  )cpp" + AnnotatedCode);
+}
+
 TEST(BlockEndHints, TrailingSemicolon) {
   assertBlockEndHints(R"cpp(
     // The hint is placed after the trailing ';'
     struct S1 {
     $S1[[}  ;]]   
 
     // The hint is always placed in the same line with the closing '}'.
     // So in this case where ';' is missing, it is attached to '}'.