[Modules] Emit warning if module cannot instantiate static class member.

[sepavloff]

Summary

Instantiation of static class members can be not obvious in some cases. Using
modules can cause problems even more difficult to diagnose. PR24425 describes
one of such cases. As a way to assist a user, compiler could issue warnings if
code that is potentially erroneous. This patch implements a warning if a static
class member is used in a module, but cannot be instantiated there.

[sepavloff]

Any feedback?

Thanks,
--Serge

[silvas]

How expensive is this check?

[rsmith]

I don't really see this as being specific to modules, nor to static data members. The natural generalization of this is:

• at the end of a translation unit (including at the end of a module), warn on any implicit instantiations that are needed but could not be performed because the definition is not available

For valid code, this means we'd warn on templates/temploids which have their definition and all relevant explicit instantiations tucked away in some source file (but for which no explicit instantiation declarations are provided in the relevant header file) -- uncommon, but an idiom used by some. There is at least a syntactic solution we can suggest for people who see the warning: add the missing explicit instantiation declarations to the header file.

lib/Sema/SemaDecl.cpp

14444–14445

This is not the right way to implement the check. RecursiveASTVisitors are generally very expensive, and are poison to modules builds (because they will import all of the contents of all module files).

The right way to handle this is to produce the warning from within PerformPendingInstantiations when it's called at the end of the translation unit / module, in the cases where we want to perform an implicit instantiation, the template is not defined, and there is no preceding explicit instantiation declaration.

warning: instantiation of 'Foo<int>::s_bar' required here, but 'Foo::s_bar' is not defined
note: add an explicit instantiation declaration to suppress this warning if 'Foo<int>::s_bar' is explicitly instantiated in another translation unit

Extra credit: include a fixit hint in the note to add 'extern template char Foo<int>::s_bar;' somewhere sensible.

[sepavloff]

Updated change D16396 implements this functionality.

include/clang/Basic/DiagnosticGroups.td

 def ImplicitlyUnsignedLiteral : DiagGroup<"implicitly-unsigned-literal">;
 def MethodSignatures : DiagGroup<"method-signatures">;
 def MismatchedParameterTypes : DiagGroup<"mismatched-parameter-types">;
 def MismatchedReturnTypes : DiagGroup<"mismatched-return-types">;
 def MismatchedTags : DiagGroup<"mismatched-tags">;
 def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleConflict : DiagGroup<"module-conflict">;
+def ModuleInstantiate : DiagGroup<"module-instantiate">;
 def NewlineEOF : DiagGroup<"newline-eof">;
 def Nullability : DiagGroup<"nullability">;
 def NullabilityDeclSpec : DiagGroup<"nullability-declspec">;
 def NullableToNonNullConversion : DiagGroup<"nullable-to-nonnull-conversion">;
 def NullabilityCompleteness : DiagGroup<"nullability-completeness">;
 def NullArithmetic : DiagGroup<"null-arithmetic">;
 def NullCharacter : DiagGroup<"null-character">;
 def NullDereference : DiagGroup<"null-dereference">;

include/clang/Basic/DiagnosticSemaKinds.td

def note_module_import_in_extern_c : Note<
   "extern \"C\" language linkage specification begins here">;
 def err_module_import_not_at_top_level : Error<
   "import of module '%0' appears within %1">;
 def note_module_import_not_at_top_level : Note<"%0 begins here">;
 def err_module_self_import : Error<
   "import of module '%0' appears within same top-level module '%1'">;
 def err_module_import_in_implementation : Error<
   "@import of module '%0' in implementation of '%1'; use #import">;
+def warn_module_external_spec : Warning<
+  "specialization of the template must be provided somewhere in the application">,
+  InGroup<ModuleInstantiate>;
 }
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Instrumentation Issue" in {

lib/Sema/SemaDecl.cpp

 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CommentDiagnostic.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/HeaderSearch.h" // TODO: Sema shouldn't depend on Lex
 #include "clang/Lex/Lexer.h" // TODO: Extract static functions to fix layering.
 #include "clang/Lex/ModuleLoader.h" // TODO: Sema shouldn't depend on Lex
 #include "clang/Lex/Preprocessor.h" // Included for isCodeCompletionEnabled()
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Sema/CXXFieldCollector.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Triple.h"
 #include <algorithm>
 #include <cstring>
 #include <functional>
 using namespace clang;
 using namespace sema;
 
if (!isa<TranslationUnitDecl>(DC)) {
     S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
       << M->getFullModuleName() << DC;
     S.Diag(cast<Decl>(DC)->getLocStart(),
            diag::note_module_import_not_at_top_level)
       << DC;
   }
 }
 
+namespace {
+
+/// \brief Visitor that processes declarations that are instantiated from
+/// templates.
+///
+class InstantiationScanner : public RecursiveASTVisitor<InstantiationScanner> {
+  Sema &SemaRef;
+  llvm::SmallPtrSet<ValueDecl*, 16> seen;
+public:
+  InstantiationScanner(Sema &S) : SemaRef(S) { }
+
+  bool VisitDeclRefExpr(DeclRefExpr *E) {
+    ValueDecl *D = E->getDecl();
+    if (seen.count(D))
+      return true;
+    if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
+      if (VD->getDeclContext()->isRecord()) {
+        if (isa<ClassTemplateSpecializationDecl>(VD->getDeclContext())) {
+          // Reference to static class member instantiation
+          seen.insert(D);
+          bool HasDefinition = false;
+          for (auto *R : VD->redecls()) {
+            if (R->getDefinition()) {
+              HasDefinition = true;
+              break;
+            }
+          }
+          if (!HasDefinition)
+            SemaRef.Diag(VD->getLocation(), diag::warn_module_external_spec);
+        }
+      }
+    }
+    return true;
+  }
+};
+
+}
+
 DeclResult Sema::ActOnModuleImport(SourceLocation AtLoc, 
                                    SourceLocation ImportLoc, 
                                    ModuleIdPath Path) {
   Module *Mod =
       getModuleLoader().loadModule(ImportLoc, Path, Module::AllVisible,
                                    /*IsIncludeDirective=*/false);
   if (!Mod)
     return true;
void Sema::ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod) {
 
   if (getLangOpts().ModulesLocalVisibility)
     VisibleModulesStack.push_back(std::move(VisibleModules));
   VisibleModules.setVisible(Mod, DirectiveLoc);
 }
 
 void Sema::ActOnModuleEnd(SourceLocation DirectiveLoc, Module *Mod) {
   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext);
+  InstantiationScanner Scanner(*this);
+  Scanner.TraverseDecl(getASTContext().getTranslationUnitDecl());
 
   if (getLangOpts().ModulesLocalVisibility) {
     VisibleModules = std::move(VisibleModulesStack.back());
     VisibleModulesStack.pop_back();
     VisibleModules.setVisible(Mod, DirectiveLoc);
   }
 }
 

test/Modules/Inputs/module-inst.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I%S/Inputs/module-inst %s | FileCheck %s
+
+#include "a.h"
+// CHECK: warning: specialization of the template must be provided somewhere in the application
+// CHECK:   static char s_bar;
+
+template <typename T> char Foo<T>::s_bar;
+
+// CHECK: 1 warning generated.

test/Modules/Inputs/module-inst/a.h

+template <class T> struct Foo_0 {
+  static char s_bar_0;
+};
+template <class T> struct Foo {
+  static char s_bar; // expected-warning{{specialization of the template must be provided somewhere in the application}}
+};
+char baz() {
+  return Foo<int>::s_bar;
+}

test/Modules/Inputs/module-inst/module.modulemap

+module a {
+  header "a.h"
+}