[clang] Don't typo-fix an expression in a SFINAE context

[• Quuxplusone]

Summary

If this is a SFINAE context, then continuing to look up names (in particular, to treat a non-function as a function, and then do ADL) might too-eagerly complete a type that it's not safe to complete right now. We should just say "okay, that's a substitution failure" and not do any more work than absolutely required.

Fixes https://github.com/llvm/llvm-project/issues/52970

[• Quuxplusone]

Clang-formatted.

@mizvekov or @rsmith: ping?
I'd like to get this in before Feb 1, which is the release/14.x branch date AFAIK; because it affects workarounds in libc++ and thus makes a difference whether we will be able to remove the workarounds in version 16 or version 17.

[• Quuxplusone]

Add some more reviewers who've touched SemaCXX recently. Ping! If no objections are forthcoming, I'd like to land this on Friday morning.

[urnathan]

go for it

[• Quuxplusone]

I'm not sure if this caused
https://lab.llvm.org/buildbot/#/builders/60/builds/6350
https://lab.llvm.org/buildbot/#/builders/119/builds/7433
but I'm acting as if it did.

Anyone see how this patch might have caused a placeholder type to survive into mangling, on Windows specifically?

Assertion failed: !E->hasPlaceholderType() && "unexpected placeholder", file C:\buildbot\as-builder-2\x-aarch64\llvm-project\clang\lib\Sema\SemaType.cpp, line 9031
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: c:\\buildbot\\as-builder-2\\x-aarch64\\build\\bin\\clang++.exe -cc1 -triple aarch64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations -disable-free -clear-ast-before-backend -main-file-name deduct_F.pass.cpp -mrelocation-model static -mframe-pointer=non-leaf -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu generic -target-feature +neon -target-feature +v8a -target-abi aapcs -fallow-half-arguments-and-returns -mllvm -treat-scalable-fixed-error-as-warning -debugger-tuning=gdb -v -fcoverage-compilation-dir=C:\\buildbot\\as-builder-2\\x-aarch64\\build\\runtimes\\runtimes-bins\\libcxx\\test\\std\\utilities\\function.objects\\func.wrap\\func.wrap.func\\func.wrap.func.con -nostdinc++ -resource-dir c:\\buildbot\\as-builder-2\\x-aarch64\\build\\lib\\clang\\14.0.0 -I C:/buildbot/as-builder-2/x-aarch64/build/include\\aarch64-unknown-linux-gnu\\c++\\v1 -I C:/buildbot/as-builder-2/x-aarch64/build/include/c++/v1 -I C:/buildbot/as-builder-2/x-aarch64/build/runtimes/runtimes-bins/libcxx\\include\\c++build -D __STDC_FORMAT_MACROS -D __STDC_LIMIT_MACROS -D __STDC_CONSTANT_MACROS -I C:/buildbot/as-builder-2/x-aarch64/llvm-project/libcxx\\test/support -D _LIBCPP_DISABLE_AVAILABILITY -D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D _LIBCPP_HAS_THREAD_API_PTHREAD -D _LIBCPP_ABI_VERSION=1 -isysroot C:/buildbot/.aarch64-ubuntu -internal-isystem c:\\buildbot\\as-builder-2\\x-aarch64\\build\\lib\\clang\\14.0.0\\include -internal-isystem C:/buildbot/.aarch64-ubuntu/usr/local/include -internal-isystem C:/buildbot/.aarch64-ubuntu/usr/lib/gcc/aarch64-linux-gnu/5.4.0/../../../../aarch64-linux-gnu/include -internal-externc-isystem C:/buildbot/.aarch64-ubuntu/usr/include/aarch64-linux-gnu -internal-externc-isystem C:/buildbot/.aarch64-ubuntu/include -internal-externc-isystem C:/buildbot/.aarch64-ubuntu/usr/include -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -Wno-macro-redefined -std=c++2b -fdeprecated-macro -fdebug-compilation-dir=C:\\buildbot\\as-builder-2\\x-aarch64\\build\\runtimes\\runtimes-bins\\libcxx\\test\\std\\utilities\\function.objects\\func.wrap\\func.wrap.func\\func.wrap.func.con -ferror-limit 19 -fcoroutines-ts -fno-signed-char -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-ex1gsg43\\deduct_F-0ed146.o -x c++ C:\\buildbot\\as-builder-2\\x-aarch64\\llvm-project\\libcxx\\test\\std\\utilities\\function.objects\\func.wrap\\func.wrap.func\\func.wrap.func.con\\deduct_F.pass.cpp
1.	C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\std\utilities\function.objects\func.wrap\func.wrap.func\func.wrap.func.con\deduct_F.pass.cpp:136:37: current parser token ')'
 #0 0x00007ff65e92b105 (c:\buildbot\as-builder-2\x-aarch64\build\bin\clang++.exe+0xfdb105)
 #1 0x00007fffb07cd167 (C:\Windows\SYSTEM32\ucrtbase.DLL+0x6d167)
 #2 0x00007fffb07cdff1 (C:\Windows\SYSTEM32\ucrtbase.DLL+0x6dff1)
