This is an archive of the discontinued LLVM Phabricator instance.

Fix crash on invalid code involving late parsed inline methods
ClosedPublic

Authored by aaron.ballman on Oct 27 2021, 5:56 AM.

Details

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.

Diff Detail

Event Timeline

aaron.ballman requested review of this revision.Oct 27 2021, 5:56 AM
aaron.ballman created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 5:56 AM
erichkeane accepted this revision.Dec 7 2021, 8:58 AM

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.

This revision is now accepted and ready to land.Dec 7 2021, 8:58 AM

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.

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.

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?