Skip to content

Commit

Permalink
Revert "[clangd] Reapply b60896f Fall back to selecting token-before-…
Browse files Browse the repository at this point in the history 
…cursor if token-after-cursor fails."

This reverts commit a2ce807.

Buildbot failures on GCC due to SelectionTree not being copyable, and
instantiating vector<Selection> in the tweak-handling in ClangdServer.
  • Loading branch information
@sam-mccall
sam-mccall committed Feb 23, 2020
1 parent a2ce807 commit b4b9706
Show file tree
Hide file tree
Showing 14 changed files with 135 additions and 304 deletions.
71 changes: 22 additions & 49 deletions clang-tools-extra/clangd/ClangdServer.cpp
View file
Expand Up @@ -395,24 +395,15 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
WorkScheduler.runWithAST("Rename", File, std::move(Action));
}

// May generate several candidate selections, due to SelectionTree ambiguity.
static llvm::Expected<std::vector<Tweak::Selection>>
static llvm::Expected<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();
std::vector<Tweak::Selection> Result;
SelectionTree::createEach(AST.AST.getASTContext(), AST.AST.getTokens(),
*Begin, *End, [&](SelectionTree T) {
Result.emplace_back(AST.Inputs.Index, AST.AST,
*Begin, *End, std::move(T));
return false;
});
assert(!Result.empty() && "Expected at least one SelectionTree");
return Result;
return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
}

void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
Expand All @@ -421,21 +412,12 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
this](Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
return CB(InpAST.takeError());
auto Selections = tweakSelection(Sel, *InpAST);
if (!Selections)
return CB(Selections.takeError());
auto Selection = tweakSelection(Sel, *InpAST);
if (!Selection)
return CB(Selection.takeError());
std::vector<TweakRef> Res;
// 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());
}
}
for (auto &T : prepareTweaks(*Selection, TweakFilter))
Res.push_back({T->id(), T->title(), T->intent()});

CB(std::move(Res));
};
Expand All @@ -450,30 +432,21 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
FS = FSProvider.getFileSystem()](Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
return CB(InpAST.takeError());
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));
}
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));
}
return CB(std::move(*Effect));
};
Expand Down
7 changes: 2 additions & 5 deletions clang-tools-extra/clangd/Hover.cpp
View file
Expand Up @@ -537,12 +537,9 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
llvm::consumeError(Offset.takeError());
return llvm::None;
}
// 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);
SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
std::vector<const Decl *> Result;
if (const SelectionTree::Node *N = ST.commonAncestor()) {
if (const SelectionTree::Node *N = Selection.commonAncestor()) {
auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
if (!Decls.empty()) {
HI = getHoverContents(Decls.front(), Index);
Expand Down
76 changes: 27 additions & 49 deletions clang-tools-extra/clangd/Selection.cpp
View file
Expand Up @@ -142,11 +142,6 @@ 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.
Expand Down Expand Up @@ -177,7 +172,9 @@ class SelectionTester {
});
// Precompute selectedness and offset for selected spelled tokens.
for (const syntax::Token *T = SelFirst; T < SelLimit; ++T) {
if (shouldIgnore(*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)
continue;
SpelledTokens.emplace_back();
Tok &S = SpelledTokens.back();
Expand Down Expand Up @@ -674,49 +671,24 @@ std::string SelectionTree::Node::kind() const {
return std::move(OS.str());
}

// 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);
// 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};
}

SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
Expand All @@ -726,6 +698,8 @@ 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;

Expand All @@ -737,6 +711,10 @@ 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)
Expand Down
49 changes: 10 additions & 39 deletions clang-tools-extra/clangd/Selection.h
View file
Expand Up @@ -29,14 +29,6 @@
// - 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
Expand Down Expand Up @@ -72,32 +64,16 @@ namespace clangd {
// point back into the AST it was constructed with.
class SelectionTree {
public:
// 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;
// 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);

// Describes to what extent an AST node is covered by the selection.
enum Selection : unsigned char {
Expand Down Expand Up @@ -145,11 +121,6 @@ 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;
Expand Down
3 changes: 1 addition & 2 deletions clang-tools-extra/clangd/SemanticSelection.cpp
View file
Expand Up @@ -39,8 +39,7 @@ llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
}

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

0 comments on commit b4b9706

Please sign in to comment.