Deferred Concept Instantiation Implementation

[erichkeane]

Summary

As reported here: https://github.com/llvm/llvm-project/issues/44178

This is an attempt to get a functional version of the delayed concept instantiation
implementation for functions. Currently this passes all lit tests, plus a few
extras I thought were valuable when I implemented this.

Note there are two places in particular I'm not sure of the fix, and would love
any reviews on my approach if at all possible (plus any suggested additional testing
that folks can suggest).

[ChuanqiXu]

I am interested in this one. But it is absolutely not easy to understand...

clang/include/clang/Sema/Sema.h

7040–7045

Now the comment is not precise.

7062

I think this one need comment too. What's the difference with the above one?

clang/lib/Sema/SemaConcept.cpp

167

We should delete this one.

176

ditto

427–433

Might you elaborate more on this. I am not sure about the intention.

535

Would you mind to elaborate for the issue "function constraints for comparison of constraints to work" more? Maybe it is said in previous messages, but the history is hard to follow...

[erichkeane]

Respond to @ChuanqiXu and fix a few comments he suggested.

[erichkeane]

clang/include/clang/Sema/Sema.h

7062

Thanks! I'll comment on that. The difference is the 'out' parameter of 'ConvertedConstraints', but I'll put details in the comment.

clang/lib/Sema/SemaConcept.cpp

535

Yep, this one is difficult :/ Basically, when we instantiate the constraints at 'checking' time, if the function is NOT a template, we call CheckFunctionConstraints. When we go to check the 'subsumption' type things with a fully instantiated version, they fail if they are dependent (as it is expected that way). See the setTrailingRequiresClause here.

HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather than the 'stored' version of the constraint on the current declaration. I tried to do something similar here for those cases, but 1: it had no effect seemingly, and 2: it ended up getting complicated in many cases (as figuring out the relationship between the constraints in the two nodes was difficult/near impossible).

I opted to not do it, and I don't THINK it needs to happen over there, but I wanted to point out that I was skipping it in case reviewers had a better idea.

[ChuanqiXu]

BTW, it looks like the patch needs to rebase with main so that other people could play it self if interested.

clang/lib/Sema/SemaConcept.cpp

535

Let me ask some questions to get in consensus for the problem:

Basically, when we instantiate the constraints at 'checking' time, if the function is NOT a template, we call CheckFunctionConstraints.

I think the function who contains a trailing require clause should be template one. Do you want to say independent ? Or do you refer the following case?

C++
template<typename T> struct A {
  static void f(int) requires xxxx;
};

And the related question would be what's the cases that CheckFunctionConstraints would be called and what's the cases that CheckinstantiatedFunctionTemplateConstraints would be called?

When we go to check the 'subsumption' type things with a fully instantiated version, they fail if they are dependent (as it is expected that way).

If it is fully instantiated, how could it be dependent?

HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather than the 'stored' version of the constraint on the current declaration.

1. What is we? I mean what's the function would pick up the constraints from the primary template.
2. Does the stored version of the constraint means the trailing require clause of FD? Would it be the original one or Converted[0].

[erichkeane]

Do a rebase, only conflict was with ReleaseNotes.rst.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

535

Or do you refer the following case?

Yeah, thats exactly the case I am talking about. A case where the function itself isn't a template, but is dependent.

And the related question would be what's the cases that CheckFunctionConstraints would be called and what's the cases that CheckinstantiatedFunctionTemplateConstraints would be called?

The former when either the function is not dependent at all, OR it is dependent-but-fully-instantiated (like in the example you gave). All other cases (where the template is NOT fully instantiated) calls the other function.

If it is fully instantiated, how could it be dependent?

By "Fully Instantiated" I mean "everything except the constraints (and technically, some noexcept)", since we are deferring constraint checking until that point. A bunch of the changes in this commit STOP us from instantiating the constraint except when checking, since that is the point of the patch! SO, the constriant is still 'dependent' (like in your example above).

What is we? I mean what's the function would pick up the constraints from the primary template.

Royal 'we', or clang. The function is getAssociatedConstraints.

Does the stored version of the constraint means the trailing require clause of FD? Would it be the original one or Converted[0].

The 'stored' version (that is, not on the primary template) is the one that we partially instantiated when going through earlier instantiations. In the case that it was a template, we seem to always ignore these instantiated versions. IN the case where it is NOT a template, we use that for checking (which is why Converted[0] needs replacing here).

[ChuanqiXu]

clang/lib/Sema/SemaConcept.cpp

535

I see roughly. The implementation looks a little bit odd to me.. But I don't find better solutions..

clang/lib/Sema/SemaTemplateInstantiate.cpp

68–70

Would you elaborate more for LookBeyondLambda and IncludeContainingStructArgs? It confuses me since I couldn't find Lambda or Struct from the context of use point.

[erichkeane]

clang/lib/Sema/SemaTemplateInstantiate.cpp

68–70

Sure!

So this function is typically used for 'getting the template instantiation args' of the current Declaration (D) for a variety of reasons. In all of those cases previously, it would 'stop' looking when it hit a lambda generic call operator (see line 157). This would block our ability to get the full instantiation tree.

Similarly, it would stop at a containing class-template (as most instantiations are done against the parent class template). Unfortunately this is sufficient, so the IncludeContainingStructArgs (see line 185) ALSO includes those arguments, as they are necessary (since they haven't been instantiated in the constraint yet).

[ChuanqiXu]

I've read the whole patch. It looks good to me roughly except some style issues and the 2 places you marked as Attn Reviewers. Let's try to fix them one by one.

clang/lib/Sema/SemaTemplate.cpp

4713

I would feel better if we could write:

CheckConstraintSatisfaction(
          NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
          SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
                      TemplateArgs->getRAngleLoc()),
          Satisfaction)

But it looks unimplementable.

5572–5573

7477–7492

I've spent some time to playing it myself to figure it out. And I found that we could fix this cleaner we adopt above suggestions. The problem here is that the instantiation is started half way. But the conversion for the constraint have been deferred. So here is the problem. I guess there are other similar problems. But let's fix them after we met them actually.

clang/lib/Sema/SemaTemplateInstantiate.cpp

68–70

I got it. It might be necessary to edit the comment too.

[erichkeane]

clang/lib/Sema/SemaTemplate.cpp

4713

I'm not sure I get the suggestion? Why would you want to put the MultiLevelTemplateArgumentList in curleys?

7477–7492

Ah, neat! Thanks for this. Done.

[erichkeane]

Make suggestions from @ChuanqiXu

[ChuanqiXu]

LGTM basically. Please wait for 1~2 weeks to land this in case there are other comments.

clang/lib/Sema/SemaConcept.cpp

534–535

We need to edit the Attn Reviewers to TODO before landing. The content need to be rewording too. Your English is much better than me. So no concrete suggestion here : )

clang/lib/Sema/SemaTemplate.cpp

4713

I just feel like the style is more cleaner. But I found the constructor might not allow us to do so... So this one might not be a suggestion.

[erichkeane]

rebase for CI + comment change requested.

[erichkeane]

I went to commit this, and found that a recently lit test now fails with a crash during constraint instantiation! I'll be looking into that. The example reduces to:

template<typename T>
constexpr bool constraint = true;
 
template < typename U>
void dependent(U&& u) {
  []() requires constraint<decltype(u)> {}();
}
 
void test_dependent() {
  int v   = 0;
  dependent(v);
}

clang/lib/Sema/SemaTemplate.cpp

4713

Ah, you mean to pass 'converted' directly in, so:

CheckConstraintSatisfaction(
          NamedConcept, {NamedConcept->getConstraintExpr()}, {Converted},
          SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
                      TemplateArgs->getRAngleLoc()),
          Satisfaction)

(notice 'Converted' instead of MLTAL). I agree with you, that WOULD be nicer, but unfortunately I think the constructor for that type was created explicitly to avoid us doing that :)

[cor3ntin]

In D119544#3458848, @erichkeane wrote:

I went to commit this, and found that a recently lit test now fails with a crash during constraint instantiation! I'll be looking into that. The example reduces to:

template<typename T>
constexpr bool constraint = true;
 
template < typename U>
void dependent(U&& u) {
  []() requires constraint<decltype(u)> {}();
}
 
void test_dependent() {
  int v   = 0;
  dependent(v);
}

I suspect this may be related to my recent lambda changes - not 100% certain but I'm looking into it too

[erichkeane]

In D119544#3458966, @cor3ntin wrote:

In D119544#3458848, @erichkeane wrote:

I went to commit this, and found that a recently lit test now fails with a crash during constraint instantiation! I'll be looking into that. The example reduces to:

template<typename T>
constexpr bool constraint = true;
 
template < typename U>
void dependent(U&& u) {
  []() requires constraint<decltype(u)> {}();
}
 
void test_dependent() {
  int v   = 0;
  dependent(v);
}

I suspect this may be related to my recent lambda changes - not 100% certain but I'm looking into it too

I've got it down to the SetupConstraintCheckingTemplateArgumentsAndScope that I added. This is a case where the function is a template instantiation but does NOT have a primary template, so I have to figure out what THAT means/what I should be using instead.

[tahonermann]

This is a case where the function is a template instantiation but does NOT have a primary template, so I have to figure out what THAT means/what I should be using instead.

I think that is not supposed to be possible. For example, FunctionDecl::isFunctionTemplateSpecialization() will return false if there is no primary template.

[erichkeane]

In D119544#3459045, @tahonermann wrote:

This is a case where the function is a template instantiation but does NOT have a primary template, so I have to figure out what THAT means/what I should be using instead.

I think that is not supposed to be possible. For example, FunctionDecl::isFunctionTemplateSpecialization() will return false if there is no primary template.

I wouldn't think so either? In this case the problem is that 'u' is not in the re-manufactured scope, I think there is a bit of work to make sure that lambdas ALSO get the scope of their containing function, if they are in a functiondecl.

[tahonermann]

I wouldn't think so either? In this case the problem is that 'u' is not in the re-manufactured scope, I think there is a bit of work to make sure that lambdas ALSO get the scope of their containing function, if they are in a functiondecl.

I wouldn't expect lambdas to require special handling; I think they should be handled via their transformation to a member function of a dependent local class or dependent member class.

[erichkeane]

In D119544#3459169, @tahonermann wrote:

I wouldn't think so either? In this case the problem is that 'u' is not in the re-manufactured scope, I think there is a bit of work to make sure that lambdas ALSO get the scope of their containing function, if they are in a functiondecl.

I wouldn't expect lambdas to require special handling; I think they should be handled via their transformation to a member function of a dependent local class or dependent member class.

Yeah, its not lambda-specific, thats just the example here. The example you gave offline shows that this is the case with a dependent local class as well.

[erichkeane]

FWIW, I am about 95% sure I have a hold on this, plus a bunch of other cases I came up with. I likely won't get a patch up for review today (in the next hour!) unless something miraculous happens, but hopefully I'll have something tomorrow for folks to take another look at.

[erichkeane]

Fixed the issue that Corentin's test came up with, added a number of others.

The problem is that we didn't properly 'collect' the parameters in cases where we referred to a parameter of a function that contains the current function, so this patch did a bit of refactoring to make sure we did so recursively.

Additionally, we found an additional case (the non-functor case) where we didn't have a way to get the primary template, so we added a new entry in the FunctionDecl's PointerUnion to handle these cases.

Please review! I'd love to get this into trunk with time to bake before the branch.

[ChuanqiXu]

clang/include/clang/AST/Decl.h

1891

hmmm, what does this literally mean? In my understanding, it should be:

A non template function which is in a dependent scope.

I am just wondering if this is covered by TK_NonTemplate.

1942

an inner-declared function in another function template

Does this refer to local lambdas or functions in local classes of a template function only? If yes, I recommend to reword this. Since I understand it by the review comment instead of the comments itself.

