Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Sema] Substitute parameter packs when deduced from function arguments #79371

Merged
merged 16 commits into from
Jan 27, 2024

Conversation

spaits
Copy link
Contributor

@spaits spaits commented Jan 24, 2024

This pull request would solve #78449 .
There is also a discussion about this on stackoverflow: https://stackoverflow.com/questions/77832658/stdtype-identity-to-support-several-variadic-argument-lists .

The following program is well formed:

#include <type_traits>

template <typename... T>
struct args_tag
{
    using type = std::common_type_t<T...>;
};

template <typename... T>
void bar(args_tag<T...>, std::type_identity_t<T>..., int, std::type_identity_t<T>...) {}

// example
int main() {
    bar(args_tag<int, int>{}, 4, 8, 15, 16, 23);
}

but Clang rejects it, while GCC and MSVC doesn't. The reason for this is that, in Sema::DeduceTemplateArguments we are not prepared for this case.

Substitution/deduction of parameter packs

The logic that handles substitution when we have explicit template arguments (SubstituteExplicitTemplateArguments) does not work here, since the types of the pack are not pushed to ParamTypes before the loop starts that does the deduction.
The other "candidate" that may could have handle this case would be the loop that does the deduction for trailing packs, but we are not dealing with trailing packs here.

Solution proposed in this PR

The solution proposed in this PR works similar to the trailing pack deduction. The main difference here is the end of the deduction cycle.
When a non-trailing template pack argument is found, whose type is not explicitly specified and the next type is not a pack type, the length of the previously deduced pack is retrieved (let that length be s). After that the next s arguments are processed in the same way as in the case of non trailing packs.

Another possible solution

There is another possible approach that would be less efficient. In the loop when we get to an element of ParamTypes that is a pack and could be substituted because the type is deduced from a previous argument, then s number of arg types would be inserted before the current element of ParamTypes type. Then we would "cancel" the processing of the current element, first process the previously inserted elements and the after that re-process the current element.
Basically we would do what SubstituteExplicitTemplateArguments does but during deduction.

Adjusted test cases

In clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p1-0x.cpp there is a test case named test_pack_not_at_end that should work, but still does not. This test case is relevant because the note for the error message has changed.
This is what the test case looks like currently:

template<typename ...Types>
void pack_not_at_end(tuple<Types...>, Types... values, int); // expected-note {{<int *, double *> vs. <int, int>}}

void test_pack_not_at_end(tuple<int*, double*> t2) {
  pack_not_at_end(t2, 0, 0, 0); // expected-error {{no match}}
  // FIXME: Should the "original argument type must match deduced parameter
  // type" rule apply here?
  pack_not_at_end<int*, double*>(t2, 0, 0, 0); // ok
}

The previous note said (before my changes):

deduced conflicting types for parameter 'Types' (<int *, double *> vs. <>)

The current note says (after my changesand also clang 14 would say this if the pack was not trailing):

deduced conflicting types for parameter 'Types' (<int *, double *> vs. <int, int>)

GCC says:

error: no matching function for call to ‘pack_not_at_end(std::tuple<int*, double*>&, int, int, int)’
   70 |     pack_not_at_end(t2, 0, 0, 9); // expected-error {{no match}}

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 24, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 24, 2024

@llvm/pr-subscribers-clang

Author: Gábor Spaits (spaits)

Changes

This pull request would solve #78449 .
There is also a discussion about this on stackoverflow: https://stackoverflow.com/questions/77832658/stdtype-identity-to-support-several-variadic-argument-lists .

The following program is well formed:

#include &lt;type_traits&gt;

template &lt;typename... T&gt;
struct args_tag
{
    using type = std::common_type_t&lt;T...&gt;;
};

template &lt;typename... T&gt;
void bar(args_tag&lt;T...&gt;, std::type_identity_t&lt;T&gt;..., int, std::type_identity_t&lt;T&gt;...) {}

// example
int main() {
    bar(args_tag&lt;int, int&gt;{}, 4, 8, 15, 16, 23);
}

but Clang rejects it, while GCC and MSVC doesn't. The reason for this is that, in Sema::DeduceTemplateArguments we are not prepared for this case.

The logic that handles substitution when we have explicit template arguments does not work here, since the types of the pack are not pushed to ParamTypes before the loop starts that does the deduction.
The other "candidate" that would may handle this case would be the loop that does the deduction for trailing packs, but we are not dealing with trailing packs here.

The solution proposed in this PR works similar to the trailing pack deduction. The main difference here is the end of the deduction cycle.
When a non-trailing template pack argument is found, whose type is not explicitly specified and the next type is not a pack type, the length of the previously deduced pack is retrieved (let that length be s). After that the next s arguments are processed in the same way as in the case of non trailing packs.

