[clang] add diagnose when member function contains invalid default argument

[HerrCai0907]

Summary

Fixed: https://github.com/llvm/llvm-project/issues/62122
This change pointer to add diagnose message for this code.

struct S {
    static int F(int n = 0 ? 0) {
        return 0;
    }
};

For default parameter, we should set it as unparsed even if meeting
syntax error because it should be issued in real parser time instead of
set is as invalid directly without diagnose.

[shafik]

Thank you for the fix!

While this will fix the crash I am a little concerned that I don't a diagnostic for each place we call ActOnParamDefaultArgumentError(...), there is only once place where we diagnose this the rest don't. So I wonder if this was purposeful or not.

We are not actually diagnosing the main problem which is a malformed conditional expression. I would hope that we would error out on that. That feels like where we should be applying the fix to me.

If you have malformed conditional expressions in other situation we do diagnose that. I was trying to debug why we don't catch this case but the code around parsing conditional expressions is involved.

[HerrCai0907]

While this will fix the crash I am a little concerned that I don't a diagnostic for each place we call ActOnParamDefaultArgumentError(...), there is only once place where we diagnose this the rest don't. So I wonder if this was purposeful or not.

No, before ActOnParamDefaultArgumentError we either call Diag or use Actions.CorrectDelayedTyposInExpr to diagnoise except here. And the coverage report shows that this is not covered by any test code[1]. So I guess it miss something.

We are not actually diagnosing the main problem which is a malformed conditional expression.

I also want to diagnose main problem. Should I emit diagnose in ConsumeAndStoreInitializer directly (control by a bool argument e.g. IsDiag)?

[1] https://lab.llvm.org/coverage/coverage-reports/coverage/Users/buildslave/jenkins/workspace/coverage/llvm-project/clang/lib/Parse/ParseDecl.cpp.html#L7388

[rsmith]

clang/lib/Parse/ParseDecl.cpp

7383–7384

I think we should just remove this reset call. That way, the tokens we've accumulated will get parsed when we come to later process the default argument, and we can produce the proper error at that time, such as diagnosing a missing :. That's what we do for default member initializers.

[HerrCai0907]

better fix

[HerrCai0907]

clang/lib/Parse/ParseDecl.cpp

7383–7384

Only remove reset does not work. I think it doesn't need to issue diagnose here. We can add it as a unparsedDefaultArgument and issue it when parsing.

[HerrCai0907]

ping

[rsmith]

Are there any calls to Sema::ActOnParamDefaultArgumentError left? If not, can you also remove the function?

Otherwise this LGTM, thanks!

[HerrCai0907]

Are there any calls to Sema::ActOnParamDefaultArgumentError left?

Yes, it has.

clang/docs/ReleaseNotes.rst

 - Fix defaulted equality operator so that it does not attempt to compare unnamed
   bit-fields. This fixes:
   (`#61355 <https://github.com/llvm/llvm-project/issues/61335>`_) and
   (`#61417 <https://github.com/llvm/llvm-project/issues/61417>`_)
 - Fix crash after suggesting typo correction to constexpr if condition.
   (`#61885 <https://github.com/llvm/llvm-project/issues/61885>`_)
 - Clang constexpr evaluator now treats comparison of [[gnu::weak]]-attributed
   member pointer as an invalid expression.
+- Fix crash when member function contains invalid default argument.
+  (`#62122 <https://github.com/llvm/llvm-project/issues/62122>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Parse/ParseDecl.cpp

if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
 
         // Parse the default argument
         if (DeclaratorCtx == DeclaratorContext::Member) {
           // If we're inside a class definition, cache the tokens
           // corresponding to the default argument. We'll actually parse
           // them when we see the end of the class definition.
           DefArgToks.reset(new CachedTokens);
 
           SourceLocation ArgStartLoc = NextToken().getLocation();
-          if (!ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument)) {
-            DefArgToks.reset();
-            Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
-          } else {
-            Actions.ActOnParamUnparsedDefaultArgument(Param, EqualLoc,
-                                                      ArgStartLoc);
-          }
+          ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument);
         } else {
           // Consume the '='.
           ConsumeToken();
 
           // The argument isn't actually potentially evaluated unless it is
           // used.
           EnterExpressionEvaluationContext Eval(
               Actions,

clang/test/Parser/cxx-member-initializers.cpp

class G {
   void l(int x = C<int, C<int, int>::C1>().f()) {}
 
   // This isn't, but it shouldn't crash. The diagnostics don't matter much.
   void m(int x = C<int, union int>().f()) {}
   // expected-error@-1 {{declaration of anonymous union must be a definition}}
   // expected-error@-2 {{type name requires a specifier or qualifier}}
   // expected-error@-3 {{expected '>'}}
   // expected-note@-4 {{to match this '<'}}
+
+  void n(int x = 1 ? 2) {}
+  // expected-error@-1 {{expected ':'}}
+  // expected-note@-2 {{to match this '?'}}
+  // expected-error@-3 {{expected expression}}
 };