This is an archive of the discontinued LLVM Phabricator instance.

Deferred Concept Instantiation Implementation
ClosedPublic

Authored by erichkeane on Feb 11 2022, 6:32 AM.

Details

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).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

clang/include/clang/Sema/Sema.h
6948–6950

Now the comment is not precise.

6967

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

clang/lib/Sema/SemaConcept.cpp
146

We should delete this one.

155

ditto

414–420

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

421

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 marked 5 inline comments as done.

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

erichkeane added inline comments.Mar 31 2022, 6:28 PM
clang/include/clang/Sema/Sema.h
6967

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
421

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.

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
421

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 updated this revision to Diff 419750.Apr 1 2022, 7:18 AM

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

erichkeane added inline comments.Apr 1 2022, 7:23 AM
clang/lib/Sema/SemaConcept.cpp
421

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 added inline comments.Apr 5 2022, 8:32 PM
clang/lib/Sema/SemaConcept.cpp
421

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

clang/lib/Sema/SemaTemplateInstantiate.cpp
57–61

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 added inline comments.Apr 6 2022, 6:37 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
57–61

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).

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
4692

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.

5551
7453–7468

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
57–61

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

erichkeane marked 8 inline comments as done.Apr 7 2022, 6:22 AM
erichkeane added inline comments.
clang/lib/Sema/SemaTemplate.cpp
4692

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

7453–7468

Ah, neat! Thanks for this. Done.

erichkeane updated this revision to Diff 421186.Apr 7 2022, 6:26 AM
erichkeane marked 2 inline comments as done.

Make suggestions from @ChuanqiXu

ChuanqiXu accepted this revision.Apr 7 2022, 7:20 PM

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

clang/lib/Sema/SemaConcept.cpp
420–421

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
4692

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.

This revision is now accepted and ready to land.Apr 7 2022, 7:20 PM
erichkeane updated this revision to Diff 421564.Apr 8 2022, 9:59 AM
erichkeane marked an inline comment as done.

rebase for CI + comment change requested.

erichkeane added a comment.EditedApr 19 2022, 6:25 AM

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
4692

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 :)

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 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.

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.

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.

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.

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.

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.

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 added inline comments.Apr 19 2022, 11:47 PM
clang/include/clang/AST/Decl.h
1891 ↗(On Diff #423713)

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 ↗(On Diff #423713)

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 ↗(On Diff #423713)

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 ↗(On Diff #423713)
clang/lib/AST/Decl.cpp
3787 ↗(On Diff #423713)

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

clang/lib/AST/DeclBase.cpp
299 ↗(On Diff #423713)

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

clang/lib/Sema/SemaConcept.cpp
467–475

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
4692

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

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2158

It looks better to add some comments.

erichkeane marked 10 inline comments as done.Apr 20 2022, 7:14 AM
erichkeane added inline comments.
clang/include/clang/AST/Decl.h
1891 ↗(On Diff #423713)

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 ↗(On Diff #423713)

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 ↗(On Diff #423713)

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 ↗(On Diff #423713)

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

clang/lib/AST/DeclBase.cpp
299 ↗(On Diff #423713)

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
467–475

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 marked 6 inline comments as done.

Thanks for the review @ChuanqiXu !

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.

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

erichkeane planned changes to this revision.Apr 20 2022, 10:06 AM

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
483

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 added inline comments.Apr 20 2022, 10:35 AM
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 added inline comments.Apr 20 2022, 10:44 AM
clang/lib/Sema/SemaConcept.cpp
483

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 added inline comments.Apr 20 2022, 11:02 AM
clang/lib/Sema/TreeTransform.h
13002

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

erichkeane added inline comments.Apr 20 2022, 12:27 PM
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.

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.

This revision is now accepted and ready to land.Apr 21 2022, 6:49 AM
erichkeane planned changes to this revision.Apr 21 2022, 6:49 AM
erichkeane added inline comments.
clang/test/SemaTemplate/concepts.cpp
391 ↗(On Diff #424183)

A bunch of the tests below this all fail.

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.

This revision is now accepted and ready to land.Apr 21 2022, 10:17 AM
erichkeane planned changes to this revision.Apr 21 2022, 10:18 AM
erichkeane added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
2317

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 ↗(On Diff #424183)

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

ChuanqiXu added inline comments.Apr 25 2022, 2:59 AM
clang/test/SemaTemplate/concepts.cpp
391 ↗(On Diff #424183)

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 added inline comments.Apr 25 2022, 7:08 AM
clang/test/SemaTemplate/concepts.cpp
391 ↗(On Diff #424183)

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.

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.

This revision is now accepted and ready to land.Apr 25 2022, 11:00 AM

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.

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.

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
2432–2440

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 ↗(On Diff #424963)

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

[]()
    requires(false)
{}();
erichkeane added inline comments.Apr 26 2022, 6:48 AM
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2432–2440

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 ↗(On Diff #424963)

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

I found

clang/test/SemaTemplate/concepts.cpp
386–388 ↗(On Diff #424963)

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.

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
399

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

erichkeane added inline comments.Apr 26 2022, 8:52 AM
clang/lib/Sema/SemaConcept.cpp
399

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.

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

OK, 1 issue left (see FriendFunc).

clang/test/SemaTemplate/concepts.cpp
436 ↗(On Diff #425275)

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 ↗(On Diff #425275)

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.

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.

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 ↗(On Diff #425275)

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..

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
67

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

321–323
354–359

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

412–456

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
2157

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
12934–12935

I think we could eliminate NewTrailingRequiresClause

clang/test/SemaTemplate/concepts.cpp
275–276 ↗(On Diff #425516)

Better to rename constraint to something like IsInt

erichkeane marked 11 inline comments as done.Apr 29 2022, 8:13 AM
erichkeane added inline comments.
clang/lib/Sema/SemaConcept.cpp
67

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.

412–456

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
2157

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 marked 3 inline comments as done.

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.

This revision was landed with ongoing or failed builds.May 2 2022, 6:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 6:07 AM
gulfem added a subscriber: gulfem.May 2 2022, 11:06 AM

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)
--

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.

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?

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!

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?

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.

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 this revision to Diff 427359.May 5 2022, 9:38 AM

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

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.

@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

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.

@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
67

Oh, my bad. It is a typo.

145
154
161–162

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.

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.

martong removed a subscriber: martong.May 23 2022, 4:09 AM