[clangd] Add option for disabling AddUsing tweak on some namespaces.

[adamcz]

Summary

For style guides forbid "using" declarations for namespaces like "std".
With this new config option, AddUsing can be selectively disabled on
those.

[sammccall]

Nice!

clang-tools-extra/clangd/Config.h

66

describe elements more precisely?
They are namespaces with/without leading/trailing ::, and sub-namespaces are implicitly included.

clang-tools-extra/clangd/ConfigCompile.cpp

215

this is a good place to validate and/or normalize (leading/trailing ::)

clang-tools-extra/clangd/ConfigFragment.h

167

Can we describe this positively first?

Namespaces whose members should be fully-qualified, rather than via using declarations or directives.
Affects availability of the "add using declaration" tweak.

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

197

validate it's a namespace, and then call printNamespaceScope (AST.h)?

This handles the right spelling of anonymous/inline NSes.

208

we should do this at config compile time instead I think - conceptually simpler if we store normalized data

210

slightly silly, but can we take a stringref, consume Banned, and verify that the result is empty or begins with ::?

Avoids a silly string copy, explicitly handles the sub-namespace case, and is just more typical of the string code we write.

[adamcz]

addressed review comments

clang-tools-extra/clangd/ConfigFragment.h

167

I copied the comment from the other place and added the bit about tweak. WDYT?

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

197

It's already guaranteed to be a namespace, that's checked before calling this function.

[sammccall]

Thanks!
Can we add a simple test to TweakTests (I should really split up that file) and to ConfigCompileTests?

(BTW, do you think we should merge Config{Compile,YAML}Tests?

clang-tools-extra/clangd/Config.h

70

should just be "without leading ::" now I think?

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

200

not needed - printNamespaceScope never starts with ::

201

inlining this into the if (consume_front(Banned) && consume_front(::)) might be clearer, up to you

[adamcz]

addressed review comments

clang-tools-extra/clangd/Config.h

70

What I was trying to say here is that both ::foo::bar and foo::bar are allowed, but:

using foo;
bar

is not. Basically I'm trying to explain that what we mean by fully qualified is the no-using-shortcuts, not always-start-with-::

This isn't about what's stored in the vector, but rather what this options means.

clang-tools-extra/clangd/Config.h

struct Config {
   } CompileFlags;
 
   enum class BackgroundPolicy { Build, Skip };
   /// Controls background-index behavior.
   struct {
     /// Whether this TU should be indexed.
     BackgroundPolicy Background = BackgroundPolicy::Build;
   } Index;
+
+  /// Style of the codebase.
+  struct {
+    // Namespaces that should always be fully qualified, meaning no "using"
+    // declarations, always spell out the whole name (with or without leading
+    // ::). All nested namespaces are affected as well.
+    std::vector<std::string> FullyQualifiedNamespaces;
+  } Style;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigCompile.cpp

if (F.Background) {
                                                            **F.Background)
                          .map("Build", Config::BackgroundPolicy::Build)
                          .map("Skip", Config::BackgroundPolicy::Skip)
                          .value())
         Out.Apply.push_back([Val](Config &C) { C.Index.Background = *Val; });
     }
   }
 
+  void compile(Fragment::StyleBlock &&F) {
+    if (!F.FullyQualifiedNamespaces.empty()) {
+      std::vector<std::string> FullyQualifiedNamespaces;
+      for (auto &N : F.FullyQualifiedNamespaces) {
+        // Normalize the data by dropping both leading and trailing ::
+        StringRef Namespace(*N);
+        Namespace.consume_front("::");
+        Namespace.consume_back("::");
+        FullyQualifiedNamespaces.push_back(Namespace.str());
+      }
+      Out.Apply.push_back([FullyQualifiedNamespaces(
+                              std::move(FullyQualifiedNamespaces))](Config &C) {
+        C.Style.FullyQualifiedNamespaces.insert(
+            C.Style.FullyQualifiedNamespaces.begin(),
+            FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
+      });
+    }
+  }
+
   constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
   constexpr static llvm::SourceMgr::DiagKind Warning =
       llvm::SourceMgr::DK_Warning;
   void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
             llvm::SMRange Range) {
     if (Range.isValid() && SourceMgr != nullptr)
       Diagnostic(SourceMgr->GetMessage(Range.Start, Kind, Message, Range));
     else

clang-tools-extra/clangd/ConfigFragment.h

struct Fragment {
   /// to clang's parser, such as incoming references.
   struct IndexBlock {
     /// Whether files are built in the background to produce a project index.
     /// This is checked for translation units only, not headers they include.
     /// Legal values are "Build" or "Skip".
     llvm::Optional<Located<std::string>> Background;
   };
   IndexBlock Index;
+
+  // Describes the style of the codebase, beyond formatting.
+  struct StyleBlock {
+    // Namespaces that should always be fully qualified, meaning no "using"
+    // declarations, always spell out the whole name (with or without leading
+    // ::). All nested namespaces are affected as well.
+    // Affects availability of the AddUsing tweak.
+    std::vector<Located<std::string>> FullyQualifiedNamespaces;
+  };
+  StyleBlock Style;
 };
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigYAML.cpp

public:
 
   // Tries to parse N into F, returning false if it failed and we couldn't
   // meaningfully recover (YAML syntax error, or hard semantic error).
   bool parse(Fragment &F, Node &N) {
     DictParser Dict("Config", this);
     Dict.handle("If", [&](Node &N) { parse(F.If, N); });
     Dict.handle("CompileFlags", [&](Node &N) { parse(F.CompileFlags, N); });
     Dict.handle("Index", [&](Node &N) { parse(F.Index, N); });
+    Dict.handle("Style", [&](Node &N) { parse(F.Style, N); });
     Dict.parse(N);
     return !(N.failed() || HadError);
   }
 
 private:
   void parse(Fragment::IfBlock &F, Node &N) {
     DictParser Dict("If", this);
     Dict.unrecognized(
void parse(Fragment::CompileFlagsBlock &F, Node &N) {
     });
     Dict.handle("Remove", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Remove = std::move(*Values);
     });
     Dict.parse(N);
   }
 