2691–2692

I can't read the original comment... I am not sure if it is my problem but I think it may be better to reword it.

clang/include/clang/AST/DeclBase.h

909–910

clang/lib/AST/Decl.cpp

3787

Do I understand incorrectly? Must it be a member function?

clang/lib/AST/DeclBase.cpp

299

From the function name, I image it should be DC = DC->getLexicalParent. Is it incorrect?

clang/lib/Sema/SemaConcept.cpp

480–488

Don't use else-after-return: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.

And I am wondering if we could hit these 2 checks only if the FD is TK_DependentNonTemplate. If yes, I think we could move these two checks in the above block. In this way, the code could be simplified further.

clang/lib/Sema/SemaTemplate.cpp

4713

Yeah, this is what mean. I understood it is not easy/good to implement.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2163

It looks better to add some comments.

[erichkeane]

clang/include/clang/AST/Decl.h

1891

Woops, I DID mess that up :) I will switch to basically your wording.

This _IS_ generally covered by TK_NonTemplate (and was before), and I considered not adding it at all, but it is really nice to check getTemplatedKind to see if the value I need is going to be there.

1942

This is actually the cases where it is NOT a local lambda or function object inside a function template. I'll give a reword a go.

2691–2692

Yeah, thats a pretty low quality comment as well :/ I'll give it another shot that will hopefully be more understandable. It isn't quite what you said though.

clang/lib/AST/Decl.cpp

3787

You do not understand incorrectly :/ Copy-paste error. I'll remove 'member'.

clang/lib/AST/DeclBase.cpp

299

This is 'exactly' the function above (getParentFunctionOrMethod), except it uses the Lexical context instead of the semantic context. Would you prefer a name of 'getParentFunctionOrMethodLexically`?

clang/lib/Sema/SemaConcept.cpp

480–488

Changes made!

And I am wondering if we could hit these 2 checks only...

We cannot, this applies generally, including in cases where the current function is a local lambda or function object.

[erichkeane]

Thanks for the review @ChuanqiXu !

[erichkeane]

Well, crud. @cor3ntin broke me again :) I went to rebase to get pre-commit to run again, and now the problem is the lambda captures. Taking a look.

[cor3ntin]

Oh no. Let me know if i can help in any way

[erichkeane]

If anyone can help here, it would be vastly appreciated... I'm simply out of ideas on how to make this work.

clang/lib/Sema/SemaConcept.cpp

496

Ugh... it looks like all of this might just be 'wrong', and I have no idea how to fix it. @rsmith ANY advise you could give here would be unbelievably appreciated.

Basically, anything involving variables besides ParmVarDecls seems to be broken in some way:

template<typename T>
void foo(T x) {

// This asserts because 'y' is not in the Scope's "FindInstantiationOf"
[y = x]() requires(constraint<decltype(y)>){}


// This asserts because 'Local' is not in the Scope's "FindInstantiationOf"
T Local;
[]() requires(constraint<decltype(Local)>){}

// This gives a "stack nearly exhausted" error, followed by a bunch of "while checking constraint satisfaction for function 'foo' required here'", THEN crashes checking Constraints due to MLTAL being empty at one point (or perhaps just corrupt?). BUT the stack is only 40 deep at the crash.
struct S {
int local;
void foo() requires(constraint<decltype(local)>){}
}

FURTHER, this is also broken by this patch:

template<typename T>
struct S {
T t;
void foo() requires(constraint<decltype(t)>){}
};
void use() {
S<int> s;
s.foo();

With "constraints not satisfied" "because substituted constraint expression is ill-formed: 'S::t' is not a member of 'class S<int>'"

Curiously, it works if 'foo' is itself a template.

I can't help but think that this attempt to re-generate the instantiation is just BROKEN, and I have no idea how to fix it, or what the right approach is. BUT I cannot help but think that I'm doing the 'wrong thing'.

[cor3ntin]

clang/lib/Sema/TreeTransform.h

13002

That doesn't look right.
At best if you don't transform the trailing return type you wouldn't refer to the transformed captures and that is the issue you are seeing with

// This asserts because 'y' is not in the Scope's "FindInstantiationOf"
[y = x]() requires(constraint<decltype(y)>){}

But i suspect this should explode even more spectacularly in some cases?
If i understand correctly, it shouldn't be checked - but it still be substituted.... or am i missing something?

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

496

Note that each of those lambdas ALSO needs to be called there, I forgot the last ().

clang/lib/Sema/TreeTransform.h

13002

At best if you don't transform the trailing return type you wouldn't refer to the transformed captures and that is the issue you are seeing with

So the point of the patch here is that we cannot transform this until we need to 'check' this constraint. I also missed the 'call' of the lambdas in each of those cases, so it does need to be 'checked', which means it needs to be 'substituted'.

But i suspect this should explode even more spectacularly in some cases?
If i understand correctly, it shouldn't be checked - but it still be substituted.... or am i missing something?

I think it cannot be substituted either, because the type might be different by the time you get to the need to check, see here: https://godbolt.org/z/GxP9T6fa1

[erichkeane]

clang/lib/Sema/TreeTransform.h

13002

Hrm.... i just discovered "RebuildExprInCurrentInstantiation". I wonder if all of these places should be doing THAT instead...

[erichkeane]

clang/lib/Sema/TreeTransform.h

13002

Hrm.... i just discovered "RebuildExprInCurrentInstantiation". I wonder if all of these places should be doing THAT instead...

This does.... SOMETHING, just not the right thing :/ It DOES do some work it seems, but not enough to update everything? I have no idea what is messed up here.

I'm pretty much out of ideas here.

[erichkeane]

Added the tests that still fail to 'concepts.cpp', I still need to figure those out.

However, I switched our 'skipping of instantiation' over to use RebuildExprInCurrentInstantiation, which LOOKS like it is doing the right thing in a couple of cases. I thought perhaps this would be useful progress for anyone who has ideas/time to poke around to help.

[erichkeane]

clang/test/SemaTemplate/concepts.cpp

391

A bunch of the tests below this all fail.

[erichkeane]

Found that the recursive var-decl collection was incorrect, since all the values were already in the parent scopes! So I ended up being able to fix MOST of the problems by simply making the 'scope' inherit when instantiating constraints. This should make us use the 'lookup rules' of the parent, which is correct I believe.

I have 1 more pair of failures (see commented out tests in concepts.cpp) where the lookup for a member-variable doesn't work right.

[erichkeane]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2329

I'm still not sure if anything should be rebuilt here, I suspect the answer is MAYBE on the named-concept, but it isn't clear to me.

clang/test/SemaTemplate/concepts.cpp

391

See these two tests, which fail by saying that "::local is not a member of class 'X'".

[ChuanqiXu]

clang/test/SemaTemplate/concepts.cpp

391

I've spent some time to look into this. I don't find the root cause now. But I find that it is related to the wrong lookups. The error is emitted in CheckQualifiedMemberReference. And it looks like we lookup for the original S instead of the instantiated S. And the compiler thinks the 2 structs are different. So here is the error. Do you have any other ideas?

[erichkeane]

clang/test/SemaTemplate/concepts.cpp

391

I unfortunately had little time to work on this over the weekend, but right before I left on Friday realized that I think the idea of doing RebuildExprInCurrentInstantiation was the incorrect one. It ATTEMPTS to rebuild things, but doesn't have enough information to do so (which is the case you're seeing I think?). If I change that back to what I had before in every case, all except CheckMemberVar::foo fails (note foo2 does not!).

THAT I have tracked down to TemplateInstantiator::TransformDecl failing to actually transform the variable from S<T> to S<int> as you noticed.

It gets me to 3 cases where that function is called, 2 of which work.
1- CheckMemberVar::foo: Fails
2- CheckMemberVar::foo2: Works
3- Similar example with the instantiation in the BODY of the func: Works

I am going to spend more time trying to look into that hopefully early this week.

[erichkeane]

Here is my current 'WIP' patch, with only ChecksMemberVar::foo failing. I noticed that the problem so far starts in TransformMemberExpr, which calls TransformDecl (TreeTransform.h: ~11018). THAT ends up being a 'pass through' and just calls SemaRef.FindInstantiatedDecl, which attempts to get the instantiated version of S in its call to FindInstantiatedContext (see SemaTemplateInstantiateDecl.cpp:6160).

THAT calls FindInstantiatedDecl again on the ParentDC (which is the uninstantiated CXXRecordDecl of S), and ends up going down past the ClassTemplate calculation (~6083, which is DeclContext *DC = CurContext), and everything is the same up to there.

That is the point that I have ended looking into it at the moment, but I think the problem/or at least something that should suggest the fix is there, since it manages to instead return the primary template instead of the instantiation-of-int.

[erichkeane]

Here is the example I'm playing with:

template<typename T>
constexpr bool constraint = true;

#define BROKE_REQUIRES 1
#define WORKING_REQUIRES 2
#define WORKING_BODY 3
 
#define TEST BROKE_REQUIRES
//#define TEST WORKING_REQUIRES
//#define TEST WORKING_BODY
 
template<typename T>
struct S {
  T t;
  void foo()
#if TEST == BROKE_REQUIRES
    requires (constraint<decltype(t)>)
#endif
    {
#if TEST == WORKING_BODY
    using G = decltype(t);
#endif
  }
#if TEST == WORKING_REQUIRES
  template<typename U>
  void foo2() requires (constraint<decltype(t)>){}
#endif
};
 
void bar() {
  S<int> s;
#if TEST == BROKE_REQUIRES || TEST == WORKING_BODY
  s.foo();
#endif
#if TEST == WORKING_REQUIRES
  s.foo2<int>();
#endif
}

For all 3 versions (see the definitions of TEST), they get to 6083 on the 1st breakpoint.

[erichkeane]

Alright, a touch more: It looks like the looping through the CurContext at line 6084 finds the wrong thing in evaluating this CXXMethodDecl! The "broken" version has its CurContext as bar in the example above, the "working" versions have foo or foo2<int>. Therefore the 2nd time through the loop, those both end up on the ClassTemplateSpecializationDecl, but the broken one finds the isFileContext version.

So I don't know HOW to fix it, but it seems that the CheckFunctionConstraints probably has to change the CurContext. I suspect the instantiation of the WorkingRequires does something like this correctly.

I'm poking on/off on it today, but if @ChuanqiXu has an idea/help, it would be greatly appreciated.

[ChuanqiXu]

In D119544#3472537, @erichkeane wrote:

Alright, a touch more: It looks like the looping through the CurContext at line 6084 finds the wrong thing in evaluating this CXXMethodDecl! The "broken" version has its CurContext as bar in the example above, the "working" versions have foo or foo2<int>. Therefore the 2nd time through the loop, those both end up on the ClassTemplateSpecializationDecl, but the broken one finds the isFileContext version.

So I don't know HOW to fix it, but it seems that the CheckFunctionConstraints probably has to change the CurContext. I suspect the instantiation of the WorkingRequires does something like this correctly.

I'm poking on/off on it today, but if @ChuanqiXu has an idea/help, it would be greatly appreciated.

Yeah, I also find the information looks in a mess. I spent some time to look at the bug (my workaround shows below) but it would make deferred-concept-inst.cpp fail... it would say foo() is not a member of Test... I don't have strong proves here. But I guess we might need to look at tree transformation for RequireExpr in TreeTransform<Derived>::TransformRequiresExpr.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2424–2432

The failing test case could be fixed by the above change. The change above is just a hack instead of a suggestion.
The rationale is that we need to record some information when we traverse the template decl. The bugs now looks like we forget to do something which would be done at the first time. And we meet in problems due to the missed information (we couldn't find the member in the class).

clang/test/SemaTemplate/concepts.cpp

386–388

We might need more negative tests.
Now it would pass even if I write:

[]()
    requires(false)
{}();

[erichkeane]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2424–2432

So this would un-do the 'deferred constraint checking', because that woudl cause us to immediately instantiate the template, so I don't think we want to do that.

clang/test/SemaTemplate/concepts.cpp

386–388

Ouch! I'll have to see if I can find where we're missing that check!

[erichkeane]

I found

clang/test/SemaTemplate/concepts.cpp

386–388

Well, I fixed most of the OTHER stuff I think by setting up the context properly (see incoming patch!), but this ends up failing crashing, as does a few other things in this test. Still looking through it, but want to get you the latest.

[erichkeane]

Now crashes a few times in concepts.cpp, but sets up the instantiation-scope for functions such that local variable references work for them.

clang/lib/Sema/SemaConcept.cpp

507

This line was the one that fixed the variable lookup in non-templates.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

507

By changing FD to FD->GetNonClosureContext (with the appropriate casts), I was at least able to fix the crash. The constraint not being checked for failure however, is still a problem.

[erichkeane]

Update the concepts-test, remove some extra commented out code.

[erichkeane]

OK, 1 issue left (see FriendFunc).

clang/test/SemaTemplate/concepts.cpp

436

So see the 'TODO' for all the interesting cases. Lambdas apparently have not had their constraints checked even before this: https://godbolt.org/z/sxjTx7WGn

So I think that is a bug that is 'existing', and I would love to make that a separate patch/bug fix.

471

THIS one looks like a regression, it seems to matter which order these two functions are called in, whether it catches it or not.

So this is the 1 example I have: https://godbolt.org/z/vY4f3961s Note that if you swap lines 32 and 33, this patch ALSO fails, but in the version in the example, it does NOT fail despite the fact that it SHOULD.

[erichkeane]

Correct the caching behavior to make the FriendFunc example work.

I THINK this is ready to review! I'd like to do the lambda examples in a followup patch as I believe that is a pre-existing issue, and this patch has gotten large enough.

[erichkeane]

In D119544#3477316, @erichkeane wrote:

Correct the caching behavior to make the FriendFunc example work.

I THINK this is ready to review! I'd like to do the lambda examples in a followup patch as I believe that is a pre-existing issue, and this patch has gotten large enough.

Ping @ChuanqiXu : This should be ready for you! Let me know what changes you suggest.

clang/test/SemaTemplate/concepts.cpp

471

This one was actually pretty easy, I never updated the 'caching' for constraints, so we were only doing it based on the outermost level of template args, so the two were considered to be the 'same' instantiation for the purposes of caching..

[ChuanqiXu]

Oh, I'm busy with coroutines these days. Thanks for reminding me for this. LGTM to me basically. Only comments for style left.

clang/lib/Sema/SemaConcept.cpp

70

I feel like the usage of the API could be further simplified.

338–340

365–370

I think it is a chance to add range style interfaces for MultiLevelTemplateArgumentList. So that we could use range-based loop here.

425–469

The suggested change works. I feel it is not identical with the original. Is it correct or do we miss something in the test?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2162

The check here is not straightforward. Here we want to check if the Function is local declared function. But what we check here is about friendness. I am not 100% sure if it is right and other reader might be confusing too. I would suggest to check it directly. e.g, check if one of its DeclContext is FunctionDecl.

clang/lib/Sema/TreeTransform.h

13003

I think we could eliminate NewTrailingRequiresClause

clang/test/SemaTemplate/concepts.cpp

275–276

Better to rename constraint to something like IsInt

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

70

Heh, the suggestion is inverted slightly (those should be !isUsable) which caught me for a while :) Either way, I think it is a good suggestion.

425–469

The very top condition is not exactly the same I think, but in a way that I don't believe matters now that this function isn't recursive.

Otherwise I think this is a vast improvement, so I'll keep it!

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2162

I cannot come up with a better one here. Really the condition here is "this is not one of the above conditions, and is still not a friend function". A friend could ALSO have a DeclContext of a FunctionDecl as well, so I don't have a good idea of what we could do.

[erichkeane]

Make all the changes that @ChuanqiXu suggested. Thank you again so much for your help during this!

I intend to commit this my Monday-AM unless someone comments differently.

[gulfem]

We started seeing several test failures after this commit:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview

One example is nothrow_forward_range.compile.pass.cpp.

Script:
--
: 'COMPILED WITH';  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp  --target=x86_64-unknown-linux-gnu -nostdinc++ -I /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I /b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings  -fsyntax-only
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "COMPILED WITH"
$ "/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++" "/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp" "--target=x86_64-unknown-linux-gnu" "-nostdinc++" "-I" "/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1" "-I" "/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1" "-I" "/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support" "-std=c++2b" "-Werror" "-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" "-fsyntax-only"
# command stderr:
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:18:
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support/test_range.h:12:
In file included from /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/ranges:278:
In file included from /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/all.h:18:
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: error: redefinition of 'operator|'
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)
                                    ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/common_view.h:107:17: note: in instantiation of template class 'std::__range_adaptor_closure<std::ranges::views::__common::__fn>' requested here
  struct __fn : __range_adaptor_closure<__fn> {
                ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: note: previous definition is here
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)
                                    ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: error: redefinition of 'operator|'
    friend constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
                          ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: note: previous definition is here
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: error: redefinition of 'operator|'
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)
--

[erichkeane]

In D119544#3486227, @gulfem wrote:

We started seeing several test failures after this commit:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview

One example is nothrow_forward_range.compile.pass.cpp.

Script:
--
: 'COMPILED WITH';  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp  --target=x86_64-unknown-linux-gnu -nostdinc++ -I /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I /b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings  -fsyntax-only
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "COMPILED WITH"
$ "/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++" "/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp" "--target=x86_64-unknown-linux-gnu" "-nostdinc++" "-I" "/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1" "-I" "/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1" "-I" "/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support" "-std=c++2b" "-Werror" "-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" "-fsyntax-only"
# command stderr:
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:18:
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support/test_range.h:12:
In file included from /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/ranges:278:
In file included from /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/all.h:18:
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: error: redefinition of 'operator|'
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)
                                    ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/common_view.h:107:17: note: in instantiation of template class 'std::__range_adaptor_closure<std::ranges::views::__common::__fn>' requested here
  struct __fn : __range_adaptor_closure<__fn> {
                ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: note: previous definition is here
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)
                                    ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: error: redefinition of 'operator|'
    friend constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
                          ^
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: note: previous definition is here
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: error: redefinition of 'operator|'
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)
--

Ah shucks... Thanks for the heads up. Is there any chance to get you to get me a pre-processed version of this failure to play with? I've not had luck compiling/running libc++ tests in the past.

[gulfem]

In D119544#3486241, @erichkeane wrote:

Ah shucks... Thanks for the heads up. Is there any chance to get you to get me a pre-processed version of this failure to play with? I've not had luck compiling/running libc++ tests in the past.

Sure, I'll try to help. What exactly do you need?
Is it possible to revert the change while you are working on the fix?

[erichkeane]

In D119544#3486385, @gulfem wrote:

In D119544#3486241, @erichkeane wrote:

Ah shucks... Thanks for the heads up. Is there any chance to get you to get me a pre-processed version of this failure to play with? I've not had luck compiling/running libc++ tests in the past.

Sure, I'll try to help. What exactly do you need?
Is it possible to revert the change while you are working on the fix?

The change has already been reverted (a 2nd time now, see https://github.com/llvm/llvm-project/commit/45c07db31cc76802a1a2e41bed1ce9c1b8198181). First time was an amd-32-bit build failure.

If you could run the command line listed above with a -E, and find a way to get me the output, I think that would be incredibly helpful!

So based on your above:

/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp  --target=x86_64-unknown-linux-gnu -nostdinc++ -I /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I /b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings  -fsyntax-only -E

Then pipe that to a file (note the -E I added at the end). You should get a file that looks like some slightly-wonky C++ code.

Thanks!

[gulfem]

Then pipe that to a file (note the -E I added at the end). You should get a file that looks like some slightly-wonky C++ code.

I got the following output after running it via -E.

/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:15:10: fatal error: 'memory' file not found
#include <memory>
         ^~~~~~~~
# 1 "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 434 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp" 2
# 21 "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
template <typename>
struct ForwardProxyIterator {
  using value_type = int;
  using difference_type = int;
  ForwardProxyIterator& operator++();
  ForwardProxyIterator operator++(int);
  bool operator==(const ForwardProxyIterator&) const;

  int operator*() const;
};

static_assert(std::ranges::__nothrow_forward_range<test_range<forward_iterator>>);
static_assert(!std::ranges::__nothrow_forward_range<test_range<cpp20_input_iterator>>);
static_assert(std::ranges::forward_range<test_range<ForwardProxyIterator>>);
static_assert(!std::ranges::__nothrow_forward_range<test_range<ForwardProxyIterator>>);

constexpr bool forward_subsumes_input(std::ranges::__nothrow_forward_range auto&&) {
  return true;
}
constexpr bool forward_subsumes_input(std::ranges::__nothrow_input_range auto&&);

static_assert(forward_subsumes_input("foo"));
1 error generated.

Would that be enough?

[erichkeane]

In D119544#3486573, @gulfem wrote:

Then pipe that to a file (note the -E I added at the end). You should get a file that looks like some slightly-wonky C++ code.

I got the following output after running it via -E.

/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:15:10: fatal error: 'memory' file not found
#include <memory>
         ^~~~~~~~
# 1 "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 434 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp" 2
# 21 "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
template <typename>
struct ForwardProxyIterator {
  using value_type = int;
  using difference_type = int;
  ForwardProxyIterator& operator++();
  ForwardProxyIterator operator++(int);
  bool operator==(const ForwardProxyIterator&) const;

  int operator*() const;
};

static_assert(std::ranges::__nothrow_forward_range<test_range<forward_iterator>>);
static_assert(!std::ranges::__nothrow_forward_range<test_range<cpp20_input_iterator>>);
static_assert(std::ranges::forward_range<test_range<ForwardProxyIterator>>);
static_assert(!std::ranges::__nothrow_forward_range<test_range<ForwardProxyIterator>>);

constexpr bool forward_subsumes_input(std::ranges::__nothrow_forward_range auto&&) {
  return true;
}
constexpr bool forward_subsumes_input(std::ranges::__nothrow_input_range auto&&);

static_assert(forward_subsumes_input("foo"));
1 error generated.

Would that be enough?

Ah, thats unfortunate, I wouldn't expect that to happen in the same place as the previous repo. I'll see if I can find another way to repro it.

[erichkeane]

I was able to find a reproduction of the problem:

  template<typename T, typename U>
    struct is_same { static constexpr bool value = false; };
  template<typename T>
    struct is_same<T,T> { static constexpr bool value = false; };

template<typename T, typename U>
  concept same_as = is_same<T, U>::value;

template <class _Tp>
struct __range_adaptor_closure {


    template </*ranges::viewable_range */typename _View, typename _Closure>
        requires same_as<_Tp, _Closure>
    friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure){};

};


struct A : __range_adaptor_closure<A>{};
struct B : __range_adaptor_closure<B>{};

As far as I can tell, there is nothing wrong with the libc++ code, so I have to figure out why after this patch that the requires clause doesn't realize these are different.

[erichkeane]

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

[erichkeane]

I ended up having to revert again after my attempt this morning, this time due to some crash compiling a libc++ example. I haven't been able to repro it yet, but I'm hopeful that one of follow-up buildbots will give me a better reproducer command line.

That said, I have a pretty heavy feeling that my approach here is perhaps misguided. I attempted to follow the implementation of noexcept for this, but I think that was an incorrect direction. I am going to try to work on a replacement for this using the same test changes that I've discovered/added through this patch, and see if my replacement is a better fit/idea.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I'm unsure what fallout this will cause otherwise, but I think is worth the diversion. Sadly, this might end up delaying our ability to compile libstdc++'s ranges for another release.

[erichkeane]

@rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is supposed to fix the friend function issues:

A non-template friend declaration with a requires-clause shall be a definition.
A friend function template with a constraint that depends on a template parameter from an enclosing template shall be a definition.
Such a constrained friend function or function template declaration does not declare the same function or function template as a declaration in any other scope.

To me, this says that

template<typename T>
struct S {
    friend void foo() requires true {}
};
void bar() {
S<int> s;
S<float> s2;
}

Should be perfectly fine, and NOT a redefinition? At least this: https://godbolt.org/z/PaK1Yhn1E seems to show that all 3 compilers get this wrong currently? Or I'm misreading this.

It ALSO means that:

template<typename T>
struct S {
template<typename U>
friend void foo() requires constriant<T> {}
};
void bar() {
S<int> s;
S<float> s2;
}

is perfectly fine, and not a redefinition. (CURRENT Clang and GCC get this right, but MSVC seems to get it wrong: https://godbolt.org/z/a7dYezoPW)

HOWEVER, By reading the rule, a slightly DIFFERENT version of that

template<typename T>
struct S {
template<typename U>
friend void foo() requires constriant<U> {} // No longer relies on the enclosing template
};
void bar() {
S<int> s;
S<float> s2;
}

Is a redefinition. All 3 compilers diagnose this. https://godbolt.org/z/7qbYsb635

Do I have this right? I'm having trouble figuring out the implementation strategy here. It seems my SemaOverload.cpp changes would need to do special work to get example #1 to work, and I got close on #2 (with the exception of some assert in libc++). And #3 I probably had right?

Can someone else read the standard bit there and make sure I got a good spread of the issues and interpreted the prose correctly?

Thanks!
-Erich

[ChuanqiXu]

In D119544#3494281, @erichkeane wrote:

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I agree this should be a better direction.

In D119544#3524844, @erichkeane wrote:

@rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is supposed to fix the friend function issues:

A non-template friend declaration with a requires-clause shall be a definition.
A friend function template with a constraint that depends on a template parameter from an enclosing template shall be a definition.
Such a constrained friend function or function template declaration does not declare the same function or function template as a declaration in any other scope.

To me, this says that

template<typename T>
struct S {
    friend void foo() requires true {}
};
void bar() {
S<int> s;
S<float> s2;
}

Should be perfectly fine, and NOT a redefinition? At least this: https://godbolt.org/z/PaK1Yhn1E seems to show that all 3 compilers get this wrong currently? Or I'm misreading this.

I agree this one should be fine. I think we could fix the problem by making non-template function inside a template class to be a dependent type. Like this one is accepted: https://godbolt.org/z/MorzcqE1a
This bug might not be horrible since few developer would write friend function definition which doesn't touch the enclosing class.

It ALSO means that:

template<typename T>
struct S {
template<typename U>
friend void foo() requires constriant<T> {}
};
void bar() {
S<int> s;
S<float> s2;
}

is perfectly fine, and not a redefinition. (CURRENT Clang and GCC get this right, but MSVC seems to get it wrong: https://godbolt.org/z/a7dYezoPW)

HOWEVER, By reading the rule, a slightly DIFFERENT version of that

template<typename T>
struct S {
template<typename U>
friend void foo() requires constriant<U> {} // No longer relies on the enclosing template
};
void bar() {
S<int> s;
S<float> s2;
}

Is a redefinition. All 3 compilers diagnose this. https://godbolt.org/z/7qbYsb635

I agree with the two judgement here. It looks like the implementation of clang is right on these two versions here.

clang/lib/Sema/SemaConcept.cpp

70

Oh, my bad. It is a typo.

166

175

182–183

[erichkeane]

In D119544#3526810, @ChuanqiXu wrote:

In D119544#3494281, @erichkeane wrote:

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I agree this should be a better direction.

I actually got a response from Richard who seems to be more in favor of the solution I tried initially (the one in this patch!). The problems I have with it I think get solved by the 'friend function' rules that I pasted above, so I THINK I can fix those and be ok. I'll still need SOME level of tree-transform, but only to see if it depends on the enclosing template.

I agree this one should be fine. I think we could fix the problem by making non-template function inside a template class to be a dependent type. Like this one is accepted: >https://godbolt.org/z/MorzcqE1a
This bug might not be horrible since few developer would write friend function definition which doesn't touch the enclosing class.

Yeah, that is a really strange one. I don't think we can make it dependent as then it wouldn't be callable when fully instantiate-able. I don't have a good idea how to make this work in the AST though, and might end up leaving it as a 'fixme'. As you said, I think you're right that this is a very small bug-vector.

[ChuanqiXu]

In D119544#3527556, @erichkeane wrote:

In D119544#3526810, @ChuanqiXu wrote:

In D119544#3494281, @erichkeane wrote:

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I agree this should be a better direction.

I actually got a response from Richard who seems to be more in favor of the solution I tried initially (the one in this patch!). The problems I have with it I think get solved by the 'friend function' rules that I pasted above, so I THINK I can fix those and be ok. I'll still need SOME level of tree-transform, but only to see if it depends on the enclosing template.

Great!

I agree this one should be fine. I think we could fix the problem by making non-template function inside a template class to be a dependent type. Like this one is accepted: >https://godbolt.org/z/MorzcqE1a
This bug might not be horrible since few developer would write friend function definition which doesn't touch the enclosing class.

Yeah, that is a really strange one. I don't think we can make it dependent as then it wouldn't be callable when fully instantiate-able. I don't have a good idea how to make this work in the AST though, and might end up leaving it as a 'fixme'. As you said, I think you're right that this is a very small bug-vector.

Yeah, my point is that this is not your fault. And we could fix the problem later when we have time since it is not hurry.

clang/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^
 - Diagnose consteval and constexpr issues that happen at namespace scope. This
   partially addresses `Issue 51593 <https://github.com/llvm/llvm-project/issues/51593>`_.
 - No longer attempt to evaluate a consteval UDL function call at runtime when
   it is called through a template instantiation. This fixes
   `Issue 54578 <https://github.com/llvm/llvm-project/issues/54578>`_.
 
 - Implemented `__builtin_source_location()` which enables library support for std::source_location.
+- Clang now correctly delays the instantiation of function constraints until
+  the time of checking, which should now allow the libstdc++ ranges implementation
+  to work for at least trivial examples.  This fixes
+  `Issue 44178 <https://github.com/llvm/llvm-project/issues/44178>`_.
 
 - The mangling scheme for C++20 modules has incompatibly changed. The
   initial mangling was discovered not to be reversible, and the weak
   ownership design decision did not give the backwards compatibility
   that was hoped for. C++20 since added ``extern "C++"`` semantics
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 

clang/include/clang/AST/Decl.h

enum TemplatedKind {
     // specialization of a member of a templated class.
     TK_MemberSpecialization,
     // An instantiation or explicit specialization of a function template.
     // Note: this might have been instantiated from a templated class if it
     // is a class-scope explicit specialization.
     TK_FunctionTemplateSpecialization,
     // A function template specialization that hasn't yet been resolved to a
     // particular specialized function template.
-    TK_DependentFunctionTemplateSpecialization
+    TK_DependentFunctionTemplateSpecialization,
+    // A non templated function which is in a dependent scope.
+    TK_DependentNonTemplate
   };
 
   /// Stashed information about a defaulted function definition whose body has
   /// not yet been lazily generated.
   class DefaultedFunctionInfo final
       : llvm::TrailingObjects<DefaultedFunctionInfo, DeclAccessPair> {
     friend TrailingObjects;
     unsigned NumLookups;
private:
   /// we can only compute the full range once the function body has been
   /// de-serialized, so it's far better to have the (sometimes-redundant)
   /// EndRangeLoc.
   SourceLocation EndRangeLoc;
 
   /// The template or declaration that this declaration
   /// describes or was instantiated from, respectively.
   ///
-  /// For non-templates, this value will be NULL. For function
-  /// declarations that describe a function template, this will be a
-  /// pointer to a FunctionTemplateDecl. For member functions
-  /// of class template specializations, this will be a MemberSpecializationInfo
-  /// pointer containing information about the specialization.
-  /// For function template specializations, this will be a
-  /// FunctionTemplateSpecializationInfo, which contains information about
-  /// the template being specialized and the template arguments involved in
-  /// that specialization.
-  llvm::PointerUnion<FunctionTemplateDecl *,
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl. For function
+  /// declarations that describe a function template, this will be a pointer to
+  /// a FunctionTemplateDecl. For member functions of class template
+  /// specializations, this will be a MemberSpecializationInfo pointer
+  /// containing information about the specialization. For function template
+  /// specializations, this will be a FunctionTemplateSpecializationInfo, which
+  /// contains information about the template being specialized and the template
+  /// arguments involved in that specialization.
+  llvm::PointerUnion<FunctionDecl *, FunctionTemplateDecl *,
                      MemberSpecializationInfo *,
                      FunctionTemplateSpecializationInfo *,
                      DependentFunctionTemplateSpecializationInfo *>
       TemplateOrSpecialization;
 
   /// Provides source/type location info for the declaration name embedded in
   /// the DeclaratorDecl base class.
   DeclarationNameLoc DNLoc;
 
   /// Specify that this function declaration is actually a function
   /// template specialization.
   ///
public:
 
   /// Specify that this record is an instantiation of the
   /// member function FD.
   void setInstantiationOfMemberFunction(FunctionDecl *FD,
                                         TemplateSpecializationKind TSK) {
     setInstantiationOfMemberFunction(getASTContext(), FD, TSK);
   }
 
+  /// Specify that this function declaration was instantiated from FunctionDecl
+  /// FD. This is only used if this is a function declaration declared locally
+  /// inside of a function template.
+  void setInstantiatedFromDecl(FunctionDecl *FD);
+
   /// Retrieves the function template that is described by this
   /// function declaration.
   ///
   /// Every function template is represented as a FunctionTemplateDecl
   /// and a FunctionDecl (or something derived from FunctionDecl). The
   /// former contains template properties (such as the template
   /// parameter lists) while the latter contains the actual
   /// description of the template's
   /// contents. FunctionTemplateDecl::getTemplatedDecl() retrieves the
   /// FunctionDecl that describes the function template,
   /// getDescribedFunctionTemplate() retrieves the
   /// FunctionTemplateDecl from a FunctionDecl.
   FunctionTemplateDecl *getDescribedFunctionTemplate() const;
 
+  FunctionDecl *getInstantiatedFromDecl() const;
+
   void setDescribedFunctionTemplate(FunctionTemplateDecl *Template);
 
   /// Determine whether this function is a function template
   /// specialization.
   bool isFunctionTemplateSpecialization() const {
     return getPrimaryTemplate() != nullptr;
   }
 

clang/include/clang/AST/DeclBase.h

public:
   /// If this decl is defined inside a function/method/block it returns
   /// the corresponding DeclContext, otherwise it returns null.
   const DeclContext *getParentFunctionOrMethod() const;
   DeclContext *getParentFunctionOrMethod() {
     return const_cast<DeclContext*>(
                     const_cast<const Decl*>(this)->getParentFunctionOrMethod());
   }
 
+  /// Does the same thing as getParentFunctionOrMethod, except starts with the
+  /// lexical declaration context instead.
+  const DeclContext *getLexicalParentFunctionOrMethod() const;
+  DeclContext *getLexicalParentFunctionOrMethod() {
+    return const_cast<DeclContext *>(
+        const_cast<const Decl *>(this)->getLexicalParentFunctionOrMethod());
+  }
+
   /// Retrieves the "canonical" declaration of the given declaration.
   virtual Decl *getCanonicalDecl() { return this; }
   const Decl *getCanonicalDecl() const {
     return const_cast<Decl*>(this)->getCanonicalDecl();
   }
 
   /// Whether this particular Decl is a canonical one.
   bool isCanonicalDecl() const { return getCanonicalDecl() == this; }

clang/include/clang/Sema/Sema.h

private:
   /// Introduce the instantiated function parameters into the local
   /// instantiation scope, and set the parameter names to those used
   /// in the template.
   bool addInstantiatedParametersToScope(
       FunctionDecl *Function, const FunctionDecl *PatternDecl,
       LocalInstantiationScope &Scope,
       const MultiLevelTemplateArgumentList &TemplateArgs);
 
+  /// used by SetupConstraintCheckingTemplateArgumentsAndScope to recursively(in
+  /// the case of lambdas) set up the LocalInstantiationScope of the current
+  /// function.
+  bool SetupConstraintScope(
+      FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
+      MultiLevelTemplateArgumentList MLTAL, LocalInstantiationScope &Scope);
+
+  /// Used during constraint checking, sets up the constraint template arguemnt
+  /// lists, and calls SetupConstraintScope to set up the
+  /// LocalInstantiationScope to have the proper set of ParVarDecls configured.
+  llvm::Optional<MultiLevelTemplateArgumentList>
+  SetupConstraintCheckingTemplateArgumentsAndScope(
+      FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
+      LocalInstantiationScope &Scope);
+
+  // Keep track of whether we are evaluating a constraint.
+  unsigned ConstraintEvaluationDepth = 0;
+
+  class ConstraintEvalRAII {
+    Sema &S;
+
+  public:
+    ConstraintEvalRAII(Sema &S) : S(S) { ++S.ConstraintEvaluationDepth; }
+    ~ConstraintEvalRAII() { --S.ConstraintEvaluationDepth; }
+  };
+
 public:
+  bool IsEvaluatingAConstraint() { return ConstraintEvaluationDepth > 0; }
   const NormalizedConstraint *
   getNormalizedAssociatedConstraints(
       NamedDecl *ConstrainedDecl, ArrayRef<const Expr *> AssociatedConstraints);
 
   /// \brief Check whether the given declaration's associated constraints are
   /// at least as constrained than another declaration's according to the
   /// partial ordering of constraints.
   ///
public:
   bool MaybeEmitAmbiguousAtomicConstraintsDiagnostic(NamedDecl *D1,
       ArrayRef<const Expr *> AC1, NamedDecl *D2, ArrayRef<const Expr *> AC2);
 
   /// \brief Check whether the given list of constraint expressions are
   /// satisfied (as if in a 'conjunction') given template arguments.
   /// \param Template the template-like entity that triggered the constraints
   /// check (either a concept or a constrained entity).
   /// \param ConstraintExprs a list of constraint expressions, treated as if
   /// they were 'AND'ed together.
-  /// \param TemplateArgs the list of template arguments to substitute into the
-  /// constraint expression.
+  /// \param TemplateArgList the multi-level list of template arguments to
+  /// substitute into the constraint expression. This should be relative to the
+  /// top-level (hence multi-level), since we need to instantiate fully at the
+  /// time of checking.
   /// \param TemplateIDRange The source range of the template id that
   /// caused the constraints check.
   /// \param Satisfaction if true is returned, will contain details of the
   /// satisfaction, with enough information to diagnose an unsatisfied
   /// expression.
   /// \returns true if an error occurred and satisfaction could not be checked,
   /// false otherwise.
   bool CheckConstraintSatisfaction(
       const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
-      ArrayRef<TemplateArgument> TemplateArgs,
+      const MultiLevelTemplateArgumentList &TemplateArgList,
+      SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction) {
+    llvm::SmallVector<Expr *, 4> Converted;
+    return CheckConstraintSatisfaction(Template, ConstraintExprs, Converted,
+                                       TemplateArgList, TemplateIDRange,
+                                       Satisfaction);
+  }
+
+  /// \brief Check whether the given list of constraint expressions are
+  /// satisfied (as if in a 'conjunction') given template arguments.
+  /// Additionally, takes an empty list of Expressions which is populated with
+  /// the instantiated versions of the ConstraintExprs.
+  /// \param Template the template-like entity that triggered the constraints
+  /// check (either a concept or a constrained entity).
+  /// \param ConstraintExprs a list of constraint expressions, treated as if
+  /// they were 'AND'ed together.
+  /// \param ConvertedConstraints a out parameter that will get populated with
+  /// the instantiated version of the ConstraintExprs if we successfully checked
+  /// satisfaction.
+  /// \param TemplateArgList the multi-level list of template arguments to
+  /// substitute into the constraint expression. This should be relative to the
+  /// top-level (hence multi-level), since we need to instantiate fully at the
+  /// time of checking.
+  /// \param TemplateIDRange The source range of the template id that
+  /// caused the constraints check.
+  /// \param Satisfaction if true is returned, will contain details of the
+  /// satisfaction, with enough information to diagnose an unsatisfied
+  /// expression.
+  /// \returns true if an error occurred and satisfaction could not be checked,
+  /// false otherwise.
+  bool CheckConstraintSatisfaction(
+      const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
+      llvm::SmallVectorImpl<Expr *> &ConvertedConstraints,
+      const MultiLevelTemplateArgumentList &TemplateArgList,
       SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction);
 
   /// \brief Check whether the given non-dependent constraint expression is
   /// satisfied. Returns false and updates Satisfaction with the satisfaction
   /// verdict if successful, emits a diagnostic and returns true if an error
   /// occurred and satisfaction could not be determined.
   ///
   /// \returns true if an error occurred, false otherwise.
public:
   ///
   /// \param TemplateArgs The converted, canonicalized template arguments.
   ///
   /// \param TemplateIDRange The source range of the template id that
   /// caused the constraints check.
   ///
   /// \returns true if the constrains are not satisfied or could not be checked
   /// for satisfaction, false if the constraints are satisfied.
-  bool EnsureTemplateArgumentListConstraints(TemplateDecl *Template,
-                                       ArrayRef<TemplateArgument> TemplateArgs,
-                                             SourceRange TemplateIDRange);
+  bool EnsureTemplateArgumentListConstraints(
+      TemplateDecl *Template, MultiLevelTemplateArgumentList TemplateArgs,
 
   /// \brief Emit diagnostics explaining why a constraint expression was deemed
   /// unsatisfied.
   /// \param First whether this is the first time an unsatisfied constraint is
   /// diagnosed for this error.
   void
   DiagnoseUnsatisfiedConstraint(const ConstraintSatisfaction &Satisfaction,
                                 bool First = true);
static void MarkDeducedTemplateParameters(ASTContext &Ctx,
                                   llvm::SmallBitVector &Deduced);
 
   //===--------------------------------------------------------------------===//
   // C++ Template Instantiation
   //
 
   MultiLevelTemplateArgumentList getTemplateInstantiationArgs(
       const NamedDecl *D, const TemplateArgumentList *Innermost = nullptr,
-      bool RelativeToPrimary = false, const FunctionDecl *Pattern = nullptr);
+      bool RelativeToPrimary = false, const FunctionDecl *Pattern = nullptr,
+      bool LookBeyondLambda = false, bool IncludeContainingStruct = false);
 
   /// A context in which code is being synthesized (where a source location
   /// alone is not sufficient to identify the context). This covers template
   /// instantiation and various forms of implicitly-generated functions.
   struct CodeSynthesisContext {
     /// The kind of template instantiation we are performing
     enum SynthesisKind {
       /// We are instantiating a template declaration. The entity is

   bool SubstParmTypes(SourceLocation Loc, ArrayRef<ParmVarDecl *> Params,
                       const FunctionProtoType::ExtParameterInfo *ExtParamInfos,
                       const MultiLevelTemplateArgumentList &TemplateArgs,
                       SmallVectorImpl<QualType> &ParamTypes,
                       SmallVectorImpl<ParmVarDecl *> *OutParams,
                       ExtParameterInfoBuilder &ParamInfos);
   ExprResult SubstExpr(Expr *E,
                        const MultiLevelTemplateArgumentList &TemplateArgs);
+  // Unlike the above, this evaluates constraints, which should only happen at
+  // 'constraint checking' time.
+  ExprResult
+  SubstConstraintExpr(Expr *E,
+                      const MultiLevelTemplateArgumentList &TemplateArgs);
 
   /// Substitute the given template arguments into a list of
   /// expressions, expanding pack expansions if required.
   ///
   /// \param Exprs The list of expressions to substitute into.
   ///
   /// \param IsCall Whether this is some form of call, in which case
   /// default arguments will be dropped.

clang/include/clang/Sema/Template.h

bool hasTemplateArgument(unsigned Depth, unsigned Index) const {
         return false;
 
       if (Index >= TemplateArgumentLists[getNumLevels() - Depth - 1].size())
         return false;
 
       return !(*this)(Depth, Index).isNull();
     }
 
+    bool isAnyArgInstantiationDependent() const {
+      for (ArgList List : TemplateArgumentLists)
+        for (const TemplateArgument &TA : List)
+          if (TA.isInstantiationDependent())
+            return true;
+      return false;
+    }
+
     /// Clear out a specific template argument.
     void setArgument(unsigned Depth, unsigned Index,
                      TemplateArgument Arg) {
       assert(NumRetainedOuterLevels <= Depth && Depth < getNumLevels());
       assert(Index < TemplateArgumentLists[getNumLevels() - Depth - 1].size());
       const_cast<TemplateArgument&>(
                 TemplateArgumentLists[getNumLevels() - Depth - 1][Index])
         = Arg;
public:
     void addOuterRetainedLevels(unsigned Num) {
       NumRetainedOuterLevels += Num;
     }
 
     /// Retrieve the innermost template argument list.
     const ArgList &getInnermost() const {
       return TemplateArgumentLists.front();
     }
+
+    /// Retrieve the outermost template argument list.
+    const ArgList &getOutermost() const { return TemplateArgumentLists.back(); }
   };
 
   /// The context in which partial ordering of function templates occurs.
   enum TPOC {
     /// Partial ordering of function templates for a function call.
     TPOC_Call,
 
     /// Partial ordering of function templates for a call to a

clang/lib/AST/ASTImporter.cpp

 
 Error ASTNodeImporter::ImportTemplateInformation(
     FunctionDecl *FromFD, FunctionDecl *ToFD) {
   switch (FromFD->getTemplatedKind()) {
   case FunctionDecl::TK_NonTemplate:
   case FunctionDecl::TK_FunctionTemplate:
     return Error::success();
 
+  case FunctionDecl::TK_DependentNonTemplate:
+    if (Expected<FunctionDecl *> InstFDOrErr =
+            import(FromFD->getInstantiatedFromDecl()))
+      ToFD->setInstantiatedFromDecl(*InstFDOrErr);
+    return Error::success();
   case FunctionDecl::TK_MemberSpecialization: {
     TemplateSpecializationKind TSK = FromFD->getTemplateSpecializationKind();
 
     if (Expected<FunctionDecl *> InstFDOrErr =
         import(FromFD->getInstantiatedFromMemberFunction()))
       ToFD->setInstantiationOfMemberFunction(*InstFDOrErr, TSK);
     else
       return InstFDOrErr.takeError();

clang/lib/AST/Decl.cpp

const IdentifierInfo *FunctionDecl::getLiteralIdentifier() const {
   if (getDeclName().getNameKind() == DeclarationName::CXXLiteralOperatorName)
     return getDeclName().getCXXLiteralIdentifier();
   return nullptr;
 }
 
 FunctionDecl::TemplatedKind FunctionDecl::getTemplatedKind() const {
   if (TemplateOrSpecialization.isNull())
     return TK_NonTemplate;
+  if (TemplateOrSpecialization.is<FunctionDecl *>())
+    return TK_DependentNonTemplate;
   if (TemplateOrSpecialization.is<FunctionTemplateDecl *>())
     return TK_FunctionTemplate;
   if (TemplateOrSpecialization.is<MemberSpecializationInfo *>())
     return TK_MemberSpecialization;
   if (TemplateOrSpecialization.is<FunctionTemplateSpecializationInfo *>())
     return TK_FunctionTemplateSpecialization;
   if (TemplateOrSpecialization.is
                                <DependentFunctionTemplateSpecializationInfo*>())

 }
 
 void FunctionDecl::setDescribedFunctionTemplate(FunctionTemplateDecl *Template) {
   assert(TemplateOrSpecialization.isNull() &&
          "Member function is already a specialization");
   TemplateOrSpecialization = Template;
 }
 
+void FunctionDecl::setInstantiatedFromDecl(FunctionDecl *FD) {
+  assert(TemplateOrSpecialization.isNull() &&
+         "function is already a specialization");
+  TemplateOrSpecialization = FD;
+}
+
+FunctionDecl *FunctionDecl::getInstantiatedFromDecl() const {
+  return TemplateOrSpecialization.dyn_cast<FunctionDecl *>();
+}
+
 bool FunctionDecl::isImplicitlyInstantiable() const {
   // If the function is invalid, it can't be implicitly instantiated.
   if (isInvalidDecl())
     return false;
 
   switch (getTemplateSpecializationKindForInstantiation()) {
   case TSK_Undeclared:
   case TSK_ExplicitInstantiationDefinition:

clang/lib/AST/DeclBase.cpp

for (const DeclContext *DC = getDeclContext();
        DC && !DC->isTranslationUnit() && !DC->isNamespace();
        DC = DC->getParent())
     if (DC->isFunctionOrMethod())
       return DC;
 
   return nullptr;
 }
 
+const DeclContext *Decl::getLexicalParentFunctionOrMethod() const {
+  for (const DeclContext *DC = getLexicalDeclContext();
+       DC && !DC->isTranslationUnit() && !DC->isNamespace();
+       DC = DC->getParent())
+    if (DC->isFunctionOrMethod())
+      return DC;
+
+  return nullptr;
+}
+
 //===----------------------------------------------------------------------===//
 // PrettyStackTraceDecl Implementation
 //===----------------------------------------------------------------------===//
 
 void PrettyStackTraceDecl::print(raw_ostream &OS) const {
   SourceLocation TheLoc = Loc;
   if (TheLoc.isInvalid() && TheDecl)
     TheLoc = TheDecl->getLocation();

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
       // Skip ConstructorDecl and DestructorDecl.
       if (isa<CXXConstructorDecl>(Method) || isa<CXXDestructorDecl>(Method))
         return true;
     }
 
     // Skip templated functions.
     switch (Decl->getTemplatedKind()) {
     case FunctionDecl::TK_NonTemplate:
+    case FunctionDecl::TK_DependentNonTemplate:
       break;
     case FunctionDecl::TK_MemberSpecialization:
     case FunctionDecl::TK_FunctionTemplateSpecialization:
       if (auto *TemplateInfo = Decl->getTemplateSpecializationInfo()) {
         if (!TemplateInfo->isExplicitInstantiationOrSpecialization())
           return true;
       }
       break;

clang/lib/Sema/SemaConcept.cpp

 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/StringExtras.h"
 
 using namespace clang;
 using namespace sema;
 
 namespace {
 class LogicalBinOp {
+  SourceLocation Loc;
   OverloadedOperatorKind Op = OO_None;
   const Expr *LHS = nullptr;
   const Expr *RHS = nullptr;
 
 public:
   LogicalBinOp(const Expr *E) {
     if (auto *BO = dyn_cast<BinaryOperator>(E)) {
       Op = BinaryOperator::getOverloadedOperator(BO->getOpcode());
       LHS = BO->getLHS();
       RHS = BO->getRHS();
+      Loc = BO->getExprLoc();
     } else if (auto *OO = dyn_cast<CXXOperatorCallExpr>(E)) {
       // If OO is not || or && it might not have exactly 2 arguments.
       if (OO->getNumArgs() == 2) {
         Op = OO->getOperator();
         LHS = OO->getArg(0);
         RHS = OO->getArg(1);
+        Loc = OO->getOperatorLoc();
       }
     }
   }
 
   bool isAnd() const { return Op == OO_AmpAmp; }
   bool isOr() const { return Op == OO_PipePipe; }
   explicit operator bool() const { return isAnd() || isOr(); }
 
   const Expr *getLHS() const { return LHS; }
   const Expr *getRHS() const { return RHS; }
+
+  ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS) {
+    return recreateBinOp(SemaRef, LHS, const_cast<Expr *>(getRHS()));
+  }
+
+  ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS, ExprResult RHS) {
+    assert((isAnd() || isOr()) && "Not the right kind of op?");
+    assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?");
+    assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?");
+    // We should just be able to 'normalize' these to the builtin Binary
+    // Operator, since that is how they are evaluated in constriant checks.
+    return BinaryOperator::Create(SemaRef.Context, LHS.get(), RHS.get(),
+                                  BinaryOperator::getOverloadedOpcode(Op),
+                                  SemaRef.Context.BoolTy, VK_PRValue,
+                                  OK_Ordinary, Loc, FPOptionsOverride{});
+  }
 };
 }
 
 bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression,
                                      Token NextToken, bool *PossibleNonPrimary,
                                      bool IsTrailingRequiresClause) {
   // C++2a [temp.constr.atomic]p1
   // ..E shall be a constant expression of type bool.
bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression,
   }
 
   if (PossibleNonPrimary)
       *PossibleNonPrimary = false;
   return true;
 }
 
 template <typename AtomicEvaluator>
-static bool
+static ExprResult
 calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
                                 ConstraintSatisfaction &Satisfaction,
                                 AtomicEvaluator &&Evaluator) {
   ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts();
 
   if (LogicalBinOp BO = ConstraintExpr) {
-    if (calculateConstraintSatisfaction(S, BO.getLHS(), Satisfaction,
-                                        Evaluator))
-      return true;
+    ExprResult LHSRes = calculateConstraintSatisfaction(
+        S, BO.getLHS(), Satisfaction, Evaluator);
+
+    if (LHSRes.isInvalid())
+      return ExprError();
 
     bool IsLHSSatisfied = Satisfaction.IsSatisfied;
 
     if (BO.isOr() && IsLHSSatisfied)
       // [temp.constr.op] p3
       //    A disjunction is a constraint taking two operands. To determine if
       //    a disjunction is satisfied, the satisfaction of the first operand
       //    is checked. If that is satisfied, the disjunction is satisfied.
       //    Otherwise, the disjunction is satisfied if and only if the second
       //    operand is satisfied.
-      return false;
+      return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty();
 
     if (BO.isAnd() && !IsLHSSatisfied)
       // [temp.constr.op] p2
       //    A conjunction is a constraint taking two operands. To determine if
       //    a conjunction is satisfied, the satisfaction of the first operand
       //    is checked. If that is not satisfied, the conjunction is not
       //    satisfied. Otherwise, the conjunction is satisfied if and only if
       //    the second operand is satisfied.
-      return false;
+      return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty();
 
-    return calculateConstraintSatisfaction(
+    ExprResult RHSRes = calculateConstraintSatisfaction(
         S, BO.getRHS(), Satisfaction, std::forward<AtomicEvaluator>(Evaluator));
+    if (RHSRes.isInvalid())
+      return ExprError();
+
+    if (!LHSRes.isUsable() || !RHSRes.isUsable())
+      return ExprEmpty();
+    return BO.recreateBinOp(S, LHSRes, RHSRes);
   } else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr)) {
-    return calculateConstraintSatisfaction(S, C->getSubExpr(), Satisfaction,
+    // These aren't evaluated, so we don't care about cleanups, so we can just
+    // evaluate these as if the cleanups didn't exist.
+    return calculateConstraintSatisfaction(
+        S, C->getSubExpr(), Satisfaction,
         std::forward<AtomicEvaluator>(Evaluator));
   }
 
   // An atomic constraint expression
   ExprResult SubstitutedAtomicExpr = Evaluator(ConstraintExpr);
 
   if (SubstitutedAtomicExpr.isInvalid())
-    return true;
+    return ExprError();
 
   if (!SubstitutedAtomicExpr.isUsable())
     // Evaluator has decided satisfaction without yielding an expression.
-    return false;
+    return ExprEmpty();
 
   EnterExpressionEvaluationContext ConstantEvaluated(
       S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   SmallVector<PartialDiagnosticAt, 2> EvaluationDiags;
   Expr::EvalResult EvalResult;
   EvalResult.Diag = &EvaluationDiags;
   if (!SubstitutedAtomicExpr.get()->EvaluateAsConstantExpr(EvalResult,
                                                            S.Context) ||
       !EvaluationDiags.empty()) {
     // C++2a [temp.constr.atomic]p1
     //   ...E shall be a constant expression of type bool.
     S.Diag(SubstitutedAtomicExpr.get()->getBeginLoc(),
            diag::err_non_constant_constraint_expression)
         << SubstitutedAtomicExpr.get()->getSourceRange();
     for (const PartialDiagnosticAt &PDiag : EvaluationDiags)
       S.Diag(PDiag.first, PDiag.second);
-    return true;
+    return ExprError();
   }
 
   assert(EvalResult.Val.isInt() &&
          "evaluating bool expression didn't produce int");
   Satisfaction.IsSatisfied = EvalResult.Val.getInt().getBoolValue();
   if (!Satisfaction.IsSatisfied)
     Satisfaction.Details.emplace_back(ConstraintExpr,
                                       SubstitutedAtomicExpr.get());
 
-  return false;
+  return SubstitutedAtomicExpr;
 }
 
-static bool calculateConstraintSatisfaction(
-    Sema &S, const NamedDecl *Template, ArrayRef<TemplateArgument> TemplateArgs,
-    SourceLocation TemplateNameLoc, MultiLevelTemplateArgumentList &MLTAL,
-    const Expr *ConstraintExpr, ConstraintSatisfaction &Satisfaction) {
+static ExprResult calculateConstraintSatisfaction(
+    Sema &S, const NamedDecl *Template, SourceLocation TemplateNameLoc,
+    const MultiLevelTemplateArgumentList &MLTAL, const Expr *ConstraintExpr,
+    ConstraintSatisfaction &Satisfaction) {
   return calculateConstraintSatisfaction(
       S, ConstraintExpr, Satisfaction, [&](const Expr *AtomicExpr) {
         EnterExpressionEvaluationContext ConstantEvaluated(
             S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
 
         // Atomic constraint - substitute arguments and check satisfaction.
         ExprResult SubstitutedExpression;
         {
           TemplateDeductionInfo Info(TemplateNameLoc);
           Sema::InstantiatingTemplate Inst(S, AtomicExpr->getBeginLoc(),
               Sema::InstantiatingTemplate::ConstraintSubstitution{},
               const_cast<NamedDecl *>(Template), Info,
               AtomicExpr->getSourceRange());
           if (Inst.isInvalid())
             return ExprError();
           // We do not want error diagnostics escaping here.
           Sema::SFINAETrap Trap(S);
-          SubstitutedExpression = S.SubstExpr(const_cast<Expr *>(AtomicExpr),
-                                              MLTAL);
+          SubstitutedExpression =
+              S.SubstConstraintExpr(const_cast<Expr *>(AtomicExpr), MLTAL);
           // Substitution might have stripped off a contextual conversion to
           // bool if this is the operand of an '&&' or '||'. For example, we
           // might lose an lvalue-to-rvalue conversion here. If so, put it back
           // before we try to evaluate.
           if (!SubstitutedExpression.isInvalid())
             SubstitutedExpression =
                 S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
           if (SubstitutedExpression.isInvalid() || Trap.hasErrorOccurred()) {
return calculateConstraintSatisfaction(
 
         if (!S.CheckConstraintExpression(SubstitutedExpression.get()))
           return ExprError();
 
         return SubstitutedExpression;
       });
 }
 
-static bool CheckConstraintSatisfaction(Sema &S, const NamedDecl *Template,
-                                        ArrayRef<const Expr *> ConstraintExprs,
-                                        ArrayRef<TemplateArgument> TemplateArgs,
-                                        SourceRange TemplateIDRange,
-                                        ConstraintSatisfaction &Satisfaction) {
+static bool CheckConstraintSatisfaction(
+    Sema &S, const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
+    llvm::SmallVectorImpl<Expr *> &Converted,
+    const MultiLevelTemplateArgumentList &TemplateArgsList,
+    SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction) {
   if (ConstraintExprs.empty()) {
     Satisfaction.IsSatisfied = true;
     return false;
   }
 
-  for (auto& Arg : TemplateArgs)
-    if (Arg.isInstantiationDependent()) {
-      // No need to check satisfaction for dependent constraint expressions.
-      Satisfaction.IsSatisfied = true;
-      return false;
-    }
+  if (TemplateArgsList.isAnyArgInstantiationDependent()) {
+    //  No need to check satisfaction for dependent constraint expressions.
 
-  Sema::InstantiatingTemplate Inst(S, TemplateIDRange.getBegin(),
+  ArrayRef<TemplateArgument> TemplateArgs =
+      TemplateArgsList.getNumSubstitutedLevels() > 0
+          ? TemplateArgsList.getOutermost()
+          : ArrayRef<TemplateArgument>{};
+
+  Sema::InstantiatingTemplate Inst(
+      S, TemplateIDRange.getBegin(),
       Sema::InstantiatingTemplate::ConstraintsCheck{},
       const_cast<NamedDecl *>(Template), TemplateArgs, TemplateIDRange);
   if (Inst.isInvalid())
     return true;
 
-  MultiLevelTemplateArgumentList MLTAL;
-  MLTAL.addOuterTemplateArguments(TemplateArgs);
-
   for (const Expr *ConstraintExpr : ConstraintExprs) {
-    if (calculateConstraintSatisfaction(S, Template, TemplateArgs,
-                                        TemplateIDRange.getBegin(), MLTAL,
-                                        ConstraintExpr, Satisfaction))
+    ExprResult Res = calculateConstraintSatisfaction(
+        S, Template, TemplateIDRange.getBegin(), TemplateArgsList,
+        ConstraintExpr, Satisfaction);
+    if (Res.isInvalid())
       return true;
-    if (!Satisfaction.IsSatisfied)
-      // [temp.constr.op] p2
+
+    Converted.push_back(Res.get());
+    if (!Satisfaction.IsSatisfied) {
+      // Backfill the 'converted' list with nulls so we can keep the Converted
+      Converted.append(ConstraintExprs.size() - Converted.size(), nullptr);
+      // and unconverted lists in sync. [temp.constr.op] p2
       //   [...] To determine if a conjunction is satisfied, the satisfaction
       //   of the first operand is checked. If that is not satisfied, the
       //   conjunction is not satisfied. [...]
       return false;
     }
+  }
   return false;
 }
 
 bool Sema::CheckConstraintSatisfaction(
     const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
-    ArrayRef<TemplateArgument> TemplateArgs, SourceRange TemplateIDRange,
-    ConstraintSatisfaction &OutSatisfaction) {
+    llvm::SmallVectorImpl<Expr *> &ConvertedConstraints,
+    const MultiLevelTemplateArgumentList &TemplateArgsList,
+    SourceRange TemplateIDRange, ConstraintSatisfaction &OutSatisfaction) {
   if (ConstraintExprs.empty()) {
     OutSatisfaction.IsSatisfied = true;
     return false;
   }
 
+  ArrayRef<TemplateArgument> TemplateArgs =
+      TemplateArgsList.getNumSubstitutedLevels() > 0
+          ? TemplateArgsList.getOutermost()
+          : ArrayRef<TemplateArgument>{};
+
   llvm::FoldingSetNodeID ID;
   void *InsertPos;
   ConstraintSatisfaction *Satisfaction = nullptr;
   bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
   if (ShouldCache) {
     ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
     Satisfaction = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos);
     if (Satisfaction) {
       OutSatisfaction = *Satisfaction;
       return false;
     }
     Satisfaction = new ConstraintSatisfaction(Template, TemplateArgs);
   } else {
     Satisfaction = &OutSatisfaction;
   }
   if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
-                                    TemplateArgs, TemplateIDRange,
-                                    *Satisfaction)) {
+                                    ConvertedConstraints, TemplateArgsList,
+                                    TemplateIDRange, *Satisfaction)) {
     if (ShouldCache)
       delete Satisfaction;
     return true;
   }
 
   if (ShouldCache) {
     // We cannot use InsertNode here because CheckConstraintSatisfaction might
     // have invalidated it.
     SatisfactionCache.InsertNode(Satisfaction);
     OutSatisfaction = *Satisfaction;
   }
   return false;
 }
 
 bool Sema::CheckConstraintSatisfaction(const Expr *ConstraintExpr,
                                        ConstraintSatisfaction &Satisfaction) {
   return calculateConstraintSatisfaction(
              *this, ConstraintExpr, Satisfaction,
              [](const Expr *AtomicExpr) -> ExprResult {
                return ExprResult(const_cast<Expr *>(AtomicExpr));
-      });
+             })
+      .isInvalid();
+}
+
+bool Sema::SetupConstraintScope(
+    FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
+    MultiLevelTemplateArgumentList MLTAL, LocalInstantiationScope &Scope) {
+  if (FD->isTemplateInstantiation() && FD->getPrimaryTemplate()) {
+    FunctionTemplateDecl *PrimaryTemplate = FD->getPrimaryTemplate();
+    InstantiatingTemplate Inst(
+        *this, FD->getPointOfInstantiation(),
+        Sema::InstantiatingTemplate::ConstraintsCheck{}, PrimaryTemplate,
+        TemplateArgs ? *TemplateArgs : ArrayRef<TemplateArgument>{},
+        SourceRange());
+    if (Inst.isInvalid())
+      return true;
+
+    // addInstantiatedParametersToScope creates a map of 'uninstantiated' to
+    // 'instantiated' parameters and adds it to the context. For the case where
+    // this function is a template being instantiated NOW, we also need to add
+    // the list of current template arguments to the list so that they also can
+    // be picked out of the map.
+    if (TemplateArgs) {
+      MultiLevelTemplateArgumentList JustTemplArgs(
+          *FD->getTemplateSpecializationArgs());
+      if (addInstantiatedParametersToScope(
+              FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs))
+        return true;
+    }
+
+    // If this is a member function, make sure we get the parameters that
+    // reference the original primary template.
+    if (const auto *FromMemTempl =
+            PrimaryTemplate->getInstantiatedFromMemberTemplate()) {
+      if (addInstantiatedParametersToScope(FD, FromMemTempl->getTemplatedDecl(),
+                                           Scope, MLTAL))
+        return true;
+    }
+  } else if (FD->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization) {
+    InstantiatingTemplate Inst(*this, FD->getPointOfInstantiation(),
+                               Sema::InstantiatingTemplate::ConstraintsCheck{},
+                               FD->getInstantiatedFromMemberFunction(),
+                               TemplateArgs ? *TemplateArgs
+                                            : ArrayRef<TemplateArgument>{},
+                               SourceRange());
+    if (Inst.isInvalid())
+      return true;
+
+    // Case where this was not a template, but instantiated as a child-function.
+    if (addInstantiatedParametersToScope(
+            FD, FD->getInstantiatedFromMemberFunction(), Scope, MLTAL))
+      return true;
+  } else if (FD->getTemplatedKind() == FunctionDecl::TK_DependentNonTemplate) {
+
+    InstantiatingTemplate Inst(*this, FD->getPointOfInstantiation(),
+                               Sema::InstantiatingTemplate::ConstraintsCheck{},
+                               FD->getInstantiatedFromDecl(),
+                               TemplateArgs ? *TemplateArgs
+                                            : ArrayRef<TemplateArgument>{},
+                               SourceRange());
+    if (Inst.isInvalid())
+      return true;
+    // case where this is a function declared inside of another.
+    if (addInstantiatedParametersToScope(FD, FD->getInstantiatedFromDecl(),
+                                         Scope, MLTAL))
+      return true;
+  }
+  return false;
+}
+
+// This function collects all of the template arguments for the purposes of
+// constraint-instantiation and checking.
+llvm::Optional<MultiLevelTemplateArgumentList>
+Sema::SetupConstraintCheckingTemplateArgumentsAndScope(
+    FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
+    LocalInstantiationScope &Scope) {
+  MultiLevelTemplateArgumentList MLTAL;
+
+  // Collect the list of template arguments relative to the 'primary' template.
+  // We need the entire list, since the constraint is completely uninstantiated
+  // at this point.
+  MLTAL = getTemplateInstantiationArgs(FD, nullptr, /*RelativeToPrimary*/ true,
+                                       /*Pattern*/ nullptr,
+                                       /*LookBeyondLambda*/ true);
+  if (SetupConstraintScope(FD, TemplateArgs, MLTAL, Scope))
+    return {};
+
+  return MLTAL;
 }
 
 bool Sema::CheckFunctionConstraints(const FunctionDecl *FD,
                                     ConstraintSatisfaction &Satisfaction,
                                     SourceLocation UsageLoc) {
-  const Expr *RC = FD->getTrailingRequiresClause();
-  if (RC->isInstantiationDependent()) {
+  // Don't check constraints if the function is dependent. Also don't check if
+  // this is a function template specialization, as the call to
+  // CheckinstantiatedFunctionTemplateConstraints after this will check it
+  // better.
+  if (FD->isDependentContext() ||
+      FD->getTemplatedKind() ==
+          FunctionDecl::TK_FunctionTemplateSpecialization) {
     Satisfaction.IsSatisfied = true;
     return false;
   }
+
+  ContextRAII SavedContext{
+      *this, cast<DeclContext>(
+                 const_cast<FunctionDecl *>(FD)->getNonClosureContext())};
+  LocalInstantiationScope Scope(*this, true);
+  llvm::Optional<MultiLevelTemplateArgumentList> MLTAL =
+      SetupConstraintCheckingTemplateArgumentsAndScope(
+          const_cast<FunctionDecl *>(FD), {}, Scope);
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast<CXXMethodDecl>(FD)) {
     ThisQuals = Method->getMethodQualifiers();
     Record = const_cast<CXXRecordDecl *>(Method->getParent());
   }
   CXXThisScopeRAII ThisScope(*this, Record, ThisQuals, Record != nullptr);
   // We substitute with empty arguments in order to rebuild the atomic
   // constraint in a constant-evaluated context.
   // FIXME: Should this be a dedicated TreeTransform?
-  return CheckConstraintSatisfaction(
-      FD, {RC}, /*TemplateArgs=*/{},
+  const Expr *RC = FD->getTrailingRequiresClause();
+  llvm::SmallVector<Expr *, 1> Converted;
+
+  if (CheckConstraintSatisfaction(
+          FD, {RC}, Converted, *MLTAL,
           SourceRange(UsageLoc.isValid() ? UsageLoc : FD->getLocation()),
-      Satisfaction);
+          Satisfaction))
+    return true;
+
+  // FIXME: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we
+  // seem to always just pick up the constraints from the primary template.
+  assert(Converted.size() <= 1 && "Got more expressions converted?");
+  if (!Converted.empty() && Converted[0] != nullptr)
+    const_cast<FunctionDecl *>(FD)->setTrailingRequiresClause(Converted[0]);
+  return false;
 }
 
 bool Sema::EnsureTemplateArgumentListConstraints(
-    TemplateDecl *TD, ArrayRef<TemplateArgument> TemplateArgs,
+    TemplateDecl *TD, MultiLevelTemplateArgumentList TemplateArgs,
     SourceRange TemplateIDRange) {
   ConstraintSatisfaction Satisfaction;
   llvm::SmallVector<const Expr *, 3> AssociatedConstraints;
   TD->getAssociatedConstraints(AssociatedConstraints);
   if (CheckConstraintSatisfaction(TD, AssociatedConstraints, TemplateArgs,
                                   TemplateIDRange, Satisfaction))
     return true;
 
   if (!Satisfaction.IsSatisfied) {
     SmallString<128> TemplateArgString;
     TemplateArgString = " ";
     TemplateArgString += getTemplateArgumentBindingsText(
-        TD->getTemplateParameters(), TemplateArgs.data(), TemplateArgs.size());
+        TD->getTemplateParameters(), TemplateArgs.getInnermost().data(),
+        TemplateArgs.getInnermost().size());
 
     Diag(TemplateIDRange.getBegin(),
          diag::err_template_arg_list_constraints_not_satisfied)
         << (int)getTemplateNameKindForDiagnostics(TemplateName(TD)) << TD
         << TemplateArgString << TemplateIDRange;
     DiagnoseUnsatisfiedConstraint(Satisfaction);
     return true;
   }
if (TemplateAC.empty()) {
     return false;
   }
 
   // Enter the scope of this instantiation. We don't use
   // PushDeclContext because we don't have a scope.
   Sema::ContextRAII savedContext(*this, Decl);
   LocalInstantiationScope Scope(*this);
 
-  // If this is not an explicit specialization - we need to get the instantiated
-  // version of the template arguments and add them to scope for the
-  // substitution.
-  if (Decl->isTemplateInstantiation()) {
-    InstantiatingTemplate Inst(*this, Decl->getPointOfInstantiation(),
-        InstantiatingTemplate::ConstraintsCheck{}, Decl->getPrimaryTemplate(),
-        TemplateArgs, SourceRange());
-    if (Inst.isInvalid())
-      return true;
-    MultiLevelTemplateArgumentList MLTAL(
-        *Decl->getTemplateSpecializationArgs());
-    if (addInstantiatedParametersToScope(
-            Decl, Decl->getPrimaryTemplate()->getTemplatedDecl(), Scope, MLTAL))
-      return true;
-  }
+  Optional<MultiLevelTemplateArgumentList> MLTAL =
+      SetupConstraintCheckingTemplateArgumentsAndScope(Decl, TemplateArgs,
+                                                       Scope);
+
+  if (!MLTAL)
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
     ThisQuals = Method->getMethodQualifiers();
     Record = Method->getParent();
   }
   CXXThisScopeRAII ThisScope(*this, Record, ThisQuals, Record != nullptr);
-  return CheckConstraintSatisfaction(Template, TemplateAC, TemplateArgs,
+  llvm::SmallVector<Expr *, 1> Converted;
+  return CheckConstraintSatisfaction(Template, TemplateAC, Converted, *MLTAL,
                                      PointOfInstantiation, Satisfaction);
 }
 
 static void diagnoseUnsatisfiedRequirement(Sema &S,
                                            concepts::ExprRequirement *Req,
                                            bool First) {
   assert(!Req->isSatisfied()
          && "Diagnose() can only be used on an unsatisfied requirement");

clang/lib/Sema/SemaTemplate.cpp

if (CheckTemplateArgumentList(NamedConcept, ConceptNameInfo.getLoc(),
                                 /*PartialTemplateArgs=*/false, Converted,
                                 /*UpdateArgsWithConversions=*/false))
     return ExprError();
 
   ConstraintSatisfaction Satisfaction;
   bool AreArgsDependent =
       TemplateSpecializationType::anyDependentTemplateArguments(*TemplateArgs,
                                                                 Converted);
+  MultiLevelTemplateArgumentList MLTAL;
+  MLTAL.addOuterTemplateArguments(Converted);
   if (!AreArgsDependent &&
       CheckConstraintSatisfaction(
-          NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+          NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
           SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
                       TemplateArgs->getRAngleLoc()),
           Satisfaction))
     return ExprError();
 
   return ConceptSpecializationExpr::Create(Context,
       SS.isSet() ? SS.getWithLocInContext(Context) : NestedNameSpecifierLoc{},
       TemplateKWLoc, ConceptNameInfo, FoundDecl, NamedConcept,
bool Sema::CheckTemplateArgument(NamedDecl *Param,
   {
     // Set up a template instantiation context.
     LocalInstantiationScope Scope(*this);
     InstantiatingTemplate Inst(*this, TemplateLoc, Template,
                                TempParm, Converted,
                                SourceRange(TemplateLoc, RAngleLoc));
     if (Inst.isInvalid())
       return true;
 
+    ConstraintEvalRAII EvalRAII(*this);
     TemplateArgumentList TemplateArgs(TemplateArgumentList::OnStack, Converted);
     Params = SubstTemplateParams(Params, CurContext,
                                  MultiLevelTemplateArgumentList(TemplateArgs));
     if (!Params)
       return true;
   }
 
   // C++1z [temp.local]p1: (DR1004)
if (ArgIdx < NumArgs) {
     return true;
   }
 
   // No problems found with the new argument list, propagate changes back
   // to caller.
   if (UpdateArgsWithConversions)
     TemplateArgs = std::move(NewArgs);
 
-  if (!PartialTemplateArgs &&
-      EnsureTemplateArgumentListConstraints(
-        Template, Converted, SourceRange(TemplateLoc,
-                                         TemplateArgs.getRAngleLoc()))) {
+  if (!PartialTemplateArgs) {
+    TemplateArgumentList StackTemplateArgs(TemplateArgumentList::OnStack,
+                                           Converted);
+    MultiLevelTemplateArgumentList MLTAL = getTemplateInstantiationArgs(
+        Template, &StackTemplateArgs, /*RelativeToPrimary*/ true,
+        /*Pattern*/ nullptr,
+        /*LookBeyondLambda*/ true, /*IncludeContainingStruct*/ true);
+    if (EnsureTemplateArgumentListConstraints(
+            Template, MLTAL,
+            SourceRange(TemplateLoc, TemplateArgs.getRAngleLoc()))) {
       if (ConstraintsNotSatisfied)
         *ConstraintsNotSatisfied = true;
       return true;
     }
+  }
 
   return false;
 }
 
 namespace {
   class UnnamedLocalNoLinkageFinder
     : public TypeVisitor<UnnamedLocalNoLinkageFinder, bool>
   {
if (isTemplateTemplateParameterAtLeastAsSpecializedAs(Params, Template,
       //   [temp.constr.order].
       SmallVector<const Expr *, 3> ParamsAC, TemplateAC;
       Params->getAssociatedConstraints(ParamsAC);
       // C++2a[temp.arg.template]p3
       //   [...] In this comparison, if P is unconstrained, the constraints on A
       //   are not considered.
       if (ParamsAC.empty())
         return false;
+
       Template->getAssociatedConstraints(TemplateAC);
+
       bool IsParamAtLeastAsConstrained;
       if (IsAtLeastAsConstrained(Param, ParamsAC, Template, TemplateAC,
                                  IsParamAtLeastAsConstrained))
         return true;
       if (!IsParamAtLeastAsConstrained) {
         Diag(Arg.getLocation(),
              diag::err_template_template_parameter_not_at_least_as_constrained)
             << Template << Param << Arg.getSourceRange();
         Diag(Param->getLocation(), diag::note_entity_declared_at) << Param;
         Diag(Template->getLocation(), diag::note_entity_declared_at)
             << Template;
         MaybeEmitAmbiguousAtomicConstraintsDiagnostic(Param, ParamsAC, Template,
                                                       TemplateAC);
         return true;
       }
       return false;
     }
     // FIXME: Produce better diagnostics for deduction failures.
   }
 
   return !TemplateParameterListsAreEqual(Template->getTemplateParameters(),
                                          Params,
                                          true,
                                          TPL_TemplateTemplateArgumentMatch,

clang/lib/Sema/SemaTemplateDeduction.cpp

 
 template<typename TemplateDeclT>
 static Sema::TemplateDeductionResult
 CheckDeducedArgumentConstraints(Sema& S, TemplateDeclT *Template,
                                 ArrayRef<TemplateArgument> DeducedArgs,
                                 TemplateDeductionInfo& Info) {
   llvm::SmallVector<const Expr *, 3> AssociatedConstraints;
   Template->getAssociatedConstraints(AssociatedConstraints);
-  if (S.CheckConstraintSatisfaction(Template, AssociatedConstraints,
-                                    DeducedArgs, Info.getLocation(),
+  MultiLevelTemplateArgumentList MLTAL;
+  MLTAL.addOuterTemplateArguments(DeducedArgs);
+  if (S.CheckConstraintSatisfaction(Template, AssociatedConstraints, MLTAL,
+                                    Info.getLocation(),
                                     Info.AssociatedConstraintsSatisfaction) ||
       !Info.AssociatedConstraintsSatisfaction.IsSatisfied) {
     Info.reset(TemplateArgumentList::CreateCopy(S.Context, DeducedArgs));
     return Sema::TDK_ConstraintsNotSatisfied;
   }
   return Sema::TDK_Success;
 }
 
TemplateArgs.addArgument(
                               Deduced, TypeLoc.getNameLoc())));
   for (unsigned I = 0, C = TypeLoc.getNumArgs(); I != C; ++I)
     TemplateArgs.addArgument(TypeLoc.getArgLoc(I));
 
   llvm::SmallVector<TemplateArgument, 4> Converted;
   if (S.CheckTemplateArgumentList(Concept, SourceLocation(), TemplateArgs,
                                   /*PartialTemplateArgs=*/false, Converted))
     return Sema::DAR_FailedAlreadyDiagnosed;
+
+  MultiLevelTemplateArgumentList MLTAL;
+  MLTAL.addOuterTemplateArguments(Converted);
   if (S.CheckConstraintSatisfaction(Concept, {Concept->getConstraintExpr()},
-                                    Converted, TypeLoc.getLocalSourceRange(),
+                                    MLTAL, TypeLoc.getLocalSourceRange(),
                                     Satisfaction))
     return Sema::DAR_FailedAlreadyDiagnosed;
   if (!Satisfaction.IsSatisfied) {
     std::string Buf;
     llvm::raw_string_ostream OS(Buf);
     OS << "'" << Concept->getName();
     if (TypeLoc.hasExplicitTemplateArgs()) {
       printTemplateArgumentList(

clang/lib/Sema/SemaTemplateInstantiate.cpp

 /// arguments relative to the primary template, even when we're
 /// dealing with a specialization. This is only relevant for function
 /// template specializations.
 ///
 /// \param Pattern If non-NULL, indicates the pattern from which we will be
 /// instantiating the definition of the given declaration, \p D. This is
 /// used to determine the proper set of template instantiation arguments for
 /// friend function template specializations.
+///
+/// \param LookBeyondLambda Indicates that this collection of arguments should
+/// continue looking when it encounters a lambda generic call operator.
+///
+/// \param IncludeContainingStructArgs Indicates that this collection of
+/// arguments should include arguments for any class template that this
+/// declaration is included inside of.
+
 MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
     const NamedDecl *D, const TemplateArgumentList *Innermost,
-    bool RelativeToPrimary, const FunctionDecl *Pattern) {
+    bool RelativeToPrimary, const FunctionDecl *Pattern, bool LookBeyondLambda,
+    bool IncludeContainingStructArgs) {