[PATCH] D70194: [clangd] Ignore more implicit nodes in computing selection.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbcbb10e2d08: [clangd] Ignore more implicit nodes in 
computing selection. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D70194?vs=229141=229319#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70194

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -213,11 +213,18 @@
   {
   R"cpp(
 struct S {
-  int foo;
-  int bar() { return [[f^oo]]; }
+  int foo() const;
+  int bar() { return [[f^oo]](); }
 };
   )cpp",
-  "MemberExpr", // Not implicit CXXThisExpr!
+  "MemberExpr", // Not implicit CXXThisExpr, or its implicit cast!
+  },
+  {
+  R"cpp(
+auto lambda = [](const char*){ return 0; };
+int x = lambda([["y^"]]);
+  )cpp",
+  "StringLiteral", // Not DeclRefExpr to operator()!
   },
 
   // Point selections.
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -10,9 +10,11 @@
 #include "Logger.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
@@ -145,6 +147,32 @@
 }
 #endif
 
+bool isImplicit(const Stmt* S) {
+  // Some Stmts are implicit and shouldn't be traversed, but there's no
+  // "implicit" attribute on Stmt/Expr.
+  // Unwrap implicit casts first if present (other nodes too?).
+  if (auto *ICE = llvm::dyn_cast(S))
+S = ICE->getSubExprAsWritten();
+  // Implicit this in a MemberExpr is not filtered out by RecursiveASTVisitor.
+  // It would be nice if RAV handled this (!shouldTraverseImplicitCode()).
+  if (auto *CTI = llvm::dyn_cast(S))
+if (CTI->isImplicit())
+  return true;
+  // Refs to operator() and [] are (almost?) always implicit as part of calls.
+  if (auto *DRE = llvm::dyn_cast(S)) {
+if (auto *FD = llvm::dyn_cast(DRE->getDecl())) {
+  switch (FD->getOverloadedOperator()) {
+  case OO_Call:
+  case OO_Subscript:
+return true;
+  default:
+break;
+  }
+}
+  }
+  return false;
+}
+
 // We find the selection by visiting written nodes in the AST, looking for 
nodes
 // that intersect with the selected character range.
 //
@@ -210,13 +238,8 @@
   }
   // Stmt is the same, but this form allows the data recursion optimization.
   bool dataTraverseStmtPre(Stmt *X) {
-if (!X)
+if (!X || isImplicit(X))
   return false;
-// Implicit this in a MemberExpr is not filtered out by 
RecursiveASTVisitor.
-// It would be nice if RAV handled this (!shouldTRaverseImplicitCode()).
-if (auto *CTI = llvm::dyn_cast(X))
-  if (CTI->isImplicit())
-return false;
 auto N = DynTypedNode::create(*X);
 if (canSafelySkipNode(N))
   return false;


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -213,11 +213,18 @@
   {
   R"cpp(
 struct S {
-  int foo;
-  int bar() { return [[f^oo]]; }
+  int foo() const;
+  int bar() { return [[f^oo]](); }
 };
   )cpp",
-  "MemberExpr", // Not implicit CXXThisExpr!
+  "MemberExpr", // Not implicit CXXThisExpr, or its implicit cast!
+  },
+  {
+  R"cpp(
+auto lambda = [](const char*){ return 0; };
+int x = lambda([["y^"]]);
+  )cpp",
+  "StringLiteral", // Not DeclRefExpr to operator()!
   },
 
   // Point selections.
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -10,9 +10,11 @@
 #include "Logger.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
+#include 

[PATCH] D70194: [clangd] Ignore more implicit nodes in computing selection.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM, thanks!




Comment at: clang-tools-extra/clangd/Selection.cpp:162
+  // Refs to operator() and [] are (almost?) always implicit as part of calls.
+  if (auto *DRE = llvm::dyn_cast(S))
+if (auto *FD = llvm::dyn_cast(DRE->getDecl()))

nit: braces



