[clang] Add type check for explicit instantiation of static data members

[nomanous]

Summary

Add type check for explicit instantiation of template classes' static data members. This is a bug fix for #38205.

[nomanous]

Change some format.

[nomanous]

This comment was removed by nomanous.

[nomanous]

Ping @rsmith @chandlerc @majnemer @lvoufo

[dblaikie]

How's this compare to the similar checks for variable templates? Is there some code/checking we could share here?

[nomanous]

In D90448#2376710, @dblaikie wrote:

How's this compare to the similar checks for variable templates? Is there some code/checking we could share here?

The code of checks for variable templates is just before the additional code. But the conditions of if statement and the format of diagnostics are different, so it may be better to do the check for static member instantiation separately. And I have reused the type comparing function (with an argument different).

[dblaikie]

clang/include/clang/AST/ASTContext.h

2342–2345

I'm guessing this function is pretty widely used - so this change might be a bit invasive/may not be what all callers want?

@rsmith - this is getting a bit out of my wheelhouse and I'd rather like you to take a look when you've got a chance

clang/lib/Sema/SemaTemplate.cpp

10149–10151

The ObjC comment seems a bit out of place here - since the code here doesn't seem to mention them. Maybe skip that part here, and leave it to be described up in the hasSameType function?

clang/test/SemaCXX/template-explicit-instant-type-mismatch.cpp

15–17 ↗

