[clangd] Fix AddUsing tweak for out-of-line functions.

[adamcz]

Summary

We used getEnclosingNamespaceContext(), which calls getParent() rather
than getLexicalParent(), so we would end up adding the "using" line in
places that do not affect the cursor location, or just return an error
when declaration was in another file.

[hokein]

thanks.

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

2673

IIUC, before this patch, the using was added inside the above namespace foo, the code was still valid after applying the tweak.

There is no enclosing namespace for foo::fun(), so we will insert the using at the first top-level decl. It seems that we will break the code if the one::two::ff decl is defined in the main file (not the test.hpp), e.g.

// using one::two::ff will be inserted here, which is not valid
namespace one {
namespace two {
void ff();
}
}

namespace foo { void fun(); }
void foo::fun() {...}

this case was working before the patch, and now is broken after this patch. It is not a common case, probably ok for now.

[adamcz]

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

2673

You are correct that this will not work. It is not a regression, however, since it was broken before too. Prior to this patch, the using would be inserted inside the namespace foo { }, so while the using statement would compile, it wouldn't affect the out-of-line definition of the function, meaning removing the qualifier would not work.

The insertion point logic will have to change a little when we add the feature to remove all other full qualifiers for that name. I think it might be best to address these corner cases at that point, but I can also do it here if you prefer.

[hokein]

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

2673

You are correct that this will not work. It is not a regression, however, since it was broken before too. Prior to this patch, the using would be inserted inside the namespace foo { }, so while the using statement would compile, it wouldn't affect the out-of-line definition of the function, meaning removing the qualifier would not work.

hmm, that's interesting, the following code is compilable.

namespace one {
namespace two {
void ff();
}
}

namespace foo {
using one::two::ff;
void fun();
} // namespace foo
void foo::fun() { ff(); }

The insertion point logic will have to change a little when we add the feature to remove all other full qualifiers for that name. I think it might be best to address these corner cases at that point, but I can also do it here if you prefer.

agree to defer it, this is something we should keep in mind.

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

findInsertionPoint(const Tweak::Selection &Inputs,
   }
   if (LastUsingLoc.isValid()) {
     InsertionPointData Out;
     Out.Loc = LastUsingLoc;
     return Out;
   }
 
   // No relevant "using" statements. Try the nearest namespace level.
-  const auto *NS = Inputs.ASTSelection.commonAncestor()
-                       ->getDeclContext()
-                       .getEnclosingNamespaceContext();
-  if (auto *ND = dyn_cast<NamespaceDecl>(NS)) {
+  const DeclContext *ParentDeclCtx =
+      &Inputs.ASTSelection.commonAncestor()->getDeclContext();
+  while (ParentDeclCtx && !ParentDeclCtx->isFileContext()) {
+    ParentDeclCtx = ParentDeclCtx->getLexicalParent();
+  }
+  if (auto *ND = llvm::dyn_cast_or_null<NamespaceDecl>(ParentDeclCtx)) {
     auto Toks = Inputs.AST->getTokens().expandedTokens(ND->getSourceRange());
     const auto *Tok = llvm::find_if(Toks, [](const syntax::Token &Tok) {
       return Tok.kind() == tok::l_brace;
     });
     if (Tok == Toks.end() || Tok->endLocation().isInvalid()) {
       return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                      "Namespace with no {");
     }

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

CALL(one::t^wo::ff);
              R"cpp(
 #include "test.hpp"
 #define CALL(name) name()
 
 using one::two::ff;
 
 void fun() {
   CALL(ff);
+})cpp"},
+            // Parent namespace != lexical parent namespace
+            {R"cpp(
+#include "test.hpp"
+namespace foo { void fun(); }
+
+void foo::fun() {
+  one::two::f^f();
+})cpp",
+             R"cpp(
+#include "test.hpp"
+using one::two::ff;
+
+namespace foo { void fun(); }
+
+void foo::fun() {
+  ff();
 })cpp"}};
   llvm::StringMap<std::string> EditedFiles;
   for (const auto &Case : Cases) {
     for (const auto &SubCase : expandCases(Case.TestSource)) {
       ExtraFiles["test.hpp"] = R"cpp(
 namespace one {
 void oo() {}
 namespace two {