+  void parse(Fragment::StyleBlock &F, Node &N) {
+    DictParser Dict("Style", this);
+    Dict.handle("FullyQualifiedNamespaces", [&](Node &N) {
+      if (auto Values = scalarValues(N))
+        F.FullyQualifiedNamespaces = std::move(*Values);
+    });
+    Dict.parse(N);
+  }
+
   void parse(Fragment::IndexBlock &F, Node &N) {
     DictParser Dict("Index", this);
     Dict.handle("Background",
                 [&](Node &N) { F.Background = scalarValue(N, "Background"); });
     Dict.parse(N);
   }
 
   // Helper for parsing mapping nodes (dictionaries).

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

 //===--- AddUsing.cpp --------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 
 namespace clang {
 namespace clangd {
if (TLDs.empty()) {
     return error("Cannot find place to insert \"using\"");
   }
   InsertionPointData Out;
   Out.Loc = SM.getExpansionLoc(TLDs[0]->getBeginLoc());
   Out.Suffix = "\n\n";
   return Out;
 }
 
+bool isNamespaceForbidden(const Tweak::Selection &Inputs,
+                          const NestedNameSpecifier &Namespace) {
+  std::string NamespaceStr = printNamespaceScope(*Namespace.getAsNamespace());
+
+  for (StringRef Banned : Config::current().Style.FullyQualifiedNamespaces) {
+    StringRef PrefixMatch = NamespaceStr;
+    if (PrefixMatch.consume_front(Banned) && PrefixMatch.consume_front("::"))
+      return true;
+  }
+
+  return false;
+}
+
 bool AddUsing::prepare(const Selection &Inputs) {
   auto &SM = Inputs.AST->getSourceManager();
 
   // Do not suggest "using" in header files. That way madness lies.
   if (isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
                    Inputs.AST->getLangOpts()))
     return false;
 
   auto *Node = Inputs.ASTSelection.commonAncestor();
   if (Node == nullptr)
     return false;
 
bool AddUsing::prepare(const Selection &Inputs) {
   // namespace names. If qualifier contains a type, we could take the longest
   // namespace prefix and remove that.
   if (!QualifierToRemove.hasQualifier() ||
       !QualifierToRemove.getNestedNameSpecifier()->getAsNamespace() ||
       Name.empty()) {
     return false;
   }
 
+  if (isNamespaceForbidden(Inputs, *QualifierToRemove.getNestedNameSpecifier()))
+    return false;
+
   // Macros are difficult. We only want to offer code action when what's spelled
   // under the cursor is a namespace qualifier. If it's a macro that expands to
   // a qualifier, user would not know what code action will actually change.
   // On the other hand, if the qualifier is part of the macro argument, we
   // should still support that.
   if (SM.isMacroBodyExpansion(QualifierToRemove.getBeginLoc()) ||
       !SM.isWrittenInSameFile(QualifierToRemove.getBeginLoc(),
                               QualifierToRemove.getEndLoc())) {

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

 //===-- TweakTests.cpp ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "TweakTesting.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/Expr.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
   };
   for (const auto &Case : Cases) {
     EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
 
 TWEAK_TEST(AddUsing);
 TEST_F(AddUsingTest, Prepare) {
+  Config Cfg;
+  Cfg.Style.FullyQualifiedNamespaces.push_back("ban");
+  WithContextValue WithConfig(Config::Key, std::move(Cfg));
+
   const std::string Header = R"cpp(
 #define NS(name) one::two::name
+namespace ban { void foo() {} }
+namespace banana { void foo() {} }
 namespace one {
 void oo() {}
 template<typename TT> class tt {};
 namespace two {
 enum ee {};
 void ff() {}
 class cc {
 public:
public:
   EXPECT_UNAVAILABLE(Header + "void fun() { N^S(c^c) inst; }");
   // This used to crash. Ideally we would support this case, but for now we just
   // test that we don't crash.
   EXPECT_UNAVAILABLE(Header +
                      "template<typename TT> using foo = one::tt<T^T>;");
   // Test that we don't crash or misbehave on unnamed DeclRefExpr.
   EXPECT_UNAVAILABLE(Header +
                      "void fun() { one::two::cc() ^| one::two::cc(); }");
+  // Do not offer code action when operating on a banned namespace.
+  EXPECT_UNAVAILABLE(Header + "void fun() { ban::fo^o(); }");
+  EXPECT_UNAVAILABLE(Header + "void fun() { ::ban::fo^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { banana::fo^o(); }");
 
   // Check that we do not trigger in header files.
   FileName = "test.h";
   ExtraArgs.push_back("-xc++-header"); // .h file is treated a C by default.
   EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
   FileName = "test.hpp";
   EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
 }