(On Diff #301872)

I assume this was already tested somewhere else, perhaps? (since your change was about static member variables of class templates, and didn't touch the existing code for variable templates) So probably doesn't need re-testing here.

[nomanous]

In this update I delete some extra test code, move several comments and split the new type comparing code into a new function to make sure that the commonly used old type comparing function not affected by it.

[nomanous]

Some format fixes.

[nomanous]

Some more format fixes.

[nomanous]

Ping @rsmith @chandlerc @majnemer @aaron.ballman @dblaikie

[nomanous]

Ping @rsmith @dblaikie

[lebedev.ri]

Tests missing

[nomanous]

In D90448#2500829, @lebedev.ri wrote:

Tests missing

I mistakenly missed that file in the last commit. I'll add it soon.

[condy]

The tests failed though

[nomanous]

In D90448#2504022, @condy wrote:

The tests failed though

Which? Do you mean the unit test? It seems to be irrelevant with this revision. This revision doesn't change the code covered by that test. The failure should be caused by other commit on the trunk.

[nomanous]

Ping @rsmith @dblaikie @MaskRay @haowei @Xiangling_L @lebedev.ri

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

10146–10159

Can we combine this with the previous if? This is really checking the same thing that the hasSameType check above checked.

10151

The check of TSK seems unnecessary (and incorrect) here -- we should check the type is correct regardless of whether this is an explicit instantiation declaration or an explicit instantiation definition.

10152

Please can you point us at an example that needs this "ignore lifetime" nuance? We should check with Apple folks what they want to happen here, and we should presumably have the same rule for all explicit instantiations of templated variables -- both in the static data member case here and the variable template case a few lines above.

My expectation is that we want one of these two rules:

1. the declared lifetime should match exactly between the declaration and the explicit instantiation, or
2. there cannot be a declared lifetime on the explicit instantiation, and a lifetime on the template declaration is ignored by the check

(or a combination of these rules where we accept either option). I don't think that matches what you're doing here -- in particular, I think a wrong declared lifetime on the explicit instantiation should result in an error.

[nomanous]

clang/lib/Sema/SemaTemplate.cpp

10146–10159

It seems difficult to combine this with the previous one.

1. The two if blocks are small and both only contain two statements, but the two statements contained by the posterior one are all different from statements contained by the previous one (they use different diagnostics);
2. The conditions are different. The previous one uses PrevTemplate to make sure that it's in a variable template declaration and the posterior one uses IsStaticDataMemberInstantiation to make sure that it's in a static data member instantiation of a template class.

So, if we want to combine the two blocks, we must write two sub-if-blocks in the combined block, which, I think, has no advantage over the current code (if not more confusing).

10152

I'll do some test and give you a code snippet to trigger the lifetime mismatch when lifetime specifiers are not ignored.

[nomanous]

clang/lib/Sema/SemaTemplate.cpp

10152

With lifetime specifiers not ignored, use command

clang -cc1  -fobjc-arc -fblocks -fsyntax-only snippet.mm

to compile the obj-c++ file snippet.mm whose contents are as follows:

typedef void (^fptr)();
template<typename T> struct StaticMembers {
  static fptr f;
};

template<typename T>
fptr StaticMembers<T>::f = [] {};

template fptr StaticMembers<float>::f;

The compiler will give a type mismatch error, saying that f's definition in the template class has a type '__strong fptr' (aka 'void (^__strong)()') while its explicit instantiation has a type 'fptr' (aka 'void (^)()'), though they should essentially have the same type.

This problem only happens in explicit instantiation of a static member of block type and doesn't affect static members of other types.

@rsmith

[nomanous]

Ping @rsmith @dblaikie @MaskRay @haowei @Xiangling_L @lebedev.ri

[nomanous]

Ping @rsmith @dblaikie @MaskRay @haowei @Xiangling_L @lebedev.ri

[nomanous]

Ping @rsmith @dblaikie @MaskRay @haowei @Xiangling_L @lebedev.ri

[nomanous]

Ping @rsmith @dblaikie @MaskRay @Xiangling_L @lebedev.ri

[nomanous]

Ping @rsmith @dblaikie @MaskRay @Xiangling_L @lebedev.ri

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang/include/clang/AST/ASTContext.h

public:
 
   /// Return the canonical parameter type corresponding to the specific
   /// potentially non-canonical one.
   ///
   /// Qualifiers are stripped off, functions are turned into function
   /// pointers, and arrays decay one level into pointers.
   CanQualType getCanonicalParamType(QualType T) const;
 
   /// Determine whether the given types \p T1 and \p T2 are equivalent.
   bool hasSameType(QualType T1, QualType T2) const {
     return getCanonicalType(T1) == getCanonicalType(T2);
   }
-  bool hasSameType(const Type *T1, const Type *T2) const {
+
+  /// Determine whether the given types \p T1 and \p T2 are equivalent with
+  /// lifetime qualifiers ignored. We want this because with option -fobjc-arc,
+  /// the compiler automatically adds Objective C lifetime qualifiers for
+  /// static data members. Sometimes, this will make two types not equal while
+  /// they should be.
+  bool hasSameTypeIgnoreLifetime(QualType T1, QualType T2) const {
+    SplitQualType ST1 = getCanonicalType(T1).split();
+    SplitQualType ST2 = getCanonicalType(T2).split();
+    ST1.Quals.removeObjCLifetime();
+    ST2.Quals.removeObjCLifetime();
+    return ST1 == ST2;
+}
+
     return getCanonicalType(T1) == getCanonicalType(T2);
   }
 
   /// Return this type as a completely-unqualified array type,
   /// capturing the qualifiers in \p Quals.
   ///
   /// This will remove the minimal amount of sugaring from the types, similar
   /// to the behavior of QualType::getUnqualifiedType().
   ///
   /// \param T is the qualified type, which may be an ArrayType
   ///
   /// \param Quals will receive the full set of qualifiers that were

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_explicit_instantiation_declaration_after_definition : Error<
   "explicit instantiation declaration (with 'extern') follows explicit "
   "instantiation definition (without 'extern')">;
 def note_explicit_instantiation_definition_here : Note<
   "explicit instantiation definition is here">;
 def err_invalid_var_template_spec_type : Error<"type %2 "
   "of %select{explicit instantiation|explicit specialization|"
   "partial specialization|redeclaration}0 of %1 does not match"
   " expected type %3">;
+def err_invalid_template_static_data_member_spec_type : Error<"type %1 "
+  "of explicit instantiation of %q0 does not match expected type %2">;
 def err_mismatched_exception_spec_explicit_instantiation : Error<
   "exception specification in explicit instantiation does not match "
   "instantiated one">;
 def ext_mismatched_exception_spec_explicit_instantiation : ExtWarn<
   err_mismatched_exception_spec_explicit_instantiation.Text>,
   InGroup<MicrosoftExceptionSpec>;
 def err_explicit_instantiation_dependent : Error<
   "explicit instantiation has dependent template arguments">;

clang/lib/Sema/SemaTemplate.cpp

if (!R->isFunctionType()) {
     // C++1y [temp.explicit]p1:
     //   A [...] variable [...] template specialization can be explicitly
     //   instantiated from its template.
     if (Previous.isAmbiguous())
       return true;
 
     VarDecl *Prev = Previous.getAsSingle<VarDecl>();
     VarTemplateDecl *PrevTemplate = Previous.getAsSingle<VarTemplateDecl>();
+    bool IsStaticDataMemberInstantiation = false;
 
     if (!PrevTemplate) {
       if (!Prev || !Prev->isStaticDataMember()) {
         // We expect to see a static data member here.
         Diag(D.getIdentifierLoc(), diag::err_explicit_instantiation_not_known)
             << Name;
         for (LookupResult::iterator P = Previous.begin(), PEnd = Previous.end();
              P != PEnd; ++P)
           Diag((*P)->getLocation(), diag::note_explicit_instantiation_here);
         return true;
       }
 
       if (!Prev->getInstantiatedFromStaticDataMember()) {
         // FIXME: Check for explicit specialization?
         Diag(D.getIdentifierLoc(),
              diag::err_explicit_instantiation_data_member_not_instantiated)
             << Prev;
         Diag(Prev->getLocation(), diag::note_explicit_instantiation_here);
         // FIXME: Can we provide a note showing where this was declared?
         return true;
       }
+
+      IsStaticDataMemberInstantiation = true;
     } else {
       // Explicitly instantiate a variable template.
 
       // C++1y [dcl.spec.auto]p6:
       //   ... A program that uses auto or decltype(auto) in a context not
       //   explicitly allowed in this section is ill-formed.
       //
       // This includes auto-typed variable template instantiations.
if (PrevTemplate && Prev && !Context.hasSameType(Prev->getType(), R)) {
       Diag(T->getTypeLoc().getBeginLoc(),
            diag::err_invalid_var_template_spec_type)
           << 0 << PrevTemplate << R << Prev->getType();
       Diag(PrevTemplate->getLocation(), diag::note_template_declared_here)
           << 2 << PrevTemplate->getDeclName();
       return true;
     }
 
+    // Check the static member's type given in the explicit instantiation
+    // definition against the one in the class template. This won't happen in
+    // explicit instantiation declaration because the instantiated code won't
+    // be generated in that case.
+    if (IsStaticDataMemberInstantiation &&
+        TSK==TSK_ExplicitInstantiationDefinition && Prev &&
+        !Context.hasSameTypeIgnoreLifetime(Prev->getType(), R)) {
+      Diag(T->getTypeLoc().getBeginLoc(),
+           diag::err_invalid_template_static_data_member_spec_type)
+          << Prev << R << Prev->getType();
+      Diag(Prev->getLocation(), diag::note_template_static_data_member_def_here)
+          <<Prev;
+      return true;
+    }
+
     // FIXME: Create an ExplicitInstantiation node?
     return (Decl*) nullptr;
   }
 
   // If the declarator is a template-id, translate the parser's template
   // argument list into our AST format.
   bool HasExplicitTemplateArgs = false;
   TemplateArgumentListInfo TemplateArgs;