Skip to content

Commit a0ff8cd

Browse files
sam-mccallsam-mccall
committed
[clangd] Reapply b60896f Fall back to selecting token-before-cursor if token-after-cursor fails.
This reverts commit 8f876d5.
1 parent 8f876d5 commit a0ff8cd

14 files changed

+265
-105
lines changed

‎clang-tools-extra/clangd/ClangdServer.cpp

+49-22
Original file line numberDiff line numberDiff line change
@@ -374,15 +374,24 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
374374
WorkScheduler.runWithAST("Rename", File, std::move(Action));
375375
}
376376

377-
static llvm::Expected<Tweak::Selection>
377+
// May generate several candidate selections, due to SelectionTree ambiguity.
378+
static llvm::Expected<std::vector<Tweak::Selection>>
378379
tweakSelection(const Range &Sel, const InputsAndAST &AST) {
379380
auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
380381
if (!Begin)
381382
return Begin.takeError();
382383
auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
383384
if (!End)
384385
return End.takeError();
385-
return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
386+
std::vector<Tweak::Selection> Result;
387+
SelectionTree::createEach(AST.AST.getASTContext(), AST.AST.getTokens(),
388+
*Begin, *End, [&](SelectionTree T) {
389+
Result.emplace_back(AST.Inputs.Index, AST.AST,
390+
*Begin, *End, std::move(T));
391+
return false;
392+
});
393+
assert(!Result.empty() && "Expected at least one SelectionTree");
394+
return Result;
386395
}
387396