[...]

[ChuanqiXu]

The problem looks similar with what we met in: https://github.com/llvm/llvm-project/issues/52909#issuecomment-1021631943
Maybe both of the problem could be solved at the same time.

Anyone see how this patch might have caused a placeholder type to survive into mangling, on Windows specifically?

I didn't have a Windows and I would leave the office for some time so I couldn't see it for a time.

[• Quuxplusone]

It seems like we need tryToRecoverWithCall to always identify a diagnostic explaining the problem, so just short-circuit-returning ExprError() with no diagnostic is not acceptable. Let's try this version instead.
(This is also now based on top of D118552.)

[• Quuxplusone]

Move the regression test to clang/test/SemaTemplate/, alongside the one for D118552. Also, make it test the error message wording.

[• Quuxplusone]

Poke CI (clang-format failed).

[var-const]

@Quuxplusone Now that it's landed, do we still need __workaround_52970 in libc++? (see https://github.com/llvm/llvm-project/blob/167b623a6af26802af47f455aaa3806faaa9da9e/libcxx/include/__concepts/class_or_enum.h#L28-L30)

[• Quuxplusone]

In D117603#3292657, @var-const wrote:

@Quuxplusone Now that it's landed, do we still need __workaround_52970 in libc++? (see https://github.com/llvm/llvm-project/blob/167b623a6af26802af47f455aaa3806faaa9da9e/libcxx/include/__concepts/class_or_enum.h#L28-L30)

It's landed in Clang 14, but libc++14 still needs to support Clang 13 for another whole release (six months? a year?), so, no, we can't remove that workaround yet.
But you're correct that that's the reason I was kinda hurrying to get it fixed already before the release/14.x branch — it'll mean libc++ gets to remove the workaround 6 (12?) months earlier than otherwise.

clang/lib/Sema/Sema.cpp

static bool IsCPUDispatchCPUSpecificMultiVersion(const Expr *E) {
   return false;
 }
 
 bool Sema::tryToRecoverWithCall(ExprResult &E, const PartialDiagnostic &PD,
                                 bool ForceComplain,
                                 bool (*IsPlausibleResult)(QualType)) {
   SourceLocation Loc = E.get()->getExprLoc();
   SourceRange Range = E.get()->getSourceRange();
+  UnresolvedSet<4> Overloads;
 
+  // If this is a SFINAE context, don't try anything that might trigger ADL
+  // prematurely.
+  if (!isSFINAEContext()) {
     QualType ZeroArgCallTy;
-  UnresolvedSet<4> Overloads;
     if (tryExprAsCall(*E.get(), ZeroArgCallTy, Overloads) &&
         !ZeroArgCallTy.isNull() &&
         (!IsPlausibleResult || IsPlausibleResult(ZeroArgCallTy))) {
       // At this point, we know E is potentially callable with 0
       // arguments and that it returns something of a reasonable type,
       // so we can emit a fixit and carry on pretending that E was
       // actually a CallExpr.
       SourceLocation ParenInsertionLoc = getLocForEndOfToken(Range.getEnd());
       bool IsMV = IsCPUDispatchCPUSpecificMultiVersion(E.get());
       Diag(Loc, PD) << /*zero-arg*/ 1 << IsMV << Range
                     << (IsCallableWithAppend(E.get())
-                          ? FixItHint::CreateInsertion(ParenInsertionLoc, "()")
+                            ? FixItHint::CreateInsertion(ParenInsertionLoc,
+                                                         "()")
                             : FixItHint());
       if (!IsMV)
         notePlausibleOverloads(*this, Loc, Overloads, IsPlausibleResult);
 
       // FIXME: Try this before emitting the fixit, and suppress diagnostics
       // while doing so.
       E = BuildCallExpr(nullptr, E.get(), Range.getEnd(), None,
                         Range.getEnd().getLocWithOffset(1));
       return true;
     }
-
+  }
   if (!ForceComplain) return false;
 
   bool IsMV = IsCPUDispatchCPUSpecificMultiVersion(E.get());
   Diag(Loc, PD) << /*not zero-arg*/ 0 << IsMV << Range;
   if (!IsMV)
     notePlausibleOverloads(*this, Loc, Overloads, IsPlausibleResult);
   E = ExprError();
   return true;

clang/lib/Sema/SemaExprMember.cpp

fail:
   //     the appropriate pointer type
   if (const PointerType *Ptr = BaseType->getAs<PointerType>()) {
     if (!IsArrow && Ptr->getPointeeType()->isRecordType() &&
         MemberName.getNameKind() != DeclarationName::CXXDestructorName) {
       S.Diag(OpLoc, diag::err_typecheck_member_reference_suggestion)
           << BaseType << int(IsArrow) << BaseExpr.get()->getSourceRange()
           << FixItHint::CreateReplacement(OpLoc, "->");
 
+      if (S.isSFINAEContext())
+        return ExprError();
+
       // Recurse as an -> access.
       IsArrow = true;
       return LookupMemberExpr(S, R, BaseExpr, IsArrow, OpLoc, SS,
                               ObjCImpDecl, HasTemplateArgs, TemplateKWLoc);
     }
   }
 
   // If the user is trying to apply -> or . to a function name, it's probably

clang/test/SemaTemplate/pr52970.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify=cxx20 %s
+// expected-no-diagnostics
+
+struct Incomplete;
+template <class T> struct Holder { T t; };
+
+namespace DotFollowingFunctionName {
+struct Good {
+  struct Nested {
+    int b;
+  } a;
+};
+
+struct Bad {
+  Holder<Incomplete> a();
+};
+
+template <class T>
+constexpr auto f(T t) -> decltype((t.a.b, true)) { return true; }
+constexpr bool f(...) { return false; }
+
+static_assert(DotFollowingFunctionName::f(Good{}), "");
+static_assert(!DotFollowingFunctionName::f(Bad{}), "");
+
+#if __cplusplus >= 202002L
+template <class T>
+concept C = requires(T t) { t.a.b; };
+  // cxx20-note@-1 {{because 't.a.b' would be invalid: reference to non-static member function must be called}}
+
+static_assert(C<Good>);
+static_assert(!C<Bad>);
+static_assert(C<Bad>); // cxx20-error {{static_assert failed}}
+  // cxx20-note@-1 {{because 'DotFollowingFunctionName::Bad' does not satisfy 'C'}}
+#endif
+} // namespace DotFollowingFunctionName
+
+namespace DotFollowingPointer {
+struct Good {
+  int begin();
+};
+using Bad = Holder<Incomplete> *;
+
+template <class T>
+constexpr auto f(T t) -> decltype((t.begin(), true)) { return true; }
+constexpr bool f(...) { return false; }
+
+static_assert(DotFollowingPointer::f(Good{}), "");
+static_assert(!DotFollowingPointer::f(Bad{}), "");
+
+#if __cplusplus >= 202002L
+template <class T>
+concept C = requires(T t) { t.begin(); };
+  // cxx20-note@-1 {{because 't.begin()' would be invalid: member reference type 'Holder<Incomplete> *' is a pointer}}
+
+static_assert(C<Good>);
+static_assert(!C<Bad>);
+static_assert(C<Bad>); // cxx20-error {{static_assert failed}}
+  // cxx20-note@-1 {{because 'DotFollowingPointer::Bad' (aka 'Holder<Incomplete> *') does not satisfy 'C'}}
+#endif
+} // namespace DotFollowingPointer