[clangd] Improve symbol qualification in DefineInline code action

[kadircet]

Summary

Currently define inline action fully qualifies any names in the
function body, which is not optimal and definitely natural.

This patch tries to improve the situation by dropping any name
specifiers shared by function and names spelled in the body. For example
if you are moving definition of a function in namespace clang::clangd,
and body has any decl's coming from clang or clang::clangd namespace,
those qualifications won't be added since they are redundant.

It also drops any qualifiers that are visible in target context.

[ilya-biryukov]

We seem to have trouble only in presence of using declarations and using namespace directives.
I wonder how complicated it would be to take them into account instead. That would clearly be easier to read, as we'll hit right into the center of the problem.

Could you describe why handling using declarations and using namespace directives looks too complicated?

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

141

Could you add an example to the description?

144

Maybe accept a NamedDecl we need to qualify instead of SourceContext?
This would help with avoiding confusion with two parameters of the same type and I believe we'll need it if we ever want to support using declarations in the same function.

160

It might be possible to have non-namespace contexts here:

namespace ns {
struct X {
  void foo();

  static void target_struct();
};
void target_ns();
}


void ns::X::foo() {
  // we start with non-namespace context.
  target_struct();
  target_ns(); 
}

Not sure if we run in those cases, though

clang-tools-extra/clangd/unittests/TweakTests.cpp

1682

We don't support using namespace and yet we choose to use them in the tests here.
Is there a case where we need to qualify without using namespace directive and using declarations?

[kuhnel]

Build failed during patching:

00:03:00.433 [Phabricator] $ arc patch --diff 225207 --nocommit --nobranch --conduit-uri=https://reviews.llvm.org ********
00:03:01.736  INFO  Base commit is not in local repository; trying to fetch.
00:03:02.694 
00:03:02.694 
00:03:02.695     This diff is against commit 48ec5c8b81c23061eac4fe6f3a14e2e876b8d265, but
00:03:02.695     the commit is nowhere in the working copy. Try to apply it against the
00:03:02.695     current working copy state? (fdccf28697e5debe861247d218cbbecf9fd4323e)
00:03:02.695     [Y/n]  Exception 
00:03:02.695 The program is attempting to read user input, but stdin is being piped from some other source (not a TTY).
00:03:02.695 (Run with `--trace` for a full exception trace.)

[kadircet]

In D69033#1710955, @ilya-biryukov wrote:

We seem to have trouble only in presence of using declarations and using namespace directives.
I wonder how complicated it would be to take them into account instead. That would clearly be easier to read, as we'll hit right into the center of the problem.

Could you describe why handling using declarations and using namespace directives looks too complicated?

As discussed offline, changed the patch to handle using directives. Using declarations are handled implicitly, as we bail out if they are not visible from target
location.

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

160

the SourceContext is guaranteed to be a namespace context to be start with, since we only call this function in qualifyAllDecls after making sure current decl is a namespace decl.
So there is no way for any of its parents to be anything but a namespacedecl.

clang-tools-extra/clangd/unittests/TweakTests.cpp

1682

We don't support using namespace and yet we choose to use them in the tests here.

I believe you misunderstood the "doesn't take using directives into account" part. It is not that we don't support them, it is just the getQualification function generates suboptimal specifiers in the presence of using directives/declarations. For example:

namespace ns1{
 namespace ns2 { void foo(); }
 using namespace ns2;
 void bar();

 void bar() {
     foo();
 }
}

when we issue an inline on function bar the body will become ns2::foo instead of just foo because we didn't take using namespace ns2 into account.

Is there a case where we need to qualify without using namespace directive and using declarations?

if there are no using directive/declarations then the visible scope of declaration and definition should hopefully be the same and we wouldn't need to qualify anything.
But as I mentioned, it is not that we are not supporting those, we are just not producing

[kadircet]

• Handle using directives

[kadircet]

• Rebase
• Move qualification logic into AST.h so that it can be used by define outline, see D69033.

[ilya-biryukov]

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

195

Why do we need to change this?
My understanding is that we want semantic decl context, not the lexical one here.

199

NIT: it looks like this FIXME belongs to the getQualification function itself.

clang-tools-extra/clangd/unittests/TweakTests.cpp

1674

NIT: could you indent functions inside the namespaces?

[kadircet]

• Address comments
• Rebase
• Get rid of string literals inside macros

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

195

oopsy, was experimenting.

199

I don't think so, as qualification function has no idea about the function body, right?

[ilya-biryukov]

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

199

But it does handle using namespace directives and namespace aliases, so if we ever add this logic it better live in the same place, i.e. inside getQualification function or inside another overload handling that case.

