Page MenuHomePhabricator

Make evaluation of nested requirement consistent with requires expr.
ClosedPublic

Authored by usaxena95 on Nov 29 2022, 5:19 AM.

Details

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.

Diff Detail

Event Timeline

usaxena95 created this revision.Nov 29 2022, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 5:19 AM
usaxena95 requested review of this revision.Nov 29 2022, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 5:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usaxena95 edited the summary of this revision. (Show Details)Nov 29 2022, 5:21 AM
usaxena95 added reviewers: Restricted Project, ilya-biryukov.
usaxena95 added a subscriber: erichkeane.
erichkeane added inline comments.Nov 29 2022, 6:21 AM
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 marked an inline comment as done.

Added tests.
Fixed regressions in diagnositcs.

erichkeane added inline comments.Nov 29 2022, 11:26 AM
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 marked 4 inline comments as done.

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.

Removed extraneous colons from diagnostic.

Remove unintended whitespaces and new lines.

erichkeane accepted this revision.Dec 1 2022, 6:44 AM

Happy now, thanks!

clang/include/clang/AST/ExprConcepts.h
425

Ok, that works for me.

This revision is now accepted and ready to land.Dec 1 2022, 6:44 AM

Removed use of SubstitutionDiagnostic from NestedRequirement.

This revision was landed with ongoing or failed builds.Dec 19 2022, 11:25 AM
This revision was automatically updated to reflect the committed changes.
usaxena95 marked an inline comment as done.
rtrieu added a subscriber: rtrieu.Dec 21 2022, 6:36 PM
rtrieu added inline comments.
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 marked an inline comment as done.Dec 21 2022, 8:26 PM
usaxena95 added inline comments.
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 added a subscriber: shafik.Dec 21 2022, 8:49 PM
shafik added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
2339

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