[clangd] Reapply b60896f Fall back to selecting token-before-cursor if token-after-cursor fails.

This reverts commit b4b9706.
Now avoiding expected<vector<selection>> in favor of expected<vector<unique_ptr<selection>>>

Author: sam-mccall

clang-tools-extra/clangd/ClangdServer.cpp

@@ -395,15 +395,26 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
   WorkScheduler.runWithAST("Rename", File, std::move(Action));
 }
 
-static llvm::Expected<Tweak::Selection>
+// May generate several candidate selections, due to SelectionTree ambiguity.
+// vector of pointers because GCC doesn't like non-copyable Selection.
+static llvm::Expected<std::vector<std::unique_ptr<Tweak::Selection>>>
 tweakSelection(const Range &Sel, const InputsAndAST &AST) {
   auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
   if (!Begin)
     return Begin.takeError();
   auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
   if (!End)
     return End.takeError();
-  return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
+  std::vector<std::unique_ptr<Tweak::Selection>> Result;
+  SelectionTree::createEach(
+      AST.AST.getASTContext(), AST.AST.getTokens(), *Begin, *End,
+      [&](SelectionTree T) {
+        Result.push_back(std::make_unique<Tweak::Selection>(
+            AST.Inputs.Index, AST.AST, *Begin, *End, std::move(T)));
+        return false;
+      });
+  assert(!Result.empty() && "Expected at least one SelectionTree");
+  return Result;
 }
 
 void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
@@ -412,12 +423,21 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
                  this](Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
-    auto Selection = tweakSelection(Sel, *InpAST);
-    if (!Selection)
-      return CB(Selection.takeError());
+    auto Selections = tweakSelection(Sel, *InpAST);
+    if (!Selections)
+      return CB(Selections.takeError());
     std::vector<TweakRef> Res;
-    for (auto &T : prepareTweaks(*Selection, TweakFilter))
-      Res.push_back({T->id(), T->title(), T->intent()});
+    // Don't allow a tweak to fire more than once across ambiguous selections.
+    llvm::DenseSet<llvm::StringRef> PreparedTweaks;
+    auto Filter = [&](const Tweak &T) {
+      return TweakFilter(T) && !PreparedTweaks.count(T.id());
+    };
+    for (const auto &Sel : *Selections) {
+      for (auto &T : prepareTweaks(*Sel, Filter)) {
+        Res.push_back({T->id(), T->title(), T->intent()});
+        PreparedTweaks.insert(T->id());
+      }
+    }
 
     CB(std::move(Res));
   };
@@ -432,21 +452,30 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
        FS = FSProvider.getFileSystem()](Expected<InputsAndAST> InpAST) mutable {
         if (!InpAST)
           return CB(InpAST.takeError());
-        auto Selection = tweakSelection(Sel, *InpAST);
-        if (!Selection)
-          return CB(Selection.takeError());
-        auto A = prepareTweak(TweakID, *Selection);
-        if (!A)
-          return CB(A.takeError());
-        auto Effect = (*A)->apply(*Selection);
-        if (!Effect)
-          return CB(Effect.takeError());
-        for (auto &It : Effect->ApplyEdits) {
-          Edit &E = It.second;
-          format::FormatStyle Style =
-              getFormatStyleForFile(File, E.InitialCode, FS.get());
-          if (llvm::Error Err = reformatEdit(E, Style))
-            elog("Failed to format {0}: {1}", It.first(), std::move(Err));
+        auto Selections = tweakSelection(Sel, *InpAST);
+        if (!Selections)
+          return CB(Selections.takeError());
+        llvm::Optional<llvm::Expected<Tweak::Effect>> Effect;
+        // Try each selection, take the first one that prepare()s.
+        // If they all fail, Effect will hold get the last error.
+        for (const auto &Selection : *Selections) {
+          auto T = prepareTweak(TweakID, *Selection);
+          if (T) {
+            Effect = (*T)->apply(*Selection);
+            break;
+          }
+          Effect = T.takeError();
+        }
+        assert(Effect.hasValue() && "Expected at least one selection");
+        if (*Effect) {
+          // Tweaks don't apply clang-format, do that centrally here.
+          for (auto &It : (*Effect)->ApplyEdits) {
+            Edit &E = It.second;
+            format::FormatStyle Style =
+                getFormatStyleForFile(File, E.InitialCode, FS.get());
+            if (llvm::Error Err = reformatEdit(E, Style))
+              elog("Failed to format {0}: {1}", It.first(), std::move(Err));
+          }
         }
         return CB(std::move(*Effect));
       };

clang-tools-extra/clangd/Hover.cpp

@@ -537,9 +537,12 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
       llvm::consumeError(Offset.takeError());
       return llvm::None;
     }