There is another possible approach that would be less efficient. In the loop when we get to an element of ParamTypes that is a pack and could be substituted because the type is deduced from a previous argument, then s number of arg types would be inserted before the current element of ParamTypes type. Then we would "cancel" the processing of the current element, first process the previously inserted elements and the after that re-process the current element.
Basically we would do what SubstituteExplicitTemplateArguments does but during deduction.


Full diff: https://github.com/llvm/llvm-project/pull/79371.diff

1 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+62-1)
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index e9e7ab5bb6698a0..46fa9eece3747a2 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -730,6 +730,7 @@ class PackDeductionScope {
   void addPack(unsigned Index) {
     // Save the deduced template argument for the parameter pack expanded
     // by this pack expansion, then clear out the deduction.
+    DeducedFromEarlierParameter = !Deduced[Index].isNull();
     DeducedPack Pack(Index);
     Pack.Saved = Deduced[Index];
     Deduced[Index] = TemplateArgument();
@@ -858,6 +859,29 @@ class PackDeductionScope {
       Info.PendingDeducedPacks[Pack.Index] = Pack.Outer;
   }
 
+  std::optional<unsigned> getSavedPackSize(unsigned Index,
+                                           TemplateArgument Pattern) const {
+
+    SmallVector<UnexpandedParameterPack, 2> Unexpanded;
+    S.collectUnexpandedParameterPacks(Pattern, Unexpanded);
+    if (Unexpanded.size() == 0 ||
+        Packs[0].Saved.getKind() != clang::TemplateArgument::Pack)
+      return {};
+    unsigned PackSize = Packs[0].Saved.pack_size();
+
+    if (std::all_of(Packs.begin() + 1, Packs.end(),
+                    [&PackSize](auto P) {
+                      return P.Saved.getKind() == TemplateArgument::Pack &&
+                             P.Saved.pack_size() == PackSize;
+                    }))
+      return PackSize;
+    return {};
+  }
+
+  /// Determine whether this pack has already been deduced from a previous
+  /// argument.
+  bool isDeducedFromEarlierParameter() const {return DeducedFromEarlierParameter;}
+
   /// Determine whether this pack has already been partially expanded into a
   /// sequence of (prior) function parameters / template arguments.
   bool isPartiallyExpanded() { return IsPartiallyExpanded; }
@@ -970,7 +994,6 @@ class PackDeductionScope {
         NewPack = Pack.DeferredDeduction;
         Result = checkDeducedTemplateArguments(S.Context, OldPack, NewPack);
       }
-
       NamedDecl *Param = TemplateParams->getParam(Pack.Index);
       if (Result.isNull()) {
         Info.Param = makeTemplateParameter(Param);
@@ -1003,9 +1026,12 @@ class PackDeductionScope {
   unsigned PackElements = 0;
   bool IsPartiallyExpanded = false;
   bool DeducePackIfNotAlreadyDeduced = false;
+  bool DeducedFromEarlierParameter = false;
+
   /// The number of expansions, if we have a fully-expanded pack in this scope.
   std::optional<unsigned> FixedNumExpansions;
 
+
   SmallVector<DeducedPack, 2> Packs;
 };
 
@@ -4371,6 +4397,41 @@ Sema::TemplateDeductionResult Sema::DeduceTemplateArguments(
           // corresponding argument is a list?
           PackScope.nextPackElement();
         }
+      } else if (!IsTrailingPack && !PackScope.isPartiallyExpanded() &&
+                 PackScope.isDeducedFromEarlierParameter() &&
+                 !isa<PackExpansionType>(ParamTypes[ParamIdx + 1])) {
+        // [temp.deduct.general#3]
+        // When all template arguments have been deduced
+        // or obtained from default template arguments, all uses of template
+        // parameters in the template parameter list of the template are
+        // replaced with the corresponding deduced or default argument values
+        //
+        // If we have a trailing parameter pack, that has been deduced perviously
+        // we substitute the pack here in a similar fashion as seen above with
+        // the trailing parameter packs. The main difference here is that, in
+        // this case we are not processing all of the remaining arguments. We
+        // are only process as many arguments as much we have in the already
+        // deduced parameter.
+        SmallVector<UnexpandedParameterPack, 2> Unexpanded;
+        collectUnexpandedParameterPacks(ParamPattern, Unexpanded);
+        if (Unexpanded.size() == 0)
+          continue;
+
+        std::optional<unsigned> ArgPosAfterSubstitution =
+            PackScope.getSavedPackSize(getDepthAndIndex(Unexpanded[0]).second,
+                                       ParamPattern);
+        if (!ArgPosAfterSubstitution)
+          continue;
+
+        unsigned PackArgEnd = ArgIdx + *ArgPosAfterSubstitution;
+        for (; ArgIdx < PackArgEnd && ArgIdx < Args.size(); ArgIdx++) {
+          ParamTypesForArgChecking.push_back(ParamPattern);
+          if (auto Result = DeduceCallArgument(ParamPattern, ArgIdx,
+                                               /*ExplicitObjetArgument=*/false))
+            return Result;
+
+          PackScope.nextPackElement();
+        }
       }
     }
 

Copy link

github-actions bot commented Jan 24, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@@ -431,6 +442,17 @@ namespace deduction_after_explicit_pack {
i<int, int>(0, 1, 2, 3, 4, 5); // expected-error {{no match}}
}

template <typename... T>
void bar(args_tag<T...>, type_identity_t<T>..., int mid, type_identity_t<T>...) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens without the mid parameter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could work. It would cost only condition being removed. I deliberately put in the condition that disables this. Should I enable it?

I think enabling it would be standard compliant, but I played it safe and stuck to the example seen in the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i think we should support that, and add a test for it )as long as the standard says it should work, which i think it does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for this.