Comment at: clang-tools-extra/clangd/Selection.cpp:163
+  if (auto *DRE = llvm::dyn_cast(S))
+if (auto *FD = llvm::dyn_cast(DRE->getDecl()))
+  switch (FD->getOverloadedOperator()) {

nit: braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70194



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

[PATCH] D70194: [clangd] Ignore more implicit nodes in computing selection.

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

merge_guards_bot added a comment.

Build result: pass - 59972 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70194



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

[PATCH] D70194: [clangd] Ignore more implicit nodes in computing selection.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: kadircet, lh123.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

The DeclRefExpr for the callee of overloaded `operator()` and `operator[]` are
assigned the range of the paren/bracket lists in the AST.
These are better thought of as implicit (at least `()` - `[] is murkier).
But there's no bit on Expr for implicit, so just ignore them on our side.

While here, deal with the case where an implicit stmt (e.g. implicit-this)
is wrapped in an implicit cast. Previously we ignored the statement but not
the cast, and so the cast ended up being selected.

Fixes https://github.com/clangd/clangd/issues/195


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70194

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -213,11 +213,18 @@
   {
   R"cpp(
 struct S {
-  int foo;
-  int bar() { return [[f^oo]]; }
+  int foo() const;
+  int bar() { return [[f^oo]](); }
 };
   )cpp",
-  "MemberExpr", // Not implicit CXXThisExpr!
+  "MemberExpr", // Not implicit CXXThisExpr, or its implicit cast!
+  },
+  {
+  R"cpp(
+auto lambda = [](const char*){ return 0; };
+int x = lambda([["y^"]]);
+  )cpp",
+  "StringLiteral", // Not DeclRefExpr to operator()!
   },
 
   // Point selections.
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -10,9 +10,11 @@
 #include "Logger.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
@@ -145,6 +147,30 @@
 }
 #endif
 
+bool isImplicit(const Stmt* S) {
+  // Some Stmts are implicit and shouldn't be traversed, but there's no
+  // "implicit" attribute on Stmt/Expr.
+  // Unwrap implicit casts first if present (other nodes too?).
+  if (auto *ICE = llvm::dyn_cast(S))
+S = ICE->getSubExprAsWritten();
+  // Implicit this in a MemberExpr is not filtered out by RecursiveASTVisitor.
+  // It would be nice if RAV handled this (!shouldTraverseImplicitCode()).
+  if (auto *CTI = llvm::dyn_cast(S))
+if (CTI->isImplicit())
+  return true;
+  // Refs to operator() and [] are (almost?) always implicit as part of calls.
+  if (auto *DRE = llvm::dyn_cast(S))
+if (auto *FD = llvm::dyn_cast(DRE->getDecl()))
+  switch (FD->getOverloadedOperator()) {
+  case OO_Call:
+  case OO_Subscript:
+return true;
+  default:
+break;
+  }
+  return false;
+}
+
 // We find the selection by visiting written nodes in the AST, looking for 
nodes
 // that intersect with the selected character range.
 //
@@ -210,13 +236,8 @@
   }
   // Stmt is the same, but this form allows the data recursion optimization.
   bool dataTraverseStmtPre(Stmt *X) {
-if (!X)
+if (!X || isImplicit(X))
   return false;
-// Implicit this in a MemberExpr is not filtered out by 
RecursiveASTVisitor.
-// It would be nice if RAV handled this (!shouldTRaverseImplicitCode()).
-if (auto *CTI = llvm::dyn_cast(X))
-  if (CTI->isImplicit())
-return false;
 auto N = DynTypedNode::create(*X);
 if (canSafelySkipNode(N))
   return false;


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -213,11 +213,18 @@
   {
   R"cpp(
 struct S {
-  int foo;
-  int bar() { return [[f^oo]]; }
+  int foo() const;
+  int bar() { return [[f^oo]](); }
 };
   )cpp",
-  "MemberExpr", // Not implicit CXXThisExpr!
+  "MemberExpr", // Not implicit CXXThisExpr, or its implicit cast!
+  },
+  {
+  R"cpp(
+auto lambda = [](const char*){ return 0; };
+int x = lambda([["y^"]]);
+  )cpp",
+  "StringLiteral", // Not DeclRefExpr to operator()!
   },
 
   // Point selections.
Index: clang-tools-extra/clangd/Selection.cpp