-    SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
+    // Editors send the position on the left of the hovered character.
+    // So our selection tree should be biased right. (Tested with VSCode).
+    SelectionTree ST = SelectionTree::createRight(
+        AST.getASTContext(), AST.getTokens(), *Offset, *Offset);
     std::vector<const Decl *> Result;
-    if (const SelectionTree::Node *N = Selection.commonAncestor()) {
+    if (const SelectionTree::Node *N = ST.commonAncestor()) {
       auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
       if (!Decls.empty()) {
         HI = getHoverContents(Decls.front(), Index);

clang-tools-extra/clangd/Selection.cpp

@@ -142,6 +142,11 @@ void update(SelectionTree::Selection &Result, SelectionTree::Selection New) {
     Result = SelectionTree::Partial;
 }
 
+// As well as comments, don't count semicolons as real tokens.
+// They're not properly claimed as expr-statement is missing from the AST.
+bool shouldIgnore(const syntax::Token &Tok) {
+  return Tok.kind() == tok::comment || Tok.kind() == tok::semi;
+}
 
 // SelectionTester can determine whether a range of tokens from the PP-expanded
 // stream (corresponding to an AST node) is considered selected.
@@ -172,9 +177,7 @@ class SelectionTester {
         });
     // Precompute selectedness and offset for selected spelled tokens.
     for (const syntax::Token *T = SelFirst; T < SelLimit; ++T) {
-      // As well as comments, don't count semicolons as real tokens.
-      // They're not properly claimed as expr-statement is missing from the AST.
-      if (T->kind() == tok::comment || T->kind() == tok::semi)
+      if (shouldIgnore(*T))
         continue;
       SpelledTokens.emplace_back();
       Tok &S = SpelledTokens.back();
@@ -671,24 +674,49 @@ std::string SelectionTree::Node::kind() const {
   return std::move(OS.str());
 }
 
-// Decide which selection emulates a "point" query in between characters.
-static std::pair<unsigned, unsigned> pointBounds(unsigned Offset, FileID FID,
-                                                 ASTContext &AST) {
-  StringRef Buf = AST.getSourceManager().getBufferData(FID);
-  // Edge-cases where the choice is forced.
-  if (Buf.size() == 0)
-    return {0, 0};
-  if (Offset == 0)
-    return {0, 1};
-  if (Offset == Buf.size())
-    return {Offset - 1, Offset};
-  // We could choose either this byte or the previous. Usually we prefer the
-  // character on the right of the cursor (or under a block cursor).
-  // But if that's whitespace/semicolon, we likely want the token on the left.
-  auto IsIgnoredChar = [](char C) { return isWhitespace(C) || C == ';'; };
-  if (IsIgnoredChar(Buf[Offset]) && !IsIgnoredChar(Buf[Offset - 1]))
-    return {Offset - 1, Offset};
-  return {Offset, Offset + 1};
+// Decide which selections emulate a "point" query in between characters.
+// If it's ambiguous (the neighboring characters are selectable tokens), returns
+// both possibilities in preference order.
+// Always returns at least one range - if no tokens touched, and empty range.
+static llvm::SmallVector<std::pair<unsigned, unsigned>, 2>
+pointBounds(unsigned Offset, const syntax::TokenBuffer &Tokens) {
+  const auto &SM = Tokens.sourceManager();
+  SourceLocation Loc = SM.getComposedLoc(SM.getMainFileID(), Offset);
+  llvm::SmallVector<std::pair<unsigned, unsigned>, 2> Result;
+  // Prefer right token over left.
+  for (const syntax::Token &Tok :
+       llvm::reverse(spelledTokensTouching(Loc, Tokens))) {
+    if (shouldIgnore(Tok))
+      continue;
+    unsigned Offset = Tokens.sourceManager().getFileOffset(Tok.location());
+    Result.emplace_back(Offset, Offset + Tok.length());
+  }
+  if (Result.empty())
+    Result.emplace_back(Offset, Offset);
+  return Result;
+}
+
+bool SelectionTree::createEach(ASTContext &AST,
+                               const syntax::TokenBuffer &Tokens,
+                               unsigned Begin, unsigned End,
+                               llvm::function_ref<bool(SelectionTree)> Func) {
+  if (Begin != End)
+    return Func(SelectionTree(AST, Tokens, Begin, End));
+  for (std::pair<unsigned, unsigned> Bounds : pointBounds(Begin, Tokens))
+    if (Func(SelectionTree(AST, Tokens, Bounds.first, Bounds.second)))
+      return true;
+  return false;
+}
+
+SelectionTree SelectionTree::createRight(ASTContext &AST,
+                                         const syntax::TokenBuffer &Tokens,
+                                         unsigned int Begin, unsigned int End) {
+  llvm::Optional<SelectionTree> Result;
+  createEach(AST, Tokens, Begin, End, [&](SelectionTree T) {
+    Result = std::move(T);
+    return true;
+  });
+  return std::move(*Result);
 }
 
 SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
@@ -698,8 +726,6 @@ SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
   // but that's all clangd has needed so far.
   const SourceManager &SM = AST.getSourceManager();
   FileID FID = SM.getMainFileID();
-  if (Begin == End)
-    std::tie(Begin, End) = pointBounds(Begin, FID, AST);
   PrintPolicy.TerseOutput = true;
   PrintPolicy.IncludeNewlines = false;
 
@@ -711,10 +737,6 @@ SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
   dlog("Built selection tree\n{0}", *this);
 }
 
-SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
-                             unsigned Offset)
-    : SelectionTree(AST, Tokens, Offset, Offset) {}
-
 const Node *SelectionTree::commonAncestor() const {
   const Node *Ancestor = Root;
   while (Ancestor->Children.size() == 1 && !Ancestor->Selected)

clang-tools-extra/clangd/Selection.h

@@ -29,6 +29,14 @@
 //   - we determine which low-level nodes are partly or completely covered
 //     by the selection.
 //   - we expose a tree of the selected nodes and their lexical parents.
+//
+// Sadly LSP specifies locations as being between characters, and this causes
+// some ambiguities we cannot cleanly resolve:
+//   lhs+rhs  // targeting '+' or 'lhs'?
+//      ^     // in GUI editors, double-clicking 'lhs' yields this position!
+//
+// The best we can do in these cases is try both, which leads to the awkward
+// SelectionTree::createEach() API.
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SELECTION_H
@@ -64,16 +72,32 @@ namespace clangd {
 // point back into the AST it was constructed with.
 class SelectionTree {
 public:
-  // Creates a selection tree at the given byte offset in the main file.
-  // This is approximately equivalent to a range of one character.
-  // (Usually, the character to the right of Offset, sometimes to the left).
-  SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
-                unsigned Offset);
-  // Creates a selection tree for the given range in the main file.
-  // The range includes bytes [Start, End).
-  // If Start == End, uses the same heuristics as SelectionTree(AST, Start).
-  SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
-                unsigned Start, unsigned End);
+  // Create selection trees for the given range, and pass them to Func.
+  //
+  // There may be multiple possible selection trees:
+  // - if the range is empty and borders two tokens, a tree for the right token
+  //   and a tree for the left token will be yielded.
+  // - Func should return true on success (stop) and false on failure (continue)
+  //
+  // Always yields at least one tree. If no tokens are touched, it is empty.
+  static bool createEach(ASTContext &AST, const syntax::TokenBuffer &Tokens,
+                         unsigned Begin, unsigned End,
+                         llvm::function_ref<bool(SelectionTree)> Func);
+
+  // Create a selection tree for the given range.
+  //
+  // Where ambiguous (range is empty and borders two tokens), prefer the token
+  // on the right.
+  static SelectionTree createRight(ASTContext &AST,
+                                   const syntax::TokenBuffer &Tokens,
+                                   unsigned Begin, unsigned End);
+
+  // Copies are no good - contain pointers to other nodes.
+  SelectionTree(const SelectionTree &) = delete;
+  SelectionTree &operator=(const SelectionTree &) = delete;
+  // Moves are OK though - internal storage is pointer-stable when moved.
+  SelectionTree(SelectionTree &&) = default;
+  SelectionTree &operator=(SelectionTree &&) = default;
 
   // Describes to what extent an AST node is covered by the selection.
   enum Selection : unsigned char {
@@ -121,6 +145,11 @@ class SelectionTree {
   const Node &root() const { return *Root; }
 
 private:
+  // Creates a selection tree for the given range in the main file.
+  // The range includes bytes [Start, End).
+  SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
+                unsigned Start, unsigned End);
+
   std::deque<Node> Nodes; // Stable-pointer storage.
   const Node *Root;
   clang::PrintingPolicy PrintPolicy;

clang-tools-extra/clangd/SemanticSelection.cpp

@@ -39,7 +39,8 @@ llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
   }
 
   // Get node under the cursor.
-  SelectionTree ST(AST.getASTContext(), AST.getTokens(), *Offset);
+  SelectionTree ST = SelectionTree::createRight(
+      AST.getASTContext(), AST.getTokens(), *Offset, *Offset);
   for (const auto *Node = ST.commonAncestor(); Node != nullptr;
        Node = Node->Parent) {
     if (const Decl *D = Node->ASTNode.get<Decl>()) {