Copy link
Contributor

@cor3ntin cor3ntin Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test for

template <typename... Y, typename... T>
void foo2(args_tag<Y...>, args_tag<T...>, type_identity_t<T>..., type_identity_t<T>...) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test. It also works.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!
It looks like a good direction.

I left a few comments

Can you add a changelog entry? Thanks

@spaits
Copy link
Contributor Author

spaits commented Jan 25, 2024

Thank you @cor3ntin for reviewing my PR.

I addressed all of your comments.

  • I fixed the types.
  • I added support for deduction of non-trailing packs when there is no separator type.
  • Simplified the code. This simplification has affect on the code you asked about.

Could you please review my recent changes?

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the release note, and there is a typo in the PR entry that should be fixed.

Else just 1 nit.

clang/lib/Sema/SemaTemplateDeduction.cpp Outdated Show resolved Hide resolved
@spaits
Copy link
Contributor Author

spaits commented Jan 25, 2024

I don't see the release note, and there is a typo in the PR entry that should be fixed.

Else just 1 nit.

Oh sorry I forgot about the release note. I will update that.

@spaits spaits changed the title [Sema]Substitue parameter packs when deduced from function parameter [Sema] Substitue parameter packs when deduced from function arguments Jan 25, 2024
@spaits spaits changed the title [Sema] Substitue parameter packs when deduced from function arguments [Sema] Substitute parameter packs when deduced from function arguments Jan 25, 2024
@spaits
Copy link
Contributor Author

spaits commented Jan 25, 2024

@cor3ntin @erichkeane
Thank you for checking the updates on my PR.
I fixed the typo and added the release notes.
Also rebased the branch.

@erichkeane
Copy link
Collaborator

@cor3ntin @erichkeane Thank you for checking the updates on my PR. I fixed the typo and added the release notes. Also rebased the branch.

Looks like the release notes change failed CI.

@spaits
Copy link
Contributor Author

spaits commented Jan 25, 2024

@cor3ntin @erichkeane Thank you for checking the updates on my PR. I fixed the typo and added the release notes. Also rebased the branch.

Looks like the release notes change failed CI.

Not my changes. This was commited 3 hours ago: spaits@c92ad41 .
In this commit the author has forgot to close a `` .

Should I fix it for this commit?
Probably it will be fixed on master when this commit is ready to merge. So when rebasing that change will not be included in this commit.

@spaits
Copy link
Contributor Author

spaits commented Jan 26, 2024

It looks like libc++26 test suites are failing. Looking at the logs from the CI I can not really figure out what is the exact reason.
I could only deduce that the failing test case is libcxx/gdb/gdb_pretty_printer_test.sh.cpp. This case seems to be related to libcxx/selftest/convenience_substitutions/build_run.sh.cpp. I will try to figure out how to set up my environment to run these tests.

@spaits
Copy link
Contributor Author

spaits commented Jan 26, 2024

I did some more digging. I saw that the test in the CI fails with permission errors.

| No symbol table is loaded.  Use the "file" command.
| warning: opening /proc/PID/mem file for lwp 4103309.4103309 failed: Permission denied (13)
| Traceback (most recent call last):
|   File "/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-h5ngp-1/llvm-project/clang-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py", line 123, in <module>
|     gdb.execute("run")
| gdb.error: Warning:
| Cannot insert breakpoint 1.
| Cannot access memory at address 0x55555555637c

