[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Reverted in f0604e73a4daa35a10eb17a998657d6c4bd0e971 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks compile on most bots in the "clang" section on 
http://lab.llvm.org:8011/console , probably the ones that use gcc as host 
compiler.

Here's an example:
http://lab.llvm.org:8011/builders/clang-cmake-armv7-selfhost-neon/builds/2714/steps/build%20stage%201/logs/stdio


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 60850 tests passed, 0 failed 
and 726 were skipped.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D71345#1783587 , @sammccall wrote:

> Indeed, sorry - I meant that if we incorporated it into an LSP server, 
> there'd be no way to target it in methods that take a position rather than a 
> selection (go to defn, hover etc).


Ah, right, I forgot that LSP tends to send positions rather than ranges as 
inputs. (I hope that changes in the future, e.g. an issue 
 is on file 
for hover.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb60896fad926: [clangd] Fall back to selecting 
token-before-cursor if token-after-cursor fails. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D71345?vs=233804=233805#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Tweak.cpp
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1971,7 +1971,7 @@
   // Basic check for function body and signature.
   EXPECT_AVAILABLE(R"cpp(
 class Bar {
-  [[void [[f^o^o]]() [[{ return; }
+  [[void [[f^o^o^]]() [[{ return; }
 };
 
 void foo();
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -63,12 +63,33 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
+// Prepare and apply the specified tweak based on the selection in Input.
+// Returns None if and only if prepare() failed.
+llvm::Optional>
+applyTweak(ParsedAST , const Annotations , StringRef TweakID,
+   const SymbolIndex *Index) {
+  auto Range = rangeOrPoint(Input);
+  llvm::Optional> Result;
+  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
+Range.second, [&](SelectionTree ST) {
+  Tweak::Selection S(Index, AST, Range.first,
+ Range.second, std::move(ST));
+  if (auto T = prepareTweak(TweakID, S)) {
+Result = (*T)->apply(S);
+return true;
+  } else {
+llvm::consumeError(T.takeError());
+return false;
+  }
+});
+  return Result;
+}
+
 MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -76,12 +97,11 @@
   TU.ExtraArgs = ExtraArgs;
   TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index, AST, Selection.first, Selection.second);
-  auto PrepareResult = prepareTweak(TweakID, S);
-  if (PrepareResult)
-return true;
-  llvm::consumeError(PrepareResult.takeError());
-  return false;
+  auto Result = applyTweak(AST, Input, TweakID, Index);
+  // We only care if prepare() succeeded, but must handle Errors.
+  if (Result && !*Result)
+consumeError(Result->takeError());
+  return Result.hasValue();
 }
 
 } // namespace
@@ -90,8 +110,6 @@
  llvm::StringMap *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
-
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -99,23 +117,20 @@
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index.get(), AST, Selection.first, Selection.second);
 
-  auto T = prepareTweak(TweakID, S);
-  if (!T) {
-llvm::consumeError(T.takeError());
-return "unavailable";
-  }
-  llvm::Expected Result = (*T)->apply(S);
+  auto Result = applyTweak(AST, Input, TweakID, Index.get());
   if (!Result)
-return "fail: " + llvm::toString(Result.takeError());
-  if (Result->ShowMessage)
-return "message:\n" + *Result->ShowMessage;
-  if (Result->ApplyEdits.empty())
+return "unavailable";
+  if (!*Result)
+return "fail: " + llvm::toString(Result->takeError());
+  const auto  = **Result;
+  if ((*Result)->ShowMessage)
+return "message:\n" + *Effect.ShowMessage;
+ 

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 233804.
sammccall added a comment.

Bias selection-tree right for Hover.
In VSCode (and presumably other GUI-based editors) the location sent is always
the one on the left of the character under the cursor.

Clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Tweak.cpp
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -254,7 +254,6 @@
   assert(Loc.isFileID());
   llvm::ArrayRef All =
   Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
-  // Comparing SourceLocations is well-defined within a FileID.
   auto *Right = llvm::partition_point(
   All, [&](const syntax::Token ) { return Tok.location() < Loc; });
   bool AcceptRight = Right != All.end() && Right->location() <= Loc;
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1971,7 +1971,7 @@
   // Basic check for function body and signature.
   EXPECT_AVAILABLE(R"cpp(
 class Bar {
-  [[void [[f^o^o]]() [[{ return; }
+  [[void [[f^o^o^]]() [[{ return; }
 };
 
 void foo();
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -63,12 +63,33 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
+// Prepare and apply the specified tweak based on the selection in Input.
+// Returns None if and only if prepare() failed.
+llvm::Optional>
+applyTweak(ParsedAST , const Annotations , StringRef TweakID,
+   const SymbolIndex *Index) {
+  auto Range = rangeOrPoint(Input);
+  llvm::Optional> Result;
+  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
+Range.second, [&](SelectionTree ST) {
+  Tweak::Selection S(Index, AST, Range.first,
+ Range.second, std::move(ST));
+  if (auto T = prepareTweak(TweakID, S)) {
+Result = (*T)->apply(S);
+return true;
+  } else {
+llvm::consumeError(T.takeError());
+return false;
+  }
+});
+  return Result;
+}
+
 MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -76,12 +97,11 @@
   TU.ExtraArgs = ExtraArgs;
   TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index, AST, Selection.first, Selection.second);
-  auto PrepareResult = prepareTweak(TweakID, S);
-  if (PrepareResult)
-return true;
-  llvm::consumeError(PrepareResult.takeError());
-  return false;
+  auto Result = applyTweak(AST, Input, TweakID, Index);
+  // We only care if prepare() succeeded, but must handle Errors.
+  if (Result && !*Result)
+consumeError(Result->takeError());
+  return Result.hasValue();
 }
 
 } // namespace
@@ -90,8 +110,6 @@
  llvm::StringMap *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
-
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -99,23 +117,20 @@
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index.get(), AST, Selection.first, Selection.second);
 
-  auto T = prepareTweak(TweakID, S);
-  if 

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

OK, I'm going to land this with some reservations. It's a mess, but the number 
of callsites isn't overwhelming.
I'd been hoping to get away with bias-right, but we keep getting complaints 
about this.
It's hard to find an approach that solves this, doesn't obviously break other 
things, and is close enough to what we do now.
Happy to revisit in future, would be nice to get rid of this wart.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D71345#1783579 , @nridge wrote:

> In D71345#1783092 , @sammccall wrote:
>
> > (though personally I'd find it frustrating to have no way to target `b` in 
> > `a+b+c`).
>
>
> (For completeness, there is a way to target `b` in `a+b+c`: by selecting `b` 
> (such that your selection range has length 1 rather than 0). Then, neither 
> `+` node will enclose the selection range, but the `b` node will.)


Indeed, sorry - I meant that if we incorporated it into an LSP server, there'd 
be no way to target it in methods that take a position rather than a selection 
(go to defn, hover etc).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D71345#1783579 , @nridge wrote:

> (For completeness, there is a way to target `b` in `a+b+c`: by selecting `b` 
> (such that your selection range has length 1 rather than 0). Then, neither 
> `+` node will enclose the selection range, but the `b` node will.)


yes but this doesn't work with most of the LSP features, e.g. go-to-def, hover, 
etc.




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:443
+break;
+  } else {
+Effect = T.takeError();

nit: no need for else


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D71345#1783092 , @sammccall wrote:

> (though personally I'd find it frustrating to have no way to target `b` in 
> `a+b+c`).


(For completeness, there is a way to target `b` in `a+b+c`: by selecting `b` 
(such that your selection range has length 1 rather than 0). Then, neither `+` 
node will enclose the selection range, but the `b` token will.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

tl;dr; LGTM, from my side as long as you are also happy with the extra 
complexity introduced by this to all call sites.

As told in the offline discussions, only problem I have with this approach is 
its intrusiveness. But arguably all of the call sites that cares about what to 
do in such situations already has some sort of handling themselves. So I am OK 
with landing this generic situation and adding some more mental overhead to all 
of the callers, hopefully we could come up with some helpers that would enable 
callers to choose one behavior or the other all the time and get rid of extra 
complexity(not sure if they are likely to stick though).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D71345#1782647 , @nridge wrote:

> Here, both navigations target the overloaded operator, but if you comment out 
> the overloaded operator, they target the respective local variable 
> declarations. I think this is pretty good do-what-I-mean behaviour, arguably 
> better than a categorical preference for the right side over the left side.


That's reasonable in eclipse (though personally I'd find it frustrating to have 
no way to target `b` in `a+b+c`).
It seems broken for editors where the cursor is on a character rather than 
between characters, though.

> - For the purpose of determining "encloses", the cursor being at either 
> boundary of a node counts as a match (so A and B are both considered to 
> enclose the first cursor, and B and C are both considered to enclose the 
> second cursor).
> - The algorithm for go-to-definition is: first look for an enclosing 
> `ImplicitName` node; if one is found, go to its target. Otherwise, look for 
> an enclosing `Name` node and go to its target.
> 
>   Other features use the same API, but perhaps with a different node type; 
> for example, the "define out of line" refactoring looks for an enclosing 
> `FunctionDeclarator` node.
> 
>   Perhaps we could have a simpler `SelectionTree` API along these lines, 
> where the type of desired node informs the selection at each call site?

Thanks for the details on CDT. This diverges quite a lot from what we currently 
have:

- we have abstractions at the preprocessor level (tokens) and at the semantic 
level (clang AST), but nothing modeling the concept of e.g. "name" that we 
would need here.
- our refactorings (tweak) relies on computing the selection by traversing the 
AST first and then querying it for each operation, which doesn't work if the 
query affects the traversal and the traversal is expensive

Syntax trees would address this by providing a tree with the relevant syntactic 
constructs that's cheap to traverse, and would certainly end up replacing 
SelectionTree altogether.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D71345#1781141 , @sammccall wrote:

> How do you feel about the approach here?


I agree that having more of the logic centralized (in `SelectionTree`) is 
preferable to having it directly in a call site like `getDeclAtPosition`.

I also agree that this makes the `SelectionTree` API a bit clunky.

I tested the behaviour in Eclipse CDT, on the following testcase:

  struct Material {
  friend Material operator+(Material, Material);
  };
  
  void foo() {
  Material hydrogen, oxygen;
  hydrogen^+^oxygen;
  }

Here, both navigations target the overloaded operator, but if you comment out 
the overloaded operator, they target the respective local variable 
declarations. I think this is pretty good do-what-I-mean behaviour, arguably 
better than a categorical preference for the right side over the left side.

I can describe how CDT achieves this:

- There is an API that takes a textual selection, and an AST node type, and 
finds the innermost node of that type that encloses the selection.
- The AST nodes built for the statement are as follows:

  hydrogen+oxygen;
  BCC;   // A and C have type Name, B has type ImplicitName



- (If the overloaded operator is commented out, there is no node B.)
- For the purpose of determining "encloses", the cursor being at either 
boundary of a node counts as a match (so A and B are both considered to enclose 
the first cursor, and B and C are both considered to enclose the second cursor).
- The algorithm for go-to-definition is: first look for an enclosing 
`ImplicitName` node; if one is found, go to its target. Otherwise, look for an 
enclosing `Name` node and go to its target.

Other features will use the same API, but perhaps with a different node type; 
for example, the "define out of line" refactoring will look for an enclosing 
`FunctionDeclarator` node.

Perhaps we could have a simpler `SelectionTree` API along these lines, where 
the type of desired node informs the selection at each call site?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D71345#1780632 , @nridge wrote:

> I tried to do a less general version of this (for go-to-definition only) in 
> D70727  :)


Ah, I hadn't seen that. After thinking about this a bit, I think the behavior 
in that patch is OK, and it's complexity that will do us in. More cases keep 
coming up - internally people complained about this in code actions which is 
the hardest case to fix and needs most of this complexity.

How do you feel about the approach here? I like having the description of the 
problem centralized and directing callsites toward a solution.
At the same time, it does make me sad that a fairly nice abstraction becomes so 
much harder to pick up and use, the API is pretty horrendous.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I tried to do a less general version of this (for go-to-definition only) in 
D70727  :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 60717 tests passed, 0 failed 
and 726 were skipped.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or apply this patch 
.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 clang-format.patch 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

So a reasonable outcome here is we conclude this does more harm than good.
If that happens, I'd like to be clear on whether we have a better way of doing 
this in general or just don't think it's worth the cost, and should hack around 
it where possible as in D71284 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 233410.
sammccall added a comment.

Use callbacks to avoid creating too many trees eagerly. Add tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Tweak.cpp
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1903,7 +1903,7 @@
   // Basic check for function body and signature.
   EXPECT_AVAILABLE(R"cpp(
 class Bar {
-  [[void [[f^o^o]]() [[{ return; }
+  [[void [[f^o^o^]]() [[{ return; }
 };
 
 void foo();
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -63,12 +63,33 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
+// Prepare and apply the specified tweak based on the selection in Input.
+// Returns None if and only if prepare() failed.
+llvm::Optional> 
+applyTweak(ParsedAST , const Annotations , StringRef TweakID,
+const SymbolIndex *Index) {
+  auto Range = rangeOrPoint(Input);
+  llvm::Optional> Result;
+  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
+Range.second, [&](SelectionTree ST) {
+  Tweak::Selection S(Index, AST, Range.first,
+ Range.second, std::move(ST));
+  if (auto T = prepareTweak(TweakID, S)) {
+Result = (*T)->apply(S);
+return true;
+  } else {
+llvm::consumeError(T.takeError());
+return false;
+  }
+});
+  return Result;
+}
+
 MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -76,12 +97,11 @@
   TU.ExtraArgs = ExtraArgs;
   TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index, AST, Selection.first, Selection.second);
-  auto PrepareResult = prepareTweak(TweakID, S);
-  if (PrepareResult)
-return true;
-  llvm::consumeError(PrepareResult.takeError());
-  return false;
+  auto Result = applyTweak(AST, Input, TweakID, Index);
+  // We only care if prepare() succeeded, but must handle Errors.
+  if (Result && !*Result)
+consumeError(Result->takeError());
+  return Result.hasValue();
 }
 
 } // namespace
@@ -90,8 +110,6 @@
  llvm::StringMap *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
-
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -99,23 +117,20 @@
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index.get(), AST, Selection.first, Selection.second);
 
-  auto T = prepareTweak(TweakID, S);
-  if (!T) {
-llvm::consumeError(T.takeError());
-return "unavailable";
-  }
-  llvm::Expected Result = (*T)->apply(S);
+  auto Result = applyTweak(AST, Input, TweakID, Index.get());
   if (!Result)
-return "fail: " + llvm::toString(Result.takeError());
-  if (Result->ShowMessage)
-return "message:\n" + *Result->ShowMessage;
-  if (Result->ApplyEdits.empty())
+return "unavailable";
+  if (!*Result)
+return "fail: " + llvm::toString(Result->takeError());
+  const auto  = **Result;
+  if ((*Result)->ShowMessage)
+return "message:\n" + *Effect.ShowMessage;
+  if (Effect.ApplyEdits.empty())
 return "no effect";
 
   std::string EditedMainFile;
-  for (auto  : Result->ApplyEdits) {
+  for (auto  : 

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 233364.
sammccall added a comment.

Make tweaktests do the fallback dance so that they test how the feature 
actually works.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Tweak.cpp
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1903,7 +1903,7 @@
   // Basic check for function body and signature.
   EXPECT_AVAILABLE(R"cpp(
 class Bar {
-  [[void [[f^o^o]]() [[{ return; }
+  [[void [[f^o^o^]]() [[{ return; }
 };
 
 void foo();
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -63,12 +63,30 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
+// Prepare and apply the specified tweak based on the selection in Input.
+// Returns None if and only if prepare() failed.
+llvm::Optional> 
+applyTweak(ParsedAST , const Annotations , StringRef TweakID,
+const SymbolIndex *Index) {
+  auto Range = rangeOrPoint(Input);
+  for (auto  : SelectionTree::create(
+   AST.getASTContext(), AST.getTokens(), Range.first, Range.second)) {
+Tweak::Selection S(Index, AST, Range.first, Range.second,
+   std::move(Selection));
+auto T = prepareTweak(TweakID, S);
+if (T)
+  return (*T)->apply(S);
+else
+  consumeError(T.takeError());
+  }
+  return llvm::None;
+}
+
 MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -76,12 +94,11 @@
   TU.ExtraArgs = ExtraArgs;
   TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index, AST, Selection.first, Selection.second);
-  auto PrepareResult = prepareTweak(TweakID, S);
-  if (PrepareResult)
-return true;
-  llvm::consumeError(PrepareResult.takeError());
-  return false;
+  auto Result = applyTweak(AST, Input, TweakID, Index);
+  // We only care if prepare() succeeded, but must handle Errors.
+  if (Result && !*Result)
+consumeError(Result->takeError());
+  return Result.hasValue();
 }
 
 } // namespace
@@ -90,8 +107,6 @@
  llvm::StringMap *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
-  auto Selection = rangeOrPoint(Input);
-
   TestTU TU;
   TU.Filename = FileName;
   TU.HeaderCode = Header;
@@ -99,23 +114,20 @@
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
-  Tweak::Selection S(Index.get(), AST, Selection.first, Selection.second);
 
-  auto T = prepareTweak(TweakID, S);
-  if (!T) {
-llvm::consumeError(T.takeError());
-return "unavailable";
-  }
-  llvm::Expected Result = (*T)->apply(S);
+  auto Result = applyTweak(AST, Input, TweakID, Index.get());
   if (!Result)
-return "fail: " + llvm::toString(Result.takeError());
-  if (Result->ShowMessage)
-return "message:\n" + *Result->ShowMessage;
-  if (Result->ApplyEdits.empty())
+return "unavailable";
+  if (!*Result)
+return "fail: " + llvm::toString(Result->takeError());
+  const auto  = **Result;
+  if ((*Result)->ShowMessage)
+return "message:\n" + *Effect.ShowMessage;
+  if (Effect.ApplyEdits.empty())
 return "no effect";
 
   std::string EditedMainFile;
-  for (auto  : Result->ApplyEdits) {
+  for (auto  : Effect.ApplyEdits) {
 auto NewText = It.second.apply();
 if (!NewText)
   return "bad edits: " + llvm::toString(NewText.takeError());
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ 

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall added a comment.

This needs tests, but wanted to share the ideas.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71345/new/

https://reviews.llvm.org/D71345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71345: [clangd] Fall back to selecting token-before-cursor if token-after-cursor fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.
sammccall planned changes to this revision.
sammccall added a comment.

This needs tests, but wanted to share the ideas.


The problem:

LSP specifies that Positions are between characters. Therefore when a position
(or an empty range) is used to target elements of the source code, there is an
ambiguity - should we look left or right of the cursor?

Until now, SelectionTree resolved this to the right except in trivial cases
(where there's whitespace, semicolon, or eof on the right).
This meant that it's unable to e.g. out-line `int foo^()` today.

Complicating this, LSP notwithstanding the cursor is *on* a character in many
editors (mostly terminal-based). In these cases there's no ambiguity - we must
"look right" - but there's also no way to tell in LSP.

(Several features currently resolve this by using getBeginningOfIdentifier,
which tries to rewind and supports end-of-identifier. But this relies on
raw lexing and is limited and buggy).

Precedent: well - most other languages aren't so full of densely packed symbols
that we might want to target. Bias-towards-identifier works well enough.
MS C++ for vscode seems to mostly use bias-toward-identifier too.
The problem with this solution is it doesn't provide any way to target some
things such as the constructor call in Foo^(bar());

Presented solution:

When an ambiguous selection is found, we generate *both* possible selection
trees. We try to run the feature on the rightward tree first, and then on the
leftward tree if it fails.

This is basically do-what-I-mean, the main downside is the need to do this on
a feature-by-feature basis (because each feature knows what "fail" means).
The most complicated instance of this is Tweaks, where the preferred selection
may vary tweak-by-tweak.

Wrinkles:

While production behavior is pretty consistent, this introduces some
inconsistency in testing, depending whether the interface we're testing is
inside or outside the "retry" wrapper.

In particular, for many features like Hover, the unit tests will show production
behavior, while for Tweaks the harness would have to run the loop itself if
we want this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71345

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Tweak.cpp
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -647,3 +647,18 @@
   }
   return OS.str();
 }
+
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  assert(Loc.isFileID());
+  llvm::ArrayRef All =
+  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
+  // SourceLocation < SourceLocation is OK for one file ID.
+  auto *Right = llvm::partition_point(All, [&](const syntax::Token ) {
+return Tok.location() < Loc;
+  });
+  bool AcceptRight = Right != All.end() && !(Loc < Right->location());
+  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() == Loc;
+  return llvm::makeArrayRef(Right - int(AcceptLeft), Right + int(AcceptRight));
+}
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -309,6 +309,11 @@
   const SourceManager *SourceMgr;
 };
 
+/// Return the spelled tokens that overlap or touch spelling location Loc.
+/// This always returns 0-2 tokens.
+llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
+
 /// Lex the text buffer, corresponding to \p FID, in raw mode and record the
 /// resulting spelled tokens. Does minimal post-processing on raw identifiers,
 /// setting the appropriate token kind (instead of the raw_identifier reported
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -76,7 +76,10 @@
   TU.ExtraArgs = ExtraArgs;
   TU.AdditionalFiles =