388397
void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
@@ -391,12 +400,21 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
391400
this](Expected<InputsAndAST> InpAST) mutable {
392401
if (!InpAST)
393402
return CB(InpAST.takeError());
394-
auto Selection = tweakSelection(Sel, *InpAST);
395-
if (!Selection)
396-
return CB(Selection.takeError());
403+
auto Selections = tweakSelection(Sel, *InpAST);
404+
if (!Selections)
405+
return CB(Selections.takeError());
397406
std::vector<TweakRef> Res;
398-
for (auto &T : prepareTweaks(*Selection, TweakFilter))
399-
Res.push_back({T->id(), T->title(), T->intent()});
407+
// Don't allow a tweak to fire more than once across ambiguous selections.
408+
llvm::DenseSet<llvm::StringRef> PreparedTweaks;
409+
auto Filter = [&](const Tweak &T) {
410+
return TweakFilter(T) && !PreparedTweaks.count(T.id());
411+
};
412+
for (const auto &Sel : *Selections) {
413+
for (auto &T : prepareTweaks(Sel, Filter)) {
414+
Res.push_back({T->id(), T->title(), T->intent()});
415+
PreparedTweaks.insert(T->id());
416+
}
417+
}
400418

401419
CB(std::move(Res));
402420
};
@@ -411,21 +429,30 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
411429
FS = FSProvider.getFileSystem()](Expected<InputsAndAST> InpAST) mutable {
412430
if (!InpAST)
413431
return CB(InpAST.takeError());
414-
auto Selection = tweakSelection(Sel, *InpAST);
415-
if (!Selection)
416-
return CB(Selection.takeError());
417-
auto A = prepareTweak(TweakID, *Selection);
418-
if (!A)
419-
return CB(A.takeError());
420-
auto Effect = (*A)->apply(*Selection);
421-
if (!Effect)
422-
return CB(Effect.takeError());
423-
for (auto &It : Effect->ApplyEdits) {
424-
Edit &E = It.second;
425-
format::FormatStyle Style =
426-
getFormatStyleForFile(File, E.InitialCode, FS.get());
427-
if (llvm::Error Err = reformatEdit(E, Style))
428-
elog("Failed to format {0}: {1}", It.first(), std::move(Err));
432+
auto Selections = tweakSelection(Sel, *InpAST);
433+
if (!Selections)
434+
return CB(Selections.takeError());
435+
llvm::Optional<llvm::Expected<Tweak::Effect>> Effect;
436+
// Try each selection, take the first one that prepare()s.
437+
// If they all fail, Effect will hold get the last error.
438+
for (const auto &Selection : *Selections) {
439+
auto T = prepareTweak(TweakID, Selection);
440+
if (T) {
441+
Effect = (*T)->apply(Selection);
442+
break;
443+
}
444+
Effect = T.takeError();
445+
}
446+
assert(Effect.hasValue() && "Expected at least one selection");
447+
if (*Effect) {
448+
// Tweaks don't apply clang-format, do that centrally here.
449+
for (auto &It : (*Effect)->ApplyEdits) {
450+
Edit &E = It.second;
451+
format::FormatStyle Style =
452+
getFormatStyleForFile(File, E.InitialCode, FS.get());
453+
if (llvm::Error Err = reformatEdit(E, Style))
454+
elog("Failed to format {0}: {1}", It.first(), std::move(Err));
455+
}
429456
}
430457
return CB(std::move(*Effect));
431458
};

‎clang-tools-extra/clangd/Hover.cpp

+5-2
Original file line numberDiff line numberDiff line change
@@ -410,9 +410,12 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
410410
llvm::consumeError(Offset.takeError());
411411
return llvm::None;
412412
}
413-
SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
413+
// Editors send the position on the left of the hovered character.
414+
// So our selection tree should be biased right. (Tested with VSCode).
415+
SelectionTree ST = SelectionTree::createRight(
416+
AST.getASTContext(), AST.getTokens(), *Offset, *Offset);
414417
std::vector<const Decl *> Result;
415-
if (const SelectionTree::Node *N = Selection.commonAncestor()) {
418+
if (const SelectionTree::Node *N = ST.commonAncestor()) {
416419
DeclRelationSet Rel = DeclRelation::TemplatePattern | DeclRelation::Alias;
417420
auto Decls = targetDecl(N->ASTNode, Rel);
418421
if (!Decls.empty()) {

‎clang-tools-extra/clangd/Selection.cpp

+49-27
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,11 @@ void update(SelectionTree::Selection &Result, SelectionTree::Selection New) {
142142
Result = SelectionTree::Partial;
143143
}
144144

145+
// As well as comments, don't count semicolons as real tokens.
146+
// They're not properly claimed as expr-statement is missing from the AST.
147+
bool shouldIgnore(const syntax::Token &Tok) {
148+
return Tok.kind() == tok::comment || Tok.kind() == tok::semi;
149+
}
145150

146151
// SelectionTester can determine whether a range of tokens from the PP-expanded
147152
// stream (corresponding to an AST node) is considered selected.
@@ -172,9 +177,7 @@ class SelectionTester {
172177
});
173178
// Precompute selectedness and offset for selected spelled tokens.
174179
for (const syntax::Token *T = SelFirst; T < SelLimit; ++T) {
175-
// As well as comments, don't count semicolons as real tokens.
176-
// They're not properly claimed as expr-statement is missing from the AST.
177-
if (T->kind() == tok::comment || T->kind() == tok::semi)
180+
if (shouldIgnore(*T))
178181
continue;
179182
SpelledTokens.emplace_back();
180183
Tok &S = SpelledTokens.back();
@@ -650,24 +653,49 @@ std::string SelectionTree::Node::kind() const {
650653
return std::move(OS.str());
651654
}
652655

653-
// Decide which selection emulates a "point" query in between characters.
654-
static std::pair<unsigned, unsigned> pointBounds(unsigned Offset, FileID FID,
655-
ASTContext &AST) {
656-
StringRef Buf = AST.getSourceManager().getBufferData(FID);
657-
// Edge-cases where the choice is forced.
658-
if (Buf.size() == 0)
659-
return {0, 0};
660-
if (Offset == 0)
661-
return {0, 1};
662-
if (Offset == Buf.size())
663-
return {Offset - 1, Offset};
664-
// We could choose either this byte or the previous. Usually we prefer the
665-
// character on the right of the cursor (or under a block cursor).
666-
// But if that's whitespace/semicolon, we likely want the token on the left.
667-
auto IsIgnoredChar = [](char C) { return isWhitespace(C) || C == ';'; };
668-
if (IsIgnoredChar(Buf[Offset]) && !IsIgnoredChar(Buf[Offset - 1]))
669-
return {Offset - 1, Offset};
670-
return {Offset, Offset + 1};
656+
// Decide which selections emulate a "point" query in between characters.
657+
// If it's ambiguous (the neighboring characters are selectable tokens), returns
658+
// both possibilities in preference order.
659+
// Always returns at least one range - if no tokens touched, and empty range.
660+
static llvm::SmallVector<std::pair<unsigned, unsigned>, 2>
661+
pointBounds(unsigned Offset, const syntax::TokenBuffer &Tokens) {
662+
const auto &SM = Tokens.sourceManager();
663+
SourceLocation Loc = SM.getComposedLoc(SM.getMainFileID(), Offset);
664+
llvm::SmallVector<std::pair<unsigned, unsigned>, 2> Result;
665+
// Prefer right token over left.
666+
for (const syntax::Token &Tok :
667+
llvm::reverse(spelledTokensTouching(Loc, Tokens))) {
668+
if (shouldIgnore(Tok))
669+
continue;
670+
unsigned Offset = Tokens.sourceManager().getFileOffset(Tok.location());
671+
Result.emplace_back(Offset, Offset + Tok.length());
672+
}
673+
if (Result.empty())
674+
Result.emplace_back(Offset, Offset);
675+
return Result;
676+
}
677+
678+
bool SelectionTree::createEach(ASTContext &AST,
679+
const syntax::TokenBuffer &Tokens,
680+
unsigned Begin, unsigned End,
681+
llvm::function_ref<bool(SelectionTree)> Func) {
682+
if (Begin != End)
683+
return Func(SelectionTree(AST, Tokens, Begin, End));
684+
for (std::pair<unsigned, unsigned> Bounds : pointBounds(Begin, Tokens))
685+
if (Func(SelectionTree(AST, Tokens, Bounds.first, Bounds.second)))
686+
return true;
687+
return false;
688+
}
689+
690+
SelectionTree SelectionTree::createRight(ASTContext &AST,
691+
const syntax::TokenBuffer &Tokens,
692+
unsigned int Begin, unsigned int End) {
693+
llvm::Optional<SelectionTree> Result;
694+
createEach(AST, Tokens, Begin, End, [&](SelectionTree T) {
695+
Result = std::move(T);
696+
return true;
697+
});
698+
return std::move(*Result);
671699
}
672700

673701
SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
@@ -677,8 +705,6 @@ SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
677705
// but that's all clangd has needed so far.
678706
const SourceManager &SM = AST.getSourceManager();
679707
FileID FID = SM.getMainFileID();
680-
if (Begin == End)
681-
std::tie(Begin, End) = pointBounds(Begin, FID, AST);
682708
PrintPolicy.TerseOutput = true;
683709
PrintPolicy.IncludeNewlines = false;
684710

@@ -690,10 +716,6 @@ SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
690716
dlog("Built selection tree\n{0}", *this);
691717
}
692718

693-
SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
694-
unsigned Offset)
695-
: SelectionTree(AST, Tokens, Offset, Offset) {}
696-
697719
const Node *SelectionTree::commonAncestor() const {
698720
const Node *Ancestor = Root;
699721
while (Ancestor->Children.size() == 1 && !Ancestor->Selected)

‎clang-tools-extra/clangd/Selection.h

+39-10
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,14 @@
2929
// - we determine which low-level nodes are partly or completely covered
3030
// by the selection.
3131
// - we expose a tree of the selected nodes and their lexical parents.
32+
//
33+
// Sadly LSP specifies locations as being between characters, and this causes
34+
// some ambiguities we cannot cleanly resolve:
35+
// lhs+rhs // targeting '+' or 'lhs'?
36+
// ^ // in GUI editors, double-clicking 'lhs' yields this position!
37+
//
38+
// The best we can do in these cases is try both, which leads to the awkward
39+
// SelectionTree::createEach() API.
3240
//===----------------------------------------------------------------------===//
3341

3442
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SELECTION_H
@@ -64,16 +72,32 @@ namespace clangd {
6472
// point back into the AST it was constructed with.
6573
class SelectionTree {
6674
public:
67-
// Creates a selection tree at the given byte offset in the main file.
68-
// This is approximately equivalent to a range of one character.
69-
// (Usually, the character to the right of Offset, sometimes to the left).
70-
SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
71-
unsigned Offset);
72-
// Creates a selection tree for the given range in the main file.
73-
// The range includes bytes [Start, End).
74-
// If Start == End, uses the same heuristics as SelectionTree(AST, Start).
75-
SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
76-
unsigned Start, unsigned End);
75+
// Create selection trees for the given range, and pass them to Func.
76+
//
77+
// There may be multiple possible selection trees:
78+
// - if the range is empty and borders two tokens, a tree for the right token
79+
// and a tree for the left token will be yielded.
80+
// - Func should return true on success (stop) and false on failure (continue)
81+
//
82+
// Always yields at least one tree. If no tokens are touched, it is empty.
83+
static bool createEach(ASTContext &AST, const syntax::TokenBuffer &Tokens,
84+
unsigned Begin, unsigned End,
85+
llvm::function_ref<bool(SelectionTree)> Func);
86+
87+
// Create a selection tree for the given range.
88+
//
89+
// Where ambiguous (range is empty and borders two tokens), prefer the token
90+
// on the right.
91+
static SelectionTree createRight(ASTContext &AST,
92+
const syntax::TokenBuffer &Tokens,
93+
unsigned Begin, unsigned End);
94+
95+
// Copies are no good - contain pointers to other nodes.
96+
SelectionTree(const SelectionTree &) = delete;
97+
SelectionTree &operator=(const SelectionTree &) = delete;
98+
// Moves are OK though - internal storage is pointer-stable when moved.
99+
SelectionTree(SelectionTree &&) = default;
100+
SelectionTree &operator=(SelectionTree &&) = default;
77101

78102
// Describes to what extent an AST node is covered by the selection.
79103
enum Selection : unsigned char {
@@ -121,6 +145,11 @@ class SelectionTree {
121145
const Node &root() const { return *Root; }
122146

123147
private:
148+
// Creates a selection tree for the given range in the main file.
149+
// The range includes bytes [Start, End).
150+
SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
151+
unsigned Start, unsigned End);
152+
124153
std::deque<Node> Nodes; // Stable-pointer storage.
125154
const Node *Root;
126155
clang::PrintingPolicy PrintPolicy;

‎clang-tools-extra/clangd/SemanticSelection.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
3939
}
4040

4141
// Get node under the cursor.
42-
SelectionTree ST(AST.getASTContext(), AST.getTokens(), *Offset);
42+
SelectionTree ST = SelectionTree::createRight(
43+
AST.getASTContext(), AST.getTokens(), *Offset, *Offset);
4344
for (const auto *Node = ST.commonAncestor(); Node != nullptr;
4445
Node = Node->Parent) {
4546
if (const Decl *D = Node->ASTNode.get<Decl>()) {

‎clang-tools-extra/clangd/XRefs.cpp

+9-8
Original file line numberDiff line numberDiff line change
@@ -131,15 +131,16 @@ SymbolLocation getPreferredLocation(const Location &ASTLoc,
131131

132132
std::vector<const Decl *> getDeclAtPosition(ParsedAST &AST, SourceLocation Pos,
133133
DeclRelationSet Relations) {
134-
FileID FID;
135-
unsigned Offset;
136-
std::tie(FID, Offset) = AST.getSourceManager().getDecomposedSpellingLoc(Pos);
137-
SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
134+
unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
138135
std::vector<const Decl *> Result;
139-
if (const SelectionTree::Node *N = Selection.commonAncestor()) {
140-
auto Decls = targetDecl(N->ASTNode, Relations);
141-
Result.assign(Decls.begin(), Decls.end());
142-
}
136+
SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Offset,
137+
Offset, [&](SelectionTree ST) {
138+
if (const SelectionTree::Node *N =
139+
ST.commonAncestor())
140+
llvm::copy(targetDecl(N->ASTNode, Relations),
141+
std::back_inserter(Result));
142+
return !Result.empty();
143+
});
143144
return Result;
144145
}
145146

‎clang-tools-extra/clangd/refactor/Rename.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
8080
unsigned Offset =
8181
AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
8282

83-
SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
83+
SelectionTree Selection = SelectionTree::createRight(
84+
AST.getASTContext(), AST.getTokens(), Offset, Offset);
8485
const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
8586
if (!SelectedNode)
8687
return {};

‎clang-tools-extra/clangd/refactor/Tweak.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,10 @@ void validateRegistry() {
4646
} // namespace
4747

4848
Tweak::Selection::Selection(const SymbolIndex *Index, ParsedAST &AST,
49-
unsigned RangeBegin, unsigned RangeEnd)
49+
unsigned RangeBegin, unsigned RangeEnd,
50+
SelectionTree ASTSelection)
5051
: Index(Index), AST(&AST), SelectionBegin(RangeBegin),
51-
SelectionEnd(RangeEnd),
52-
ASTSelection(AST.getASTContext(), AST.getTokens(), RangeBegin, RangeEnd) {
52+
SelectionEnd(RangeEnd), ASTSelection(std::move(ASTSelection)) {
5353
auto &SM = AST.getSourceManager();
5454
Code = SM.getBufferData(SM.getMainFileID());
5555
Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin);

‎clang-tools-extra/clangd/refactor/Tweak.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class Tweak {
4848
/// Input to prepare and apply tweaks.
4949
struct Selection {
5050
Selection(const SymbolIndex *Index, ParsedAST &AST, unsigned RangeBegin,
51-
unsigned RangeEnd);
51+
unsigned RangeEnd, SelectionTree ASTSelection);
5252
/// The text of the active document.
5353
llvm::StringRef Code;
5454
/// The Index for handling codebase related queries.

‎clang-tools-extra/clangd/unittests/FindTargetTests.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ class TargetDeclTest : public ::testing::Test {
7777
auto AST = TU.build();
7878
EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty()) << Code;
7979
llvm::Annotations::Range R = A.range();
80-
SelectionTree Selection(AST.getASTContext(), AST.getTokens(), R.Begin,
81-
R.End);
80+
auto Selection = SelectionTree::createRight(
81+
AST.getASTContext(), AST.getTokens(), R.Begin, R.End);
8282
const SelectionTree::Node *N = Selection.commonAncestor();
8383
if (!N) {
8484
ADD_FAILURE() << "No node selected!\n" << Code;

‎clang-tools-extra/clangd/unittests/HoverTests.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ TEST(Hover, NoHover) {
568568
}
569569
)cpp",
570570
R"cpp(// Template auto parameter. Nothing (Not useful).
571-
template<^auto T>
571+
template<a^uto T>
572572
void func() {
573573
}
574574
void foo() {

0 commit comments

Comments
 (0)