[PATCH] D62855: [clangd] Implementation of auto type expansion.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Let's chat offline about how to land this.

Main points are:

- we may just be able to use the AutoTypeLoc instead of the separate RecursiveASTVisitor. (If so, we can do more checks in prepare())
- I have lots of questions about where to draw the line on pretty-printing, and whether the initial version here is a useful first step. Maybe we should just move this part to AST.h and stub it out, with a plan to incrementally improve later.



================
Comment at: clang-tools-extra/clangd/AST.h:20
 #include "clang/Lex/MacroInfo.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 
----------------
(can now revert this change)


================
Comment at: clang-tools-extra/clangd/XRefs.h:143
 
+/// Retrieves the deduced type at a given location (auto, decltype).
+/// SourceLocationBeg must point to the first character of the token
----------------
Can you elaborate slightly whether this returns the AutoType/DecltypeType or its underlying type?


================
Comment at: clang-tools-extra/clangd/XRefs.h:144
+/// Retrieves the deduced type at a given location (auto, decltype).
+/// SourceLocationBeg must point to the first character of the token
+llvm::Optional<QualType> getDeducedType(ParsedAST &AST,
----------------
nit: I'd change the second sentence to be "Retuns None unless SourceLocationBeg starts an auto/decltype token".

The current wording isn't totally specific about the token, or what happens in other cases (UB is common for preconditions)


================
Comment at: clang-tools-extra/clangd/XRefs.h:150
+/// SourceLocationBeg must point to the first character of the token
+bool hasDeducedType(ParsedAST &AST, SourceLocation SourceLocationBeg);
+
----------------
This appears to be unused, and isn't really worth having unless it's significantly more efficient than getDeducedType().hasValue() and we think that matters.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:32
+  CachedLocation = findAutoType(Node);
+  return CachedLocation != llvm::None;
+}
----------------
&& `!CachedLocation->getTypePtr()->getDeducedType()->isNull()`?

to avoid triggering in e.g. dependent code like
```
template <typename T> void f() {
auto X = T::foo();
}
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:39
+  llvm::Optional<clang::QualType> DeductedType =
+      getDeducedType(Inputs.AST, CachedLocation->getBeginLoc());
+
----------------
is this ever not just `CachedLocation->getTypePtr()->getDeducedType()`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:47
+  // if it's a lambda expression, return an error message
+  if (isa<RecordType>(*DeductedType) and
+      dyn_cast<RecordType>(*DeductedType)->getDecl()->isLambda()) {
----------------
again, this is actually a cheap test (if we don't need to use the deduced type visitor), we can lift it into prepare


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:53
+
+  SourceRange OriginalRange(CachedLocation->getBeginLoc(),
+                            CachedLocation->getEndLoc());
----------------
nit: this is just CachedLocation->getSourceRange()


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:55
+                            CachedLocation->getEndLoc());
+  PrintingPolicy PP(Inputs.AST.getASTContext().getPrintingPolicy());
+  PP.SuppressTagKeyword = 1;
----------------
this logic around pretty-printing a type for insertion in code is going to be:
 - eventually more complicated than this: (e.g. this won't trim the "clangd" from `std::vector<clangd::Tweak>`)
 - needed in several places, like "extract expression" refactoring
 - fairly easily isolated from the rest of the code here

Can we move this to AST.h, with some signature like:
`std::string printType(QualType, DeclContext &)`

Note that the namespace and (most) governing using decls/directives can be obtained from the DC (printNamespaceScope in AST.h).
The limitations in the current version are fine for now, but we should document them.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:68
+const llvm::Optional<AutoTypeLoc>
+ExpandAutoType::findAutoType(const SelectionTree::Node* StartNode) {
+  auto Node = StartNode;
----------------
I'm confused about this - how can the user select the child of an AutoTypeLoc rather than the AutoTypeLoc itself? i.e. do we need the loop?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:70
+  auto Node = StartNode;
+  while (Node != nullptr) {
+    auto TypeNode = Node->ASTNode.get<TypeLoc>();
----------------
(nit: for loop in disguise!)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:108
+    const SelectionTree::Node* StartNode) {
+  auto Node = StartNode;
+  while (Node != nullptr) {
----------------
also a for loop in disguise


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:111
+    LLVM_DEBUG(Node->ASTNode.print());
+    if (const Decl* Current = Node->ASTNode.get<Decl>()) {
+      if (auto CurrentNameSpace = dyn_cast<NamespaceDecl>(Current)) {
----------------
nit: you can combine these with

`if (const auto* NSD = dyn_cast_or_null<NamespaceDecl>(Node->ASTNode.get<Decl>()))`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:112
+    if (const Decl* Current = Node->ASTNode.get<Decl>()) {
+      if (auto CurrentNameSpace = dyn_cast<NamespaceDecl>(Current)) {
+        return CurrentNameSpace->getQualifiedNameAsString();
----------------
nit: prefer auto*


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:113
+      if (auto CurrentNameSpace = dyn_cast<NamespaceDecl>(Current)) {
+        return CurrentNameSpace->getQualifiedNameAsString();
+      }
----------------
with namespaces, both inline and (probably) anonymous namespaces should be ignored, which I don't think this function will do.

note there can also be non-namespace qualifiers like `MyClass::MyNestedClass`.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:396
+  )cpp";
+  checkTransform(ID, Input, Output);
+}
----------------
can we have one for function pointers?

```
int foo();
auto x = &foo;
```

The correct replacement would be `int (*x)() = &foo;` but I think we should just aim to produce no edits in such cases.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2111
 
+TEST(GetDeductedType, KwAutoExpansion) {
+  struct Test {
----------------
(still "deducted" here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62855