Page MenuHomePhabricator

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
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
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 ↗(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
2406–2414

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
2406–2414

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
472

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
472

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

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
12977–12978

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

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
70

Oh, my bad. It is a typo.

166
175
182–183

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