[ilya-biryukov]

clang-tools-extra/clangd/unittests/TweakTests.cpp

1670

NIT: capitalize the last word: DropCommonNameSpecifiers

[ilya-biryukov]

clang-tools-extra/clangd/unittests/TweakTests.cpp

1805

NIT: move the FIXME to the test itself, it's a little hard to find

[kadircet]

• Address comments

clang-tools-extra/clangd/AST.h

 /// you want to qualify clang::clangd::bar::foo in clang::clangd::x, this
 /// function will return bar. Note that the result might be sub-optimal for
 /// classes, e.g. when the \p ND is a member of the base class.
 ///
 /// This version considers all the using namespace directives before \p
 /// InsertionPoint. i.e, if you have `using namespace
 /// clang::clangd::bar`, this function will return an empty string for the
 /// example above since no qualification is necessary in that case.
+/// FIXME: Also take using directives and namespace aliases inside function body
+/// into account.
 std::string getQualification(ASTContext &Context,
                              const DeclContext *DestContext,
                              SourceLocation InsertionPoint,
                              const NamedDecl *ND);
 
 /// This function uses the \p VisibleNamespaces to figure out if a shorter
 /// qualification is sufficient for \p ND, and ignores any using namespace
 /// directives. It can be useful if there's no AST for the DestContext, but some

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

for (const auto *DR : DeclRefs) {
     else if (const auto *FD = llvm::dyn_cast<FieldDecl>(D))
       Parent = FD->getParent();
     if (Parent != Class)
       return false;
   }
   return true;
 }
 
-// Rewrites body of FD to fully-qualify all of the decls inside.
-llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
+// Rewrites body of FD by re-spelling all of the names to make sure they are
+// still valid in context of Target.
+llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD,
+                                            const FunctionDecl *Target) {
   // There are three types of spellings that needs to be qualified in a function
   // body:
   // - Types:       Foo                 -> ns::Foo
   // - DeclRefExpr: ns2::foo()          -> ns1::ns2::foo();
   // - UsingDecls:
   //    using ns2::foo      -> using ns1::ns2::foo
   //    using namespace ns2 -> using namespace ns1::ns2
   //    using ns3 = ns2     -> using ns3 = ns1::ns2
   //
   // Go over all references inside a function body to generate replacements that
-  // will fully qualify those. So that body can be moved into an arbitrary file.
+  // will qualify those. So that body can be moved into an arbitrary file.
   // We perform the qualification by qualyfying the first type/decl in a
   // (un)qualified name. e.g:
   //    namespace a { namespace b { class Bar{}; void foo(); } }
   //    b::Bar x; -> a::b::Bar x;
   //    foo(); -> a::b::foo();
-  // FIXME: Instead of fully qualyfying we should try deducing visible scopes at
-  // target location and generate minimal edits.
 
+  auto *TargetContext = Target->getLexicalDeclContext();
   const SourceManager &SM = FD->getASTContext().getSourceManager();
+
   tooling::Replacements Replacements;
   bool HadErrors = false;
   findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) {
     // Since we want to qualify only the first qualifier, skip names with a
     // qualifier.
     if (Ref.Qualifier)
       return;
     // There might be no decl in dependent contexts, there's nothing much we can
findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) {
     // Skip anything from a non-namespace scope, these can be:
     // - Function or Method scopes, which means decl is local and doesn't need
     //   qualification.
     // - From Class/Struct/Union scope, which again doesn't need any qualifiers,
     //   rather the left side of it requires qualification, like:
     //   namespace a { class Bar { public: static int x; } }
     //   void foo() { Bar::x; }
     //                ~~~~~ -> we need to qualify Bar not x.
     if (!ND->getDeclContext()->isNamespace())
       return;
 
-    std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+    const std::string Qualifier = getQualification(
+        FD->getASTContext(), TargetContext, Target->getBeginLoc(), ND);
     if (auto Err = Replacements.add(
             tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
       HadErrors = true;
       elog("define inline: Failed to add quals: {0}", std::move(Err));
     }
   });
 
   if (HadErrors) {
if (!Semicolon) {
           llvm::inconvertibleErrorCode(),
           "Couldn't find semicolon for target declaration.");
     }
 
     auto ParamReplacements = renameParameters(Target, Source);
     if (!ParamReplacements)
       return ParamReplacements.takeError();
 
