closed by ac2d2652db8de9ea2b750dd2bf1232941039dffe

Addressed comments.

Rebased over AST changes.

Addressed comments.

Rebase

More refactorings.

starting a line with an opening paren is pretty weird!)

I do not think this is weird. On the contrary, this is more readable to me and separates the requires clause from the parameters list. For example this one looks so much better:

// trunk.
template <typename T>
void func() {
  auto x = []<typename U>
    requires Foo<T> && Foo<T>(T t, U u) requires BarBar<U> && BarBar<U> ||
                 BarBar<U>
  {
    return u + t;
  };
}

// patch.
template <typename T> void func() {
  auto x = []<typename U>
    requires Foo<T> && Foo<T>
  (T t, U u)
    requires BarBar<U> && BarBar<U> || BarBar<U>
  { return u + t; };
}

This does not seem to be a pressing issue as there are no other reported module/cpp20 related crashes which were fixed by this.
Also since it has no tests, it should be fine to postpone it to the next release.

Addressed comments.

Addressed comments.

In D142384#4096935, @ilya-biryukov wrote:

@usaxena95 could you give an example of the code that fails the assertion? Is it some of the tests?

assert(getDefinition()) fails about 3.6k tests.
assert(!isa<CXXRecordDecl>(this) || getDefinition()); fails a handful 23 tests in ObjC:

Failed Tests (23):
  Clang :: Analysis/CFContainers.mm
  Clang :: Analysis/blocks.m
  Clang :: Analysis/dtor-array.cpp
  Clang :: Analysis/edges-new.mm
  Clang :: Analysis/incorrect-checker-names.mm
  Clang :: Analysis/malloc.mm
  Clang :: Analysis/mig.mm
  Clang :: Analysis/retain-release.m
  Clang :: Analysis/retain-release.mm
  Clang :: Analysis/stack-capture-leak-arc.mm
  Clang :: Analysis/stack-capture-leak-no-arc.mm
  Clang :: Analysis/taint-tester.cpp
  Clang :: CodeGenObjCXX/encode.mm
  Clang-Unit :: AST/./ASTTests/0/100
  Clang-Unit :: AST/./ASTTests/1/100
  Clang-Unit :: AST/./ASTTests/14/100
  Clang-Unit :: AST/./ASTTests/16/100
  Clang-Unit :: AST/./ASTTests/76/100
  Clang-Unit :: AST/./ASTTests/77/100
  Clang-Unit :: AST/./ASTTests/78/100
  Clang-Unit :: AST/./ASTTests/79/100
  Clang-Unit :: AST/./ASTTests/98/100
  Clang-Unit :: AST/./ASTTests/99/100

bin/clang $SRC/llvm-project/clang/test/CodeGenObjCXX/encode.mm
Stacktrace: https://pastebin.pl/view/c6f505a7

Moved to RecordDecl::field_begin. Assertion is no more valid in RecordDecl.

Use definition based field access only for CXXRecordDecl.
Avoid dynamic dispatch.

Moved the use of definition where fields are accessed.
This now resolves several other C++20/Module related crashes as well.

Add message in the assertion.

Use getDefinition

I have deleted the test in https://github.com/llvm/llvm-project/commit/a3b632ab8772237ae23638f702bdceda028b2016.
It is safe to delete as this is a brittle test trying to generate an invalid requirement using very deep template instantiation. I would find a better way to test this in another forward patch.

Remove new lines.

Addressed comments.

Use ParsingDeclRAIIObject instead of ContextRAII.

Return a valid RequriesExpr instead of a ExprError in case of depenedent diagnositcs.
We would also need to store the diagnositcs in the RequiresExpr for better diagnosis.

Added more tests. Still investigating libcxx failure and tests.

Moving the changes to https://reviews.llvm.org/D140547

Adding access check related changes from https://reviews.llvm.org/D140876

In D140876#4023286, @ilya-biryukov wrote:

Should access checks should happen in the context where concept is written or where it's used?
If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted:

More tests.

Improved comment and added comment.

Perform access-dependent checks after transforming requires clause.
This is more generic and less invasive than the previous version which changed RebuildMemberExpr and also produced duplicate diagnostics.

Removed use of SubstitutionDiagnostic from NestedRequirement.

Remove unintended whitespaces and new lines.

Removed extraneous colons from diagnostic.

Addressed comments.

Added tests.
Fixed regressions in diagnositcs.

Second attempt along the lines of the last comment: https://reviews.llvm.org/D138914
PTAL.

Addressed commented. Added Release notes.
Removed infrastructure changes and deferred them to a future patch.

Moving closer to show diagnostic of entity for which SFNIAE occurred.

Coincidentally 2 of the bugs were just fixed in b9a77b56d8. I do not completely agree with the approach there though. We should be fixing the root cause instead of circumventing the assert in InsertNode by insert-if-not-present. This is why we have 44304 and 50891 still persistent.

My suggestion would be to properly handle cycles in CheckConstraintSatisfaction. This problem goes beyond cycles introduced by conversion. See #53213, #44304 and #45736 https://godbolt.org/z/hzM6f3qnW. Most of the bugs are due to an assertion failure while inserting entry into SatisfactionCache.

Not sure if this has come up already but this would still crash on

template <typename T>
concept C = requires(T a) { foo(a); };
struct D1 {
    template <C TO> D1(TO);
};
struct D2 {
    friend void foo(D1);
};

Addressed comments. Added release notes.