[clang-tidy] Adding Fuchsia checker for statically constructed objects

[juliehockett]

Summary

Adds a check to the Fuchsia module to warn if statically-stored objects are created, unless constructed with constexpr.

See https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md for reference.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

63

Please highlight constexpr with `, not .

docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst

7

Please highlight constexpr with `, not .

26

Please highlight constexpr with `, not .

[JonasToth]

Is the singleton allowed, that uses a static object in the instance method?

[JonasToth]

What happens for c++98? I realize that fuchsia is c++14 but we might still think about not having constexpr.
If we just assume c++11 you can do the matching only for it. (getLangOpts or similar, see other checks for it.)

[aaron.ballman]

clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp

24

The coding standard document is not very clear about what is and is not covered by this check. For instance, it seems it would also cover static int i; (because i is an object that is statically constructed).

Do you intend to cover code that has implicit static storage duration, or only if it's explicitly declared with the static storage specifier? For instance, this check currently will flag:

struct S { S(); }

static S s1; // Expected
S s2; // Is this expected?
extern S s3; // How about this?

26

So things with implicit constructors that are not constexpr should also be caught, or only with explicit non-constexpr constructors?

37

This doesn't seem like a good use of a note (it would make more sense as part of the diagnostic). Also, shouldn't the guidance be to not use the static object in the first place?

[juliehockett]

1. Narrowing check to only warn if the declaration is a global non-trivial object with explicit static storage, unless the object either has a constexpr constructor or the object has no explicit constructor.
2. Restricting check to only consider C++11 or higher.
3. Updating tests to reflect the above

[aaron.ballman]

clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp

49

Replace the colon with a semicolon.

51

You should drop this part of the string (it should be automatically added by the diagnostics engine).

docs/ReleaseNotes.rst

63

global non-trivial -> global, non-trivial

docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst

7

global non-trivial -> global, non-trivial

test/clang-tidy/fuchsia-statically-constructed-objects.cpp

27–28

This may be expected behavior, but I find it somewhat strange. This kind of rule is usually in place because you want to prohibit constructors that trigger during program initialization. What is the rule trying to accomplish by disallowing static globals that are declared static while allowing static globals that are not declared with an explicit static keyword?

If the rule is trying to prevent constructors from triggering prior to executing main(), what about constexpr constructors that must be evaluated at runtime? e.g.,

class S {
  int Val;
public:
  constexpr S(int i) : Val(100 / i) {}
  int getVal() const { return Val; }
};

static S s1(1); // Constructor call is constexpr; s1 does not need to be dynamically initialized
static S s2(0); // Constructor call is not constexpr; s2 must be dynamically initialized

extern int get_i();
static S s3(get_i()); // Constructor call is not constexpr; s3 must be dynamically initialized

[juliehockett]

Updating matcher to catch dynamically initialized constructors and fixing comments.

[aaron.ballman]

Aside from a minor formatting nit that I missed, LGTM!

clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp

52

You can elide the braces here.

clang-tidy/fuchsia/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyFuchsiaModule
   DefaultArgumentsCheck.cpp
   FuchsiaTidyModule.cpp
   OverloadedOperatorCheck.cpp
+  StaticallyConstructedObjectsCheck.cpp
   VirtualInheritanceCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

 //===--- FuchsiaTidyModule.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 "DefaultArgumentsCheck.h"
 #include "OverloadedOperatorCheck.h"
+#include "StaticallyConstructedObjectsCheck.h"
 #include "VirtualInheritanceCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace fuchsia {
 
 /// This module is for Fuchsia-specific checks.
 class FuchsiaModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "fuchsia-default-arguments");
     CheckFactories.registerCheck<OverloadedOperatorCheck>(
         "fuchsia-overloaded-operator");
+    CheckFactories.registerCheck<StaticallyConstructedObjectsCheck>(
+        "fuchsia-statically-constructed-objects");
     CheckFactories.registerCheck<VirtualInheritanceCheck>(
         "fuchsia-virtual-inheritance");
   }
 };
 // Register the FuchsiaTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<FuchsiaModule>
     X("fuchsia-module", "Adds Fuchsia platform checks.");
 } // namespace fuchsia
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the FuchsiaModule.
 volatile int FuchsiaModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.h

+//===--- StaticallyConstructedObjectsCheck.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_FUCHSIA_STATICALLY_CONSTRUCTED_OBJECTS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_STATICALLY_CONSTRUCTED_OBJECTS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+/// Constructing global, non-trivial objects with static storage is
+/// disallowed, unless the object is statically initialized with a constexpr 
+/// constructor or has no explicit constructor.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html
+class StaticallyConstructedObjectsCheck : public ClangTidyCheck {
+public:
+  StaticallyConstructedObjectsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_STATICALLY_CONSTRUCTED_OBJECTS_H

clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp

+//===--- StaticallyConstructedObjectsCheck.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 "StaticallyConstructedObjectsCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+  
+AST_MATCHER(Expr, isConstantInitializer) {
+  return Node.isConstantInitializer(Finder->getASTContext(), false);
+}
+
+AST_MATCHER(VarDecl, isGlobalStatic) {
+  return Node.getStorageDuration() == SD_Static && !Node.isLocalVarDecl();
+}
+  
+void StaticallyConstructedObjectsCheck::registerMatchers(MatchFinder *Finder) {
+  // Constructing global, non-trivial objects with static storage is
+  // disallowed, unless the object is statically initialized with a constexpr 
+  // constructor or has no explicit constructor.
+
+  // Constexpr requires C++11 or later.
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  Finder->addMatcher(
+      varDecl(allOf(
+                  // Match global, statically stored objects...
+                  isGlobalStatic(),
+                  // ... that have C++ constructors...
+                  hasDescendant(cxxConstructExpr(unless(allOf(
+                      // ... unless it is constexpr ...
+                      hasDeclaration(cxxConstructorDecl(isConstexpr())),
+                      // ... and is statically initialized.
+                      isConstantInitializer()))))))
+          .bind("decl"),
+      this);
+}
+
+void StaticallyConstructedObjectsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<VarDecl>("decl"))
+    diag(D->getLocStart(), "static objects are disallowed; if possible, use a "
+                           "constexpr constructor instead");
+}
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
-- ...
+- New `fuchsia-statically-constructed-objects
+  <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check
+
+  Warns if global, non-trivial objects with static storage are constructed, unless the 
+  object is statically initialized with a ``constexpr`` constructor or has no 
+  explicit constructor.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst

+.. title:: clang-tidy - fuchsia-statically-constructed-objects
+
+fuchsia-statically-constructed-objects
+======================================
+
+Warns if global, non-trivial objects with static storage are constructed, unless 
+the object is statically initialized with a ``constexpr`` constructor or has no 
+explicit constructor.
+
+For example:
+
+.. code-block:: c++
+
+  class A {};
+
+  class B {
+  public:
+    B(int Val) : Val(Val) {}
+  private:
+    int Val;
+  };
+
+  class C {
+  public:
+    C(int Val) : Val(Val) {}
+    constexpr C() : Val(0) {}
+
+  private:
+    int Val;
+  };
+
+  static A a;         // No warning, as there is no explicit constructor
+  static C c(0);      // No warning, as constructor is constexpr
+
+  static B b(0);      // Warning, as constructor is not constexpr
+  static C c2(0, 1);  // Warning, as constructor is not constexpr
+  
+  static int i;       // No warning, as it is trivial
+  
+  extern int get_i();
+  static C(get_i())   // Warning, as the constructor is dynamically initialized
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
    fuchsia-default-arguments
    fuchsia-overloaded-operator
+   fuchsia-statically-constructed-objects
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-objc-avoid-throwing-exception

test/clang-tidy/fuchsia-statically-constructed-objects.cpp

+// RUN: %check_clang_tidy %s fuchsia-statically-constructed-objects %t
+
+// Trivial static is fine
+static int i;
+
+class ClassWithNoCtor {};
+
+class ClassWithCtor {
+public:
+  ClassWithCtor(int Val) : Val(Val) {}
+private:
+  int Val;
+};
+
+class ClassWithConstexpr {
+public:
+  ClassWithConstexpr(int Val1, int Val2) : Val(Val1) {}
+  constexpr ClassWithConstexpr(int Val) : Val(Val) {}
+
+private:
+  int Val;
+};
+
+ClassWithNoCtor A;
+ClassWithConstexpr C(0);
+ClassWithConstexpr E(0, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  ClassWithConstexpr E(0, 1);
+ClassWithCtor G(0);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  ClassWithCtor G(0);
+
+static ClassWithNoCtor A2;
+static ClassWithConstexpr C2(0);
+static ClassWithConstexpr E2(0, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  static ClassWithConstexpr E2(0, 1);
+static ClassWithCtor G2(0);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  static ClassWithCtor G2(0);
+
+struct StructWithConstexpr { constexpr StructWithConstexpr(int Val) {} };
+struct StructWithNoCtor {};
+struct StructWithCtor { StructWithCtor(); };
+
+StructWithNoCtor SNoCtor;
+StructWithConstexpr SConstexpr(0);
+StructWithCtor SCtor;
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  StructWithCtor SCtor;
+
+static StructWithConstexpr SConstexpr2(0);
+static StructWithNoCtor SNoCtor2;
+static StructWithCtor SCtor2;
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  static StructWithCtor SCtor2;
+
+extern StructWithCtor SCtor3;
+
+class ClassWithStaticMember {
+private:
+  static StructWithNoCtor S;
+};
+
+ClassWithStaticMember Z();
+
+class S {
+  int Val;
+public:
+  constexpr S(int i) : Val(100 / i) {}
+  int getVal() const { return Val; }
+};
+
+static S s1(1);
+static S s2(0); 
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT: static S s2(0);
+
+extern int get_i();
+static S s3(get_i());
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: static objects are disallowed; if possible, use a constexpr constructor instead [fuchsia-statically-constructed-objects]
+// CHECK-MESSAGES-NEXT:  static S s3(get_i());
+
+void f() {
+  // Locally static is fine
+  static int i;
+  static ClassWithNoCtor A2;
+  static ClassWithConstexpr C2(0);
+  static ClassWithConstexpr E2(0, 1);
+  static ClassWithCtor G2(0);
+}