Fix crash on invalid code involving late parsed inline methods

[aaron.ballman]

Summary

When parsing the following construct, we parse it as an erroneous deduction guide declaration and correctly diagnose the issues with it.

template<class> struct B;
struct A { B() noexcept(false); };

However, we then go on to finish late parsing the declaration and this expects that what we've parsed is a CXXMethodDecl. A CXXDeductionGuideDecl is not a CXXMethodDecl (it's a FunctionDecl), and so we assert on the cast.

This fixes the crash by switching from cast<> to dyn_cast<> and not setting up a "this" scope when the declaration is not a CXXMethodDecl. This fixes PR49735.

[aaron.ballman]

Ping

[aaron.ballman]

In D112616#3106436, @aaron.ballman wrote:

Ping

Ping

[erichkeane]

I'm about as confident as _I_ could be on this one, so between that and the time, I think my LGTM now carries a bit of weight.

[rjmccall]

The only alternative to this that I can see would be for the invalid code to produce more valid-seeming AST.

Are deduction guides just not normally allowed for methods?

Also, while I don't think it would eliminate the need to be defensive here, our recovery from misnamed constructors is pretty bad, and it would be great if we recognized that pattern in the parser.

[erichkeane]

In D112616#3177261, @rjmccall wrote:

The only alternative to this that I can see would be for the invalid code to produce more valid-seeming AST.

Are deduction guides just not normally allowed for methods?

Also, while I don't think it would eliminate the need to be defensive here, our recovery from misnamed constructors is pretty bad, and it would be great if we recognized that pattern in the parser.

Deduction guides apply to the record themselves... they are this odd 'translation' step to type deduction so they actually don't apply to methods in any way whatsoever. All they do is provide an additional 'lookup' when trying to construct a class-template. So they are their own special little thing...

They exist on this weird edge of 'being part of the type'... they aren't part of the class at all, but they indicate how to do some level of conversion to get the correct class-template deduction.

As far as producing a more reasonable AST node, I'm not sure what that could be? I would think we could perhaps have the error case of "no 'return type'" just simply produce NOTHING in the AST, since it has no real value without that part.

[rjmccall]

Okay. I guess the only choice, then, would be to not try to parse a template deduction guide when you're in a context where template deduction guides aren't allowed. Maybe that's not actually reasonable.

Well, whatever. I don't really have a problem with this patch; it seems reasonable and defensive. Feel free to go ahead.

I am curious why this doesn't affect friend functions, though. Do friend functions not have delayed exception specifications, or do they end up taking a different path somehow?

[aaron.ballman]

Thanks, I've commit in a1879e52e3ae9424b59747d9b837a7a87bcb996d

clang/lib/Parse/ParseCXXInlineMethods.cpp

if (CachedTokens *Toks = LM.ExceptionSpecTokens) {
     //   If a declaration declares a member function or member function
     //   template of a class X, the expression this is a prvalue of type
     //   "pointer to cv-qualifier-seq X" between the optional cv-qualifer-seq
     //   and the end of the function-definition, member-declarator, or
     //   declarator.
     CXXMethodDecl *Method;
     if (FunctionTemplateDecl *FunTmpl
           = dyn_cast<FunctionTemplateDecl>(LM.Method))
-      Method = cast<CXXMethodDecl>(FunTmpl->getTemplatedDecl());
+      Method = dyn_cast<CXXMethodDecl>(FunTmpl->getTemplatedDecl());
     else
-      Method = cast<CXXMethodDecl>(LM.Method);
+      Method = dyn_cast<CXXMethodDecl>(LM.Method);
 
-    Sema::CXXThisScopeRAII ThisScope(Actions, Method->getParent(),
-                                     Method->getMethodQualifiers(),
-                                     getLangOpts().CPlusPlus11);
+    Sema::CXXThisScopeRAII ThisScope(
+        Actions, Method ? Method->getParent() : nullptr,
+        Method ? Method->getMethodQualifiers() : Qualifiers{},
+        Method && getLangOpts().CPlusPlus11);
 
     // Parse the exception-specification.
     SourceRange SpecificationRange;
     SmallVector<ParsedType, 4> DynamicExceptions;
     SmallVector<SourceRange, 4> DynamicExceptionRanges;
     ExprResult NoexceptExpr;
     CachedTokens *ExceptionSpecTokens;
 

clang/test/Parser/cxx1z-class-template-argument-deduction.cpp

namespace within_template_arg_list {
   template<typename T> struct X { constexpr X(T v) : v(v) {} T v; };
   template<int N = X(1).v> struct Y {};
   using T = Y<>;
   using T = Y<X(1).v>;
   using T = Y<within_template_arg_list::X(1).v>;
 
   template<int ...N> struct Z { Y<X(N)...> y; };
 }
+
+namespace PR49735 {
+// Ensure that we do not crash when parsing code which looks like an invalid
+// deduction guide declaration.
+template<class> struct B; // expected-note 2{{template is declared here}}
+struct A1 {
+  B() noexcept(false); // expected-error {{deduction guide must be declared in the same scope as template 'PR49735::B'}} \
+                       // expected-error {{deduction guide declaration without trailing return type}}
+};
+
+struct A2 {
+  template <typename Ty> // expected-note {{non-deducible template parameter 'Ty'}}
+  B() noexcept(false); // expected-error {{deduction guide must be declared in the same scope as template 'PR49735::B'}} \
+                       // expected-error {{deduction guide template contains a template parameter that cannot be deduced}} \
+                       // expected-error {{deduction guide declaration without trailing return type}}
+};
+
+}