[SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

[HerrCai0907]

Summary

PR #61326

• fix clang crash when fold expression contains a delayed typos correction.

code snippet in ActOnCXXFoldExpr

if (!LHS || !RHS) {
  Expr *Pack = LHS ? LHS : RHS;
  assert(Pack && "fold expression with neither LHS nor RHS");
  DiscardOperands();
  if (!Pack->containsUnexpandedParameterPack())
    return Diag(EllipsisLoc, diag::err_pack_expansion_without_parameter_packs)
           << Pack->getSourceRange();
}

DiscardOperands will be triggered when LHS/RHS is delayed typo correction expression.
It will output and clean all diagnose but still return a valid expression. (in else branch)
valid expression will be handled in caller function. When caller wants to output the diagnose, the diagnose in delayed typo correction expression has been consumed in ActOnCXXFoldExpr. It causes clang crash.

[erichkeane]

This needs a release note. Also, the patch message doesn't do a good job explaining what is going on here.

Also, I'm not sure this is the right answer. The purpose of CorrectDelayedTyposInExpr is, in part, to make sure we emit the diagnostics of all child expressions. It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist, which will make our compiler experience worse, particularly if it is the reason it seems that we don't see an expansion here. I cannot debug right now, but I suspect the fix to this issue is really in TransformTypoExpr.

[HerrCai0907]

The idea is that:

1. If function return an invalid ExprResult, this error should be handled in the callee
2. If function return valid Expr, the potential error should be handled in the caller, in this case, is ActOnExprStmt. If callee handles error, when caller find some error and want to output error message, the error in this expression has been processed and cause crash.

It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist,

I try this code and it will not miss other issues

c++
template <typename... T>
void foo(T &&...Params) {
  Unknown;
  foo<T>(Unknown);
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
}

build-demo/a.cpp:3:3: error: use of undeclared identifier 'Unknown'
  Unknown;
  ^
build-demo/a.cpp:4:3: error: expression contains unexpanded parameter pack 'T'
  foo<T>(Unknown);
  ^   ~
build-demo/a.cpp:5:35: error: expression is not assignable
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                                ~~^
build-demo/a.cpp:5:12: error: use of undeclared identifier 'Unknown'
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
           ^
build-demo/a.cpp:5:26: warning: division by zero is undefined [-Wdivision-by-zero]
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                         ^ ~
build-demo/a.cpp:4:10: error: use of undeclared identifier 'Unknown'
  foo<T>(Unknown);
         ^
1 warning and 5 errors generated.

[erichkeane]

In D145892#4189603, @HerrCai0907 wrote:

The idea is that:

1. If function return an invalid ExprResult, this error should be handled in the callee
2. If function return valid Expr, the potential error should be handled in the caller, in this case, is ActOnExprStmt. If callee handles error, when caller find some error and want to output error message, the error in this expression has been processed and cause crash.

It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist,

I try this code and it will not miss other issues

c++
template <typename... T>
void foo(T &&...Params) {
  Unknown;
  foo<T>(Unknown);
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
}

build-demo/a.cpp:3:3: error: use of undeclared identifier 'Unknown'
  Unknown;
  ^
build-demo/a.cpp:4:3: error: expression contains unexpanded parameter pack 'T'
  foo<T>(Unknown);
  ^   ~
build-demo/a.cpp:5:35: error: expression is not assignable
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                                ~~^
build-demo/a.cpp:5:12: error: use of undeclared identifier 'Unknown'
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
           ^
build-demo/a.cpp:5:26: warning: division by zero is undefined [-Wdivision-by-zero]
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                         ^ ~
build-demo/a.cpp:4:10: error: use of undeclared identifier 'Unknown'
  foo<T>(Unknown);
         ^
1 warning and 5 errors generated.

Hmm... What about multiple packs, where the inner pack expansion would consume the ..., but not this one? Add a few more tests for the above and ones like this as well to confirm.

[HerrCai0907]

I cannot find an example that throw error: expression contains unexpanded parameter pack 'T' in fold expression because code like (foo<T>(10 + (static_cast<U>(1))) + ...); can be unpack both T and U in same ... operator

But this code can be diagnosed correctly

template <typename... U> struct A {
  template <typename... T> void foo(T &&...Params) {
    (foo<T>(1 + (static_cast<U>(1))) + ...); // ok

    // error: expression contains unexpanded parameter pack 'T'
    foo<T>((... + static_cast<U>(1)));

    (foo<T>((... + static_cast<U>(1))) + ...); // ok
  }
};

Should I add this case into test?

[erichkeane]

In D145892#4191220, @HerrCai0907 wrote:

I cannot find an example that throw error: expression contains unexpanded parameter pack 'T' in fold expression because code like (foo<T>(10 + (static_cast<U>(1))) + ...); can be unpack both T and U in same ... operator

But this code can be diagnosed correctly

template <typename... U> struct A {
  template <typename... T> void foo(T &&...Params) {
    (foo<T>(1 + (static_cast<U>(1))) + ...); // ok

    // error: expression contains unexpanded parameter pack 'T'
    foo<T>((... + static_cast<U>(1)));

    (foo<T>((... + static_cast<U>(1))) + ...); // ok
  }
};

Should I add this case into test?

I spent some time on this and I can't come up with any counter examples either. Please add this to the test, add a release note, then this LGTM.

[HerrCai0907]

add more testcase

[HerrCai0907]

Should I add release note in clang/docs/ReleaseNotes.rst or any other way?

[erichkeane]

In D145892#4193085, @HerrCai0907 wrote:

Should I add release note in clang/docs/ReleaseNotes.rst or any other way?

Yes, that is the correct place.

[HerrCai0907]

add release note

[erichkeane]

clang/docs/ReleaseNotes.rst

173

This should go in the Bug Fixes In This Version section, right below this. Also, try to elaborate and make it more like the format of the others around there.

[erichkeane]

LGTM with 2 nits.

clang/docs/ReleaseNotes.rst

196

[erichkeane]

Looks good! Thanks! Commit when you're ready.

[HerrCai0907]

rebase to main

[HerrCai0907]

I don't have write access now. Could you kindly help me to commit it?

[erichkeane]

In D145892#4193428, @HerrCai0907 wrote:

I don't have write access now. Could you kindly help me to commit it?

Sure, just give me the name and email address you'd like it committed as.

[HerrCai0907]

HerrCai0907 and [e-mail]

Thanks a lot!

[HerrCai0907]

Hello Erichkeane, I have the access now, maybe I can push it by myself. Thanks again!

[erichkeane]

In D145892#4193522, @HerrCai0907 wrote:

Hello Erichkeane, I have the access now, maybe I can push it by myself. Thanks again!

Alright, let me know if you need any help!
-Erich

clang/docs/ReleaseNotes.rst

 - Diagnostics relating to macros on the command line of a preprocessed assembly
   file are now reported as coming from the file ``<command line>`` instead of
   ``<built-in>``.
 - Clang constexpr evaluator now provides a more concise diagnostic when calling
   function pointer that is known to be null.
 - Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` statements
   previously issued from ``-Wunreachable-code`` and ``-Wunreachable-code-fallthrough``
   by prioritizing ``-Wunreachable-code-fallthrough``.
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``
   driver mode and emit an error which suggests using ``/TC`` or ``/TP``
   ``clang-cl`` options instead.
   (`#59307 <https://github.com/llvm/llvm-project/issues/59307>`_)
 - Fix assert that fails when the expression causing the this pointer to be
   captured by a block is part of a constexpr if statement's branch and
   instantiation of the enclosing method causes the branch to be discarded.
 - Fix __VA_OPT__ implementation so that it treats the concatenation of a
   non-placemaker token and placemaker token as a non-placemaker token.
   (`#60268 <https://github.com/llvm/llvm-project/issues/60268>`_)
 - Fix crash when taking the address of a consteval lambda call operator.
   (`#57682 <https://github.com/llvm/llvm-project/issues/57682>`_)
 - Clang now support export declarations in the language linkage.
   (`#60405 <https://github.com/llvm/llvm-project/issues/60405>`_)
 - Fix aggregate initialization inside lambda constexpr.
   (`#60936 <https://github.com/llvm/llvm-project/issues/60936>`_)
 - No longer issue a false positive diagnostic about a catch handler that cannot
   be reached despite being reachable. This fixes
   `#61177 <https://github.com/llvm/llvm-project/issues/61177>`_ in anticipation
   of `CWG2699 <https://wg21.link/CWG2699>_` being accepted by WG21.
+- Fix crash when parsing fold expression containing a delayed typo correction.
+  (`#61326 <https://github.com/llvm/llvm-project/issues/61326>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Sema/SemaTemplateVariadic.cpp

ExprResult Sema::ActOnCXXFoldExpr(Scope *S, SourceLocation LParenLoc, Expr *LHS,
   }
 
   // [expr.prim.fold]p2:
   //   In a unary fold, the cast-expression shall contain an unexpanded
   //   parameter pack.
   if (!LHS || !RHS) {
     Expr *Pack = LHS ? LHS : RHS;
     assert(Pack && "fold expression with neither LHS nor RHS");
+    if (!Pack->containsUnexpandedParameterPack()) {
       DiscardOperands();
-    if (!Pack->containsUnexpandedParameterPack())
       return Diag(EllipsisLoc, diag::err_pack_expansion_without_parameter_packs)
              << Pack->getSourceRange();
     }
+  }
 
   BinaryOperatorKind Opc = ConvertTokenKindToBinaryOpcode(Operator);
 
   // Perform first-phase name lookup now.
   UnresolvedLookupExpr *ULE = nullptr;
   {
     UnresolvedSet<16> Functions;
     LookupBinOp(S, EllipsisLoc, Opc, Functions);

clang/test/SemaCXX/fold_expr_typo.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+template <typename... T>
+void foo(T &&...Params) {
+  foo<T>(Unknown); // expected-error {{expression contains unexpanded parameter pack 'T'}}\
+                      expected-error {{use of undeclared identifier 'Unknown'}}
+  ((foo<T>(Unknown)), ...); // expected-error {{use of undeclared identifier 'Unknown'}}
+}
+
+template <typename... U> struct A {
+  template <typename... T> void foo(T &&...Params) {
+    foo<T>((... + static_cast<U>(1))); // expected-error {{expression contains unexpanded parameter pack 'T'}}
+  }
+};