Make evaluation of nested requirement consistent with requires expr.

[usaxena95]

Summary

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

template<class T>  concept True = true;

template <class T>
concept C1 = requires (T) {
   requires True<typename T::value> || True<T>;
};

template <class T>
constexpr bool foo()
requires True<typename T::value> || True<T> {
    return true;
}
static_assert(C1<double>); // Previously failed due to SFINAE error
static_assert(foo<int>()); // but this works fine.

The issue here is the discrepancy between how a nested requirement is evaluated Vs how a non-nested requirement is evaluated.

This patch makes constraint checking consistent for nested requirement
and trailing requires expressions by using the same evaluator.

[erichkeane]

clang/include/clang/AST/ExprConcepts.h

425

I'm a little concerned that we're changing to a situation where the constraint in a nested-requirement can be null like this. The other cases it can be null we consider it a 'substitution failure' (see the 1st constructor).

It seems to me that we perhaps need a different state here for that.

clang/test/SemaTemplate/instantiate-requires-expr.cpp

27 ↗

(On Diff #478539)

This is wrong here, we shouldn't be making our diagnostics incorrect like this.

[usaxena95]

Added tests.
Fixed regressions in diagnositcs.

[erichkeane]

clang/include/clang/AST/ExprConcepts.h

425

I still have this concern, we need to make sure that NestedRequirement is written to consider this type of failure, it likely needs a separate constructor that contains information about the failure. We probably want at least some sort of stored diagnostic here, sot hat we dont lose the output we have below.

clang/test/CXX/expr/expr.prim/expr.prim.req/nested-requirement.cpp

23

Losing the extra context here is unfortunate as well... why do we lose this info?

93

Can you re-arrange this test with the #NAME type bookmarks so that the instantiation notes/etc are all arranged with the error they are associated with?

[usaxena95]

Addressed comments.

clang/include/clang/AST/ExprConcepts.h

425

NestedRequirement will no more capture a substitution failure. I am planning to remove all references to SubstitutionDiagnostics for it. I have temporarily added unreachable tags to the references of SubstitutionDiagnostics.

If this looks fine to you then I would move forward with refactoring it to remove all support for SubstitutionDiagnostics in NestedRequirement. WDYT ?

clang/lib/Sema/SemaConcept.cpp

183–184

For context: This is not necessarily invalid but more importantly the BinaryOp cannot be constant evaluated as it would have dependent RHS.

clang/test/CXX/expr/expr.prim/expr.prim.req/nested-requirement.cpp

23

This is now restored.

[usaxena95]

Removed extraneous colons from diagnostic.

[usaxena95]

Remove unintended whitespaces and new lines.

[erichkeane]

Happy now, thanks!

clang/include/clang/AST/ExprConcepts.h

425

Ok, that works for me.

[usaxena95]

Removed use of SubstitutionDiagnostic from NestedRequirement.

[rtrieu]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2339

I have found a crash here when it access vector Result without checking the size first, leading to out of bounds memory access. CReduce gave the following testcase:

template <class a, a> struct b;
template <bool c> using d = b<bool, c>;
template <class a, class e> using f = d<__is_same(a, e)>;
template <class a, class e>
concept g = f<a, e>::h;
template <class a, class e>
concept i = g<e, a>;
template <typename> class j {
  template <typename k>
  requires requires { requires i<j, k>; }
  j();
};
template <> j();

clang reduce.ii --std=c++20

assertion failed at llvm/include/llvm/ADT/SmallVector.h:294 in reference llvm::SmallVectorTemplateCommon<clang::Expr *>::operator[](size_type) [T = clang::Expr *]: idx < size()
...
...
(anonymous namespace)::TemplateInstantiator::TransformNestedRequirement(clang::concepts::NestedRequirement*) clang/lib/Sema/SemaTemplateInstantiate.cpp:2339:25
...
...

[usaxena95]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2339

Thanks for noticing and sorry for the trouble. I have fixed this forward in https://github.com/llvm/llvm-project/commit/8c0aa53b07caa604d58a0d83dc571d8fcb004972.

[shafik]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2339

Can you add a test case as well so if we regress we will catch it, thank you!