-    auto QualifiedBody = qualifyAllDecls(Source);
+    auto QualifiedBody = qualifyAllDecls(Source, Target);
     if (!QualifiedBody)
       return QualifiedBody.takeError();
 
     const tooling::Replacement SemicolonToFuncBody(SM, *Semicolon, 1,
                                                    *QualifiedBody);
     auto DefRange = toHalfOpenFileRange(
         SM, AST.getLangOpts(),
         SM.getExpansionRange(CharSourceRange::getCharRange(getBeginLoc(Source),

clang-tools-extra/clangd/unittests/TweakTests.cpp

std::pair<llvm::StringRef, llvm::StringRef> Cases[] = {
           #define TARGET foo
           void TARGET(){ return; }
           )cpp"},
   };
   for (const auto &Case : Cases)
     EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
+TEST_F(DefineInlineTest, DropCommonNameSpecifiers) {
+  struct {
+    llvm::StringRef Test;
+    llvm::StringRef Expected;
+  } Cases[] = {
+      {R"cpp(
+        namespace a { namespace b { void aux(); } }
+        namespace ns1 {
+          void foo();
+          namespace qq { void test(); }
+          namespace ns2 {
+            void bar();
+            namespace ns3 { void baz(); }
+          }
+        }
+
+        using namespace a;
+        using namespace a::b;
+        using namespace ns1::qq;
+        void ns1::ns2::ns3::b^az() {
+          foo();
+          bar();
+          baz();
+          ns1::ns2::ns3::baz();
+          aux();
+          test();
+        })cpp",
+       R"cpp(
+        namespace a { namespace b { void aux(); } }
+        namespace ns1 {
+          void foo();
+          namespace qq { void test(); }
+          namespace ns2 {
+            void bar();
+            namespace ns3 { void baz(){
+          foo();
+          bar();
+          baz();
+          ns1::ns2::ns3::baz();
+          a::b::aux();
+          qq::test();
+        } }
+          }
+        }
+
+        using namespace a;
+        using namespace a::b;
+        using namespace ns1::qq;
+        )cpp"},
+      {R"cpp(
+        namespace ns1 {
+          namespace qq { struct Foo { struct Bar {}; }; using B = Foo::Bar; }
+          namespace ns2 { void baz(); }
+        }
+
+        using namespace ns1::qq;
+        void ns1::ns2::b^az() { Foo f; B b; })cpp",
+       R"cpp(
+        namespace ns1 {
+          namespace qq { struct Foo { struct Bar {}; }; using B = Foo::Bar; }
+          namespace ns2 { void baz(){ qq::Foo f; qq::B b; } }
+        }
+
+        using namespace ns1::qq;
+        )cpp"},
+      {R"cpp(
+        namespace ns1 {
+          namespace qq {
+            template<class T> struct Foo { template <class U> struct Bar {}; };
+            template<class T, class U>
+            using B = typename Foo<T>::template Bar<U>;
+          }
+          namespace ns2 { void baz(); }
+        }
+
+        using namespace ns1::qq;
+        void ns1::ns2::b^az() { B<int, bool> b; })cpp",
+       R"cpp(
+        namespace ns1 {
+          namespace qq {
+            template<class T> struct Foo { template <class U> struct Bar {}; };
+            template<class T, class U>
+            using B = typename Foo<T>::template Bar<U>;
+          }
+          namespace ns2 { void baz(){ qq::B<int, bool> b; } }
+        }
+
+        using namespace ns1::qq;
+        )cpp"},
+  };
+  for (const auto &Case : Cases)
+    EXPECT_EQ(apply(Case.Test), Case.Expected) << Case.Test;
+}
+
+TEST_F(DefineInlineTest, QualifyWithUsingDirectives) {
+  llvm::StringRef Test = R"cpp(
+    namespace a {
+      void bar();
+      namespace b { struct Foo{}; void aux(); }
+      namespace c { void cux(); }
+    }
+    using namespace a;
+    using X = b::Foo;
+    void foo();
+
+    using namespace b;
+    using namespace c;
+    void ^foo() {
+      cux();
+      bar();
+      X x;
+      aux();
+      using namespace c;
+      // FIXME: The last reference to cux() in body of foo should not be
+      // qualified, since there is a using directive inside the function body.
+      cux();
+    })cpp";
+  llvm::StringRef Expected = R"cpp(
+    namespace a {
+      void bar();
+      namespace b { struct Foo{}; void aux(); }
+      namespace c { void cux(); }
+    }
+    using namespace a;
+    using X = b::Foo;
+    void foo(){
+      c::cux();
+      bar();
+      X x;
+      b::aux();
+      using namespace c;
+      // FIXME: The last reference to cux() in body of foo should not be
+      // qualified, since there is a using directive inside the function body.
+      c::cux();
+    }
+
+    using namespace b;
+    using namespace c;
+    )cpp";
+  EXPECT_EQ(apply(Test), Expected) << Test;
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang