Suppress errors from well-formed-testing type traits in SFINAE contexts (#135390)

There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.

(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)

Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.

So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.

Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)

Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for validity of some expression, `ForValidityCheck`
seems to be a good name.

The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.

We put the `ForValidityCheck` condition first because it's constant in
all uses and this would then allow the compiler to prune the call to
`isSFINAEContext` when true.

Fixes #132044.

1 files changed, 1 insertions, 1 deletions

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 543bd45..aef7864 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -907,7 +907,7 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
   if (MLTAL.getNumSubstitutedLevels() == 0)
     return ConstrExpr;
 
-  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+  Sema::SFINAETrap SFINAE(S);
 
   Sema::InstantiatingTemplate Inst(
       S, DeclInfo.getLocation(),