ids: check for unannotated static fields and extern global variables

Add scanning of variable declarations in headers. This allows us to catch instances of data symbols including static members who are external which we would not previously flag.

Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>

Author: andrurogerz

Sources/idt/idt.cc

@@ -44,20 +44,20 @@ inplace("inplace", llvm::cl::init(false),
         llvm::cl::cat(idt::category));
 
 llvm::cl::list<std::string>
-ignored_functions("ignore",
-                  llvm::cl::desc("Ignore one or more functions"),
-                  llvm::cl::value_desc("function-name[,function-name...]"),
-                  llvm::cl::CommaSeparated,
-                  llvm::cl::cat(idt::category));
+ignored_symbols("ignore",
+                llvm::cl::desc("Ignore one or more functions"),
+                llvm::cl::value_desc("function-name[,function-name...]"),
+                llvm::cl::CommaSeparated,
+                llvm::cl::cat(idt::category));
 
 template <typename Key, typename Compare, typename Allocator>
 bool contains(const std::set<Key, Compare, Allocator>& set, const Key& key) {
   return set.find(key) != set.end();
 }
 
-const std::set<std::string> &get_ignored_functions() {
+const std::set<std::string> &get_ignored_symbols() {
   static auto kIgnoredFunctions = [&]() -> std::set<std::string> {
-      return { ignored_functions.begin(), ignored_functions.end() };
+      return { ignored_symbols.begin(), ignored_symbols.end() };
     }();
 
   return kIgnoredFunctions;
@@ -97,6 +97,19 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
     return context_.getFullLoc(TD->getBeginLoc()).getExpansionLoc();
   }
 
+  template <typename Decl_>
+  bool is_in_header(const Decl_ *D) const {
+    const clang::FullSourceLoc location = get_location(D);
+  const clang::FileID id = source_manager_.getFileID(location);
+  if (const auto entry = source_manager_.getFileEntryRefForID(id)) {
+    const llvm::StringRef name = entry->getName();
+    for (const auto &extension : { ".h", ".hh", ".hpp", ".hxx" })
+      if (name.ends_with(extension))
+        return true;
+    }
+    return false;
+  }
+
 public:
   explicit visitor(clang::ASTContext &context)
       : context_(context), source_manager_(context.getSourceManager()) {}
@@ -150,7 +163,7 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
       return true;
 
     // TODO(compnerd) replace with std::set::contains in C++20
-    if (contains(get_ignored_functions(), FD->getNameAsString()))
+    if (contains(get_ignored_symbols(), FD->getNameAsString()))
       return true;
 
     clang::SourceLocation insertion_point =
@@ -163,6 +176,46 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
                                              export_macro + " ");
     return true;
   }
+
+  // VisitVarDecl will visit all variable declarations as well as static fields
+  // in classes and structs. Non-static fields are not visited by this method.
+  bool VisitVarDecl(clang::VarDecl *VD) {
+    if (VD->hasAttr<clang::DLLExportAttr>() ||
+        VD->hasAttr<clang::DLLImportAttr>())
+      return true;
+
+    if (VD->hasInit())
+      return true;
+
+    // Skip local variables.
+    if (VD->getParentFunctionOrMethod())
+      return true;
+
+    // Skip all variable declarations not in header files.
+    if (!is_in_header(VD))
+      return true;
+
+    // Skip private static members.
+    if (VD->getAccess() == clang::AccessSpecifier::AS_private)
+      return true;
+
+    // Skip all other local and global variables unless they are extern.
+    if (!VD->isStaticDataMember() &&
+        VD->getStorageClass() != clang::StorageClass::SC_Extern)
+      return true;
+
+    // TODO(compnerd) replace with std::set::contains in C++20
+    if (contains(get_ignored_symbols(), VD->getNameAsString()))
+      return true;
+
+    clang::FullSourceLoc location = get_location(VD);
+    clang::SourceLocation insertion_point = VD->getBeginLoc();
+    unexported_public_interface(location)
+        << VD
+        << clang::FixItHint::CreateInsertion(insertion_point,
+                                             export_macro + " ");
+    return true;
+  }
 };
 
 class consumer : public clang::ASTConsumer {

Tests/Variables.cc

@@ -0,0 +1,8 @@
+// RUN: %idt -export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s
+
+extern int extern_variable;
+// CHECK-NOT: Variables.cc:[[@LINE-1]]:{{.*}}
+
+extern const int extern_const_variable;
+// CHECK-NOT: Variables.cc:[[@LINE-1]]:{{.*}}
+

Tests/Variables.hh

@@ -0,0 +1,64 @@
+// RUN: %idt -ignore ignored_extern_variable -export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s
+
+class ClassWithFields {
+public:
+  int public_class_field;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+  static int public_static_class_field;
+  // CHECK: Variables.hh:[[@LINE-1]]:3: remark: unexported public interface 'public_static_class_field'
+
+  static const int public_static_const_class_field;
+  // CHECK: Variables.hh:[[@LINE-1]]:3: remark: unexported public interface 'public_static_const_class_field'
+
+  static const int public_static_const_init_class_field = 0;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+  static constexpr int public_static_constexpr_init_class_field = 0;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+private:
+  static int private_static_class_field;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+};
+
+struct StructWithFields {
+  int public_struct_field;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+  static int public_static_struct_field;
+  // CHECK: Variables.hh:[[@LINE-1]]:3: remark: unexported public interface 'public_static_struct_field'
+
+  static const int public_static_const_struct_field;
+  // CHECK: Variables.hh:[[@LINE-1]]:3: remark: unexported public interface 'public_static_const_struct_field'
+
+  static const int public_static_const_init_struct_field = 0;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+  static constexpr int public_static_constexpr_init_struct_field = 0;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+private:
+  static int private_static_struct_field;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+};
+
+extern int extern_variable;
+// CHECK: Variables.hh:[[@LINE-1]]:1: remark: unexported public interface 'extern_variable'
+
+extern const int extern_const_variable;
+// CHECK: Variables.hh:[[@LINE-1]]:1: remark: unexported public interface 'extern_const_variable'
+
+extern int ignored_extern_variable;
+// CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+int global_variable;
+// CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+
+void function() {
+  extern int extern_local_variable;
+  // CHECK: Variables.hh:[[@LINE-1]]:3: remark: unexported public interface 'extern_local_variable'
+
+  int local_variable;
+  // CHECK-NOT: Variables.hh:[[@LINE-1]]:{{.*}}
+}