Fix PR19169:Crash on invalid attempting to specialize a template method as a template variable

[karthikthecool]

Summary

Hi All,
A small patch to fix PR19169. In ActOnVarTemplateSpecialization the getAsTemplateDecl() function might return null if template name refers to a set of function templates. Handle the same by using dynamic_cast_or_null instead of dynamic_cast which will crash if the pointer being casted is null.

Please let me know if i can go ahead and commit the same?

Thanks and Regards
Karthik Bhat

[karthikthecool]

Hi Nick,
Modified the diagnostics in this case to make it more intutive as per your comments. Does this seems reasonable?
Thanks and Regards
Karthik Bhat

[karthikthecool]

Hi Nick, Richard,
A gintle ping. Can i go ahead and commit this patch?
Thanks
Karthik Bhat

[karthikthecool]

Incorrectly added llvm-commits in cc. Move patch review to cfe commit mailing list.

Thanks
Karthik Bhat

[karthikthecool]

ping..

Thanks
Karthik Bhat

[karthikthecool]

Gentle ping..
Thanks

[rsmith]

Sorry for the delay! The crash fix LGTM, but the diagnostic improvement needs refinement.

Ideally, I'd like for us to also give a note pointing at the template declaration in the case where we have a TemplateDecl that's not a VarTemplateDecl, and to give notes pointing at each function template in the case where we have a function template rather than a variable template, but I'm happy for this patch to be committed without that.

(Also, it'd be awesome if we could give a fixit adding the parens to the template declaration in either the case where there's only one element in the overload set or where we can figure out which one the user meant somehow.)

include/clang/Basic/DiagnosticSemaKinds.td

3318

The partial specialization case here makes no sense; you can't partially specialize a function template, so we know the user didn't mean that.

3319

"did you mean" diagnostics usually have a ? at the end.

lib/Sema/SemaTemplate.cpp

2429–2431

We should give this diagnostic in the case where there's only one declaration that's a function as well as in the case where there's an overload set of them.

[karthikthecool]

Hi Richard,
Thanks a lot for your valuable inputs. Modified the diagnostic as per comments. I will try to address adding fixit for these cases in later revisions.
For now does this look good to commit?
Thanks again for spending your time on this i really appreciate your help.
Regards
Karthik Bhat

[karthikthecool]

gentle ping..

Thanks
Karthik Bhat

[karthikthecool]

Hi Richard,
a gentle ping.. would like to know if this patch is good to commit.
Thanks
Karthik Bhat

[rsmith]

lib/Sema/SemaTemplate.cpp

2432

Please fold the two cases together:

NamedDecl *FnTemplate;
if (auto *OTS = Name.getAsOverloadedTemplate())
  FnTemplate = *OTS->begin();
else
  FnTemplate = dyn_cast_or_null<FunctionTemplateDecl>(Name.getAsTemplateDecl());
if (FnTemplate)
  // ...

2433–2435

This check seems unnecessary; if we have an overloaded template name then by definition there are two or more function templates in the storage.

[karthikthecool]

Thanks Richard for the review. Updating the patch as per review comments.

[rsmith]

LGTM, thanks!

[karthikthecool]

Thanks Richard for the review. Committed as r208313.

include/clang/Basic/DiagnosticSemaKinds.td

 
 // C++ Variable Template Partial Specialization
 def err_var_partial_spec_redeclared : Error<
   "variable template partial specialization %0 cannot be redefined">;
 def note_var_prev_partial_spec_here : Note<
   "previous declaration of variable template partial specialization is here">;
 def err_var_spec_no_template : Error<
   "no variable template matches%select{| partial}0 specialization">;
+def err_var_spec_no_template_but_method : Error<
+  "no variable template matches specialization; "
+  "did you mean to use %0 as function template instead?">;
   
 // C++ Function template specializations
 def err_function_template_spec_no_match : Error<
     "no function template matches function template specialization %0">;
 def err_function_template_spec_ambiguous : Error<
     "function template specialization %0 ambiguously refers to more than one "
     "function template; explicitly specify%select{| additional}1 template "
     "arguments to identify a particular function template">;

lib/Sema/SemaTemplate.cpp

DeclResult Sema::ActOnVarTemplateSpecialization(
   SourceLocation TemplateNameLoc = D.getIdentifierLoc();
   SourceLocation LAngleLoc = TemplateId->LAngleLoc;
   SourceLocation RAngleLoc = TemplateId->RAngleLoc;
 
   TemplateName Name = TemplateId->Template.get();
 
   // The template-id must name a variable template.
   VarTemplateDecl *VarTemplate =
-      dyn_cast<VarTemplateDecl>(Name.getAsTemplateDecl());
-  if (!VarTemplate)
+      dyn_cast_or_null<VarTemplateDecl>(Name.getAsTemplateDecl());
+  if (!VarTemplate) {
+    NamedDecl *FnTemplate;
+    if (auto *OTS = Name.getAsOverloadedTemplate())
+      FnTemplate = *OTS->begin();
+    else
+      FnTemplate = dyn_cast_or_null<FunctionTemplateDecl>(Name.getAsTemplateDecl());
+    if (FnTemplate)
+      return Diag(D.getIdentifierLoc(), diag::err_var_spec_no_template_but_method)
+               << FnTemplate->getDeclName();
     return Diag(D.getIdentifierLoc(), diag::err_var_spec_no_template)
              << IsPartialSpecialization;
+  }
 
   // Check for unexpanded parameter packs in any of the template arguments.
   for (unsigned I = 0, N = TemplateArgs.size(); I != N; ++I)
     if (DiagnoseUnexpandedParameterPack(TemplateArgs[I],
                                         UPPC_PartialSpecialization))
       return true;
 
   // Check that the template argument list is well-formed for this

test/SemaCXX/cxx1y-variable-templates_top_level.cpp

 }
 
 namespace PR19152 {
 #ifndef PRECXX11
   template<typename T> const auto x = 1;
   static_assert(x<int> == 1, "");
 #endif
 }
+
+namespace PR19169 {
+  template <typename T> int* f();
+  template <typename T> void f();
+  template<> int f<double>; // expected-error {{no variable template matches specialization; did you mean to use 'f' as function template instead?}}
+  
+  template <typename T> void g();
+  template<> int g<double>; // expected-error {{no variable template matches specialization; did you mean to use 'g' as function template instead?}}
+}
+