[clang-tidy] Adding Zircon checker for std namespace

[juliehockett]

Summary

Adds a checker to warn against using the std namespace, as Zircon's kernel lib++ policy does not allow it. Written documentation of the policy is not yet published, I will add the link when it is.

[JonasToth]

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

29

please do not use auto here as the type is not clear, same below (line 32 right now)

31

The second sentence sounds a bit weird. I think you can even ellide it completely, as the first sentence does clarify quite well.

71

Please create a StringRef for the diagnostic message and reuse that.

Did you consider merging all Decl classes if you just use getNodeAs<Decl>("common_decl_name")?

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.h

1

There was a bug in the template, please add a blank after clang-tidy but keep the total length.

clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp

28

please keep lexigraphical ordering.

clang-tools-extra/docs/clang-tidy/checks/zircon-no-std-namespace.rst

1

Could you please clarify what exactly is forbidden?

Using stuff from std like std::string in normal "user" code or opening std to add stuff?

If the first thing is the case why are the variables flagged but not the functions? And adding things to namespace std is AFAIK UB with the exception of template-function specializations, so maybe that would be worth a general check?

clang-tools-extra/test/clang-tidy/zircon-no-std-namespace.cpp

33

What about using int64_t and these typedefs. Are they forbidden, too?

[juliehockett]

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

71

Yes, but getting the location right posed an issue there. The std token is not always at D->getLocation().

clang-tools-extra/docs/clang-tidy/checks/zircon-no-std-namespace.rst

1

Both. Documentation updated -- the uses in particular are to be flagged.

clang-tools-extra/test/clang-tidy/zircon-no-std-namespace.cpp

33

Yes, which is why that's tested. Is there an additional test case I'm missing regarding those?

[JonasToth]

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

53

are FunctionDecl, RecordDecl (maybe better TagDecl) covered already?

71

Ok, thats unfortunate but no problem :)

clang-tools-extra/docs/ReleaseNotes.rst

176

s/its/it's/

Could std be considered code here? Not sure, but maybe using quotes is better?

clang-tools-extra/test/clang-tidy/zircon-no-std-namespace.cpp

33

Thanks for clarifying.

I think the usual problems (templates, macros) would need some test coverage.
Especially

template <std::size_t NonTypeParm>
void MyFunc();

these cases are interesting to check.

[steveire]

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

53

Recommend extracting namespaceDecl(isStdNamespace()) to a local variable.

clang-tools-extra/test/clang-tidy/zircon-no-std-namespace.cpp

14

Are you sure it's wise to warn on every declaration inside the std namespace? Surely warning only on declarations outside it is enough.

[alexfh]

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

31

s/its/it's/, but maybe rephrase this a bit more: ", check that the target namespace of the alias is the std namespace.".

clang-tools-extra/docs/ReleaseNotes.rst

176

Actually, "its" is correct in this context ("its use" vs "it's used").

177

I guess that you're referring here to this wording from https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md: "Zircon does not use the C++ standard library". If so, it's better to avoid using the term libc++, which is the name of one particular implementation of the C++ standard library (http://libcxx.llvm.org/docs/).

[JonasToth]

clang-tools-extra/docs/ReleaseNotes.rst

176

whupsi. sry for noise.

[aaron.ballman]

I am a bit confused by what this check is trying to accomplish. It seems like this is intended to catch use of anything declared within the standard library, but that includes library support things that are needed to write useful code. For instance, it seems this means users cannot use initializer lists or type traits. I'm not even certain users can overload operator new() because that uses std::size_t as an argument.

Can you expound on the requirements in a bit more detail? Is it truly "no standard library functionality at all, including things required to be supported in freestanding implementations"?

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

58–61

Is the intention here to diagnose code like this?

#include <cstdlib>

int main() {
  abort(); // Diagnose this?
}

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

176

Please synchronize with first statement in documentation.

[juliehockett]

In D53882#1282219, @aaron.ballman wrote:

I am a bit confused by what this check is trying to accomplish. It seems like this is intended to catch use of anything declared within the standard library, but that includes library support things that are needed to write useful code. For instance, it seems this means users cannot use initializer lists or type traits. I'm not even certain users can overload operator new() because that uses std::size_t as an argument.

Can you expound on the requirements in a bit more detail? Is it truly "no standard library functionality at all, including things required to be supported in freestanding implementations"?

Yes. We're in the process of enabling the C++ standard library in the Zircon userspace code, and there is a firm requirement that no kernel code use anything in the std namespace (which declares its own implementations of things like type traits).

That said, the policy rollout has shifted a bit since I implemented this and so I'll be reworking it a bit (and also likely renaming it to zircon-kernel-no-std-namespace, so we can disable it for userspace code). Thanks all for your comments!

clang-tools-extra/clang-tidy/zircon/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyZirconModule
+  NoStdNamespaceCheck.cpp
   TemporaryObjectsCheck.cpp
   ZirconTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.h

+//===--- NoStdNamespace.h - clang-tidy---------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_NOSTDNAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_NOSTDNAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/OptionsUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// Use of the std namespace is not allowed in the Zircon kernel according to
+/// its libc++ policy.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/zircon-no-std-namespace.html
+class NoStdNamespaceCheck : public ClangTidyCheck {
+public:
+  NoStdNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_NOSTDNAMESPACECHECK_H

clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp

+//===--- NoStdNamespaceCheck.cpp - clang-tidy------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NoStdNamespaceCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+namespace {
+
+AST_MATCHER(NamespaceDecl, isStdNamespace) { return Node.isStdNamespace(); }
+
+AST_MATCHER(NamespaceAliasDecl, isAliasedToStd) {
+  if (const auto *AN = Node.getAliasedNamespace()) {
+    // If this aliases to an actual namespace, check if its std. If it doesn't,
+    // it aliases to another alias and thus shouldn't be flagged.
+    if (const auto *N = dyn_cast<NamespaceDecl>(AN))
+      return N->isStdNamespace();
+  }
+  return false;
+}
+
+AST_MATCHER_P(UsingDirectiveDecl, redirectsTo,
+              ast_matchers::internal::Matcher<NamespaceDecl>, InnerMatcher) {
+  if (const auto *N = Node.getNominatedNamespace())
+    return InnerMatcher.matches(*N, Finder, Builder);
+  return false;
+}
+
+} // namespace
+
+void NoStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // Match all values scoped to std namespace.
+  Finder->addMatcher(
+      valueDecl(hasType(decl(hasDeclContext(namespaceDecl(isStdNamespace())))))
+          .bind("stdVar"),
+      this);
+  Finder->addMatcher(namespaceDecl(isStdNamespace()).bind("stdNamespace"),
+                     this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasDeclContext(
+                                  namespaceDecl(isStdNamespace())))))
+                         .bind("stdCall"),
+                     this);
+  Finder->addMatcher(namespaceAliasDecl(isAliasedToStd()).bind("stdAlias"),
+                     this);
+  Finder->addMatcher(
+      usingDirectiveDecl(redirectsTo(namespaceDecl(isStdNamespace())))
+          .bind("stdUsing"),
+      this);
+}
+
+void NoStdNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<ValueDecl>("stdVar"))
+    diag(D->getBeginLoc(),
+         "use of the 'std' namespace is not allowed in Zircon kernel code");
+  if (const auto *D = Result.Nodes.getNodeAs<NamespaceDecl>("stdNamespace"))
+    diag(D->getLocation(),
+         "use of the 'std' namespace is not allowed in Zircon kernel code");
+  if (const auto *D = Result.Nodes.getNodeAs<CallExpr>("stdCall"))
+    diag(D->getBeginLoc(),
+         "use of the 'std' namespace is not allowed in Zircon kernel code");
+  if (const auto *D = Result.Nodes.getNodeAs<NamespaceAliasDecl>("stdAlias"))
+    diag(D->getTargetNameLoc(),
+         "use of the 'std' namespace is not allowed in Zircon kernel code");
+  if (const auto *D = Result.Nodes.getNodeAs<UsingDirectiveDecl>("stdUsing"))
+    diag(D->getLocation(),
+         "use of the 'std' namespace is not allowed in Zircon kernel code");
+}
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp

 //===--- ZirconTidyModule.cpp - clang-tidy---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "NoStdNamespaceCheck.h"
 #include "TemporaryObjectsCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace zircon {
 
 /// This module is for Zircon-specific checks.
 class ZirconModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<TemporaryObjectsCheck>(
         "zircon-temporary-objects");
+    CheckFactories.registerCheck<NoStdNamespaceCheck>(
+        "zircon-no-std-namespace");
   }
 };
 
 // Register the ZirconTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ZirconModule>
     X("zircon-module", "Adds Zircon kernel checks.");
 } // namespace zircon
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ZirconModule.
 volatile int ZirconModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`hicpp-uppercase-literal-suffix
   :doc:`readability-uppercase-literal-suffix
   <clang-tidy/checks/readability-uppercase-literal-suffix>`
   added.
 
 - The :doc:`readability-redundant-smartptr-get
   <clang-tidy/checks/readability-redundant-smartptr-get>` check does not warn
   about calls inside macros anymore by default.
 
+- New :doc:`zircon-no-std-namespace
+  <clang-tidy/checks/zircon-no-std-namespace>` check.
+
+  Warns when the `std` namespace is used, as its use is against Zircon's libc++
+  policy for the kernel.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-simplify-subscript-expr
    readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-string-compare
    readability-uniqueptr-delete-release
    readability-uppercase-literal-suffix
+   zircon-no-std-namespace
    zircon-temporary-objects

clang-tools-extra/docs/clang-tidy/checks/zircon-no-std-namespace.rst

+.. title:: clang-tidy - zircon-no-std-namespace
+
+zircon-no-std-namespace
+=======================
+
+Ensures code does not open ``namespace std`` as that violates Zircon's
+kernel libc++ policy. 
+
+Any code that uses:
+
+.. code-block:: c++
+
+ namespace std {
+  ...
+ }
+
+or 
+
+.. code-block:: c++
+
+ using namespace std;
+
+or uses a declaration from the ``std`` namespace:
+
+.. code-block:: c++
+
+ std::string foo;
+
+will be prompted with a warning.

clang-tools-extra/test/clang-tidy/zircon-no-std-namespace.cpp

+// RUN: %check_clang_tidy %s zircon-no-std-namespace %t
+
+int func() { return 1; }
+
+namespace std {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+
+typedef int std_int;
+int std_func() { return 1; }
+
+int a = 1;
+int b = func();
+std_int c;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+int d = std_func();
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+
+}
+
+// Warn on uses of quailfied std namespace.
+int i = 1;
+int j = func();
+std::std_int k;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+int l = std::std_func();
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+
+
+namespace foo {
+
+int w = 1;
+int x = func();
+std::std_int y;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+int z = std::std_func();
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+
+}
+
+// Warn on the alias declaration, and on users of it.
+namespace bar = std;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+bar::std_int q;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+
+// Warn on the using declaration, and on users of it.
+using namespace std;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use of the 'std' namespace is not allowed in Zircon kernel code
+std_int r = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of the 'std' namespace is not allowed in Zircon kernel code