I thought that maybe the problem is that, the program to be debugged by dbg does not compile with the compiler that includes my changes. I decided to just compile the test file with my modified compiler. I copied the test file. Removed one include that is only needed for manual debugging and recreated the command used in the CI as much as I could. The command looks like this now:

/home/spaits/repo/llvm-project/build/bin/clang++ -pthread --target=x86_64-unknown-linux-gnu /home/spaits/cpp/deb.cpp -o /home/spaits/cpp/deb.out -std=c++26 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-noexcept-type -Wno-atomic-alignment -Wno-reserved-module-identifier -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_NONE -Werror=thread-safety -Wuser-defined-warnings -g   -lc++ -latomic
0 spaits@work-laptop:~/cpp$ 

Because of the GCC 13+ prereq. I did not build libc++ for my self, but used the one available on my machine.
This way the file has compiled successfully.

I will still try to run the tests myself.

Until that if you have any idea what should I, do what wen possibly wrong pleas share that with me.

Thanks.

Co-authored-by: cor3ntin <corentinjabot@gmail.com>
@erichkeane
Copy link
Collaborator

I did some more digging. I saw that the test in the CI fails with permission errors.

| No symbol table is loaded.  Use the "file" command.
| warning: opening /proc/PID/mem file for lwp 4103309.4103309 failed: Permission denied (13)
| Traceback (most recent call last):
|   File "/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-h5ngp-1/llvm-project/clang-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py", line 123, in <module>
|     gdb.execute("run")
| gdb.error: Warning:
| Cannot insert breakpoint 1.
| Cannot access memory at address 0x55555555637c

I thought that maybe the problem is that, the program to be debugged by dbg does not compile with the compiler that includes my changes. I decided to just compile the test file with my modified compiler. I copied the test file. Removed one include that is only needed for manual debugging and recreated the command used in the CI as much as I could. The command looks like this now:

/home/spaits/repo/llvm-project/build/bin/clang++ -pthread --target=x86_64-unknown-linux-gnu /home/spaits/cpp/deb.cpp -o /home/spaits/cpp/deb.out -std=c++26 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-noexcept-type -Wno-atomic-alignment -Wno-reserved-module-identifier -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_NONE -Werror=thread-safety -Wuser-defined-warnings -g   -lc++ -latomic
0 spaits@work-laptop:~/cpp$ 

Because of the GCC 13+ prereq. I did not build libc++ for my self, but used the one available on my machine. This way the file has compiled successfully.

I will still try to run the tests myself.

Until that if you have any idea what should I, do what wen possibly wrong pleas share that with me.

Thanks.

It could very well just be a transient error/something that failed and has since been fixed. See if it still happens after this pending build.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!
The changes looks good to me.
Will you need me to merge them on your behalf?

@spaits
Copy link
Contributor Author

spaits commented Jan 27, 2024

Thank you for reviewing.

Before merge we should take a look at the CI.

It still fails for libc++ 26 suite, with the same reason as before.

I wanted to reproduce the issue. I built libc++ with the clang++ I compiled:

cmake -S "../runtimes" -GNinja \
          -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" \
          -DLIBCXX_CXX_ABI=libcxxabi \
          -DCMAKE_BUILD_TYPE=Debug \
          -DLIBCXX_TEST_PARAMS="std=c++26" \
          -DLIBCXXABI_TEST_PARAMS="std=c++26" \
          -DCMAKE_CXX_COMPILER=/home/spaits/repo/llvm-project/build/bin/clang++ \
          -DLLVM_DIR=/home/spaits/repo/llvm-project/build/lib/cmake/llvm

Then I run the test:

 /home/spaits/repo/llvm-project/build/bin/llvm-lit -a /home/spaits/repo/llvm-project/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp

Everything worked fine for me.
I don't know why the CI always fails for my PR with the same reason. It looks like an env issue in the CI(it is talking about some permissions), but it can not be that since other PRs are getting green runs and mine fails always with the same reason for the same test.

@spaits
Copy link
Contributor Author

spaits commented Jan 27, 2024

@cor3ntin I see that you have CI runs that fail with exactly the same reason as my runs: https://buildkite.com/llvm-project/clang-ci/builds/10874#018d49c2-1224-4939-9430-0e5a2be796a9 .

@cor3ntin
Copy link
Contributor

It's unrelated to your changes. Configuration issue on the CI, probably

@spaits
Copy link
Contributor Author

spaits commented Jan 27, 2024

It's unrelated to your changes. Configuration issue on the CI, probably

Okay. Then I will merge this.

Thank you very much for reviewing my PR and helping me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants