On Sat, 13 Jul 2024, Seyed Sajad Kahani wrote:
> On Fri, 12 Jul 2024 at 21:31, Patrick Palka <ppa...@redhat.com> wrote:
> >
> > Interesting, thanks for the detailed write-up!
>
> Thank you very much.
>
> > FWIW it should be much easier to obtain the partially specialised
> > tmpl/args of a class/variable specialization since GCC 14, by looking
> > at TI_PARTIAL_INFO of DECL_TEMPLATE_INFO (if it exists).
> >
> > Maybe passing the TI_PARTIAL_INFO args to do_auto_deduction where
> > appropriate might fix this PR too?
>
> You are right, TI_PARTIAL_INFO does the job if it is used in decl.cc. I have
> done some basic tests and everything is fine with it too.
>
> > Your approach indeed seems like a nice simplification, and while it
> > won't change the behavior of the vast majority of code, eager
> > substitution into constraints versus waiting until satisfaction
> > can cause us to reject otherwise valid code in some edge cases.
> > Consider
> >
> > template<class T, class U>
> > concept C = __is_same(T, int) || __is_same(T, U);
> >
> > template<class T>
> > void f(T t) {
> > C<typename T::type> auto x = t;
> > }
> >
> > int main() {
> > f(0);
> > }
> >
> > With the eager approach we'd substitute T=int into 'typename T::type' during
> > instantiation of f<int>(), which fails and causes the program to be
> > ill-formed
> > since we're not in a SFINAE context.
> >
> > With the non-eager approach we pass the original constraint C<auto,
> > typename T::type>
> > and the full set of arguments to satisfaction, which instead substitutes and
> > evaluates each atom of the constraint in a SFINAE manner. And since the
> > first
> > term of the disjunction is satisfied, the entire constraint is satisfied,
> > and
> > there's no error.
> >
> > This is basically the motivation for deferring substitution into
> > constraints,
> > most importantly the associated constraints of template declaration.
> > Admittedly it's probably less important to do this for constrained autos,
> > but
> > IIUC we do so anyway for QoI/consistency. Clang and MSVC reject the above
> > testcase FWIW, indicating they do substitute into constrained autos eagerly.
> > I'm not sure if the standard is completely clear about what to do here.
> >
> > Jason would have more context, but IIUC we deliberately don't eagerly
> > substitute into constrained autos and accept the implementation
> > complexities that this approach entails.
>
> What I can understand from 13.5.4.1.4 of N4950 is that SFINAE is not applied
> to the concepts. As it is stated: "If any such substitution results in an
> invalid type or expression, the program is ill-formed; no diagnostic is
> required." Adding that the following code is ill-formed:
>
> template<typename T> concept A = T::value || true;
> template<typename U> concept B = A<U*>;
> template<typename V> concept C = B<V&>;
>
> That we are rejecting it, if we add `static_assert(C<int>);` or `C auto x =
> 1;`
> to the code.
> One could conclude that the following code is ill-formed too:
>
> template<typename T> concept A = T::value || true;
> template<typename U> concept B = A<U*>;
> static_assert(A<int&>);
I guess you mean B<int&> here?
>
> But we are accepting it. As far as I can understand, I cannot distinguish the
> difference between the two cases. I would appreciate if you could elaborate it
> for me.
Ah, that's because the substitution failure in the first example occurs
during constraint _normalization_, and in second example it occurs
during atomic constraint _satisfaction_. Substitution failure during
constraint normalization is indeed a hard error according to
https://eel.is/c++draft/temp.constr.normal#1.4.sentence-2 as you cited.
But substitution failure during satisfaction just causes the current
atomic constraint to silently evaluate to false.
>
> If SFINAE applies to the disjunctions of the atomic constraints, I can make a
> new patch to use TI_PARTIAL_INFO, as you suggested. Otherwise, in addition to
> these changes, we also need to modify constraint_satisfaction_value
> (constraints.cc) as well to raise an error for the static_assert and other
> usages of concept expressions as well.
A patch using TI_PARTIAL_INFO would be much appreciated :)
>
> On Fri, 12 Jul 2024 at 21:31, Patrick Palka <ppa...@redhat.com> wrote:
> >
> > Interesting, thanks for the detailed write-up!
> >
> > On Tue, 9 Jul 2024, Seyed Sajad Kahani wrote:
> >
> > > Hi.
> > >
> > > While investigating a fix for C++/PR115030 (a bug in constrained auto
> > > deduction), I was wondering why we are not substituting constraint args
> > > of an
> > > auto node in tsubst (pt.cc:16533). Instead, this substitution is delayed
> > > until
> > > do_auto_deduction (pt.cc), where we attempt to find the substituted args
> > > of the
> > > enclosing scope. At that point, it becomes difficult to find partially
> > > specialised args, as they are erased in the process.
> >
> > FWIW it should be much easier to obtain the partially specialised
> > tmpl/args of a class/variable specialization since GCC 14, by looking
> > at TI_PARTIAL_INFO of DECL_TEMPLATE_INFO (if it exists).
> >
> > Maybe passing the TI_PARTIAL_INFO args to do_auto_deduction where
> > appropriate might fix this PR too?
> >
> > > I propose modifying tsubst to eagerly substitute the constraint args of
> > > an auto node.
> > >
> > > By making this change, we would not need to provide outer_targs for
> > > do_auto_deduction in cases where tsubst has been called for the type,
> > > which
> > > covers most scenarios. However, we still need outer_targs for cases like:
> > >
> > > template <typename T, C<T> auto V>
> > > struct S { };
> > >
> > > Hence, outer_targs cannot be completely removed but will be set to
> > > TREE_NULL in
> > > all calls except the one in convert_template_argument (pt.cc:8788).
> > > Additionally, the tmpl argument of do_auto_deduction, which helps to
> > > provide
> > > outer args in the scope, will no longer be necessary and can be safely
> > > removed.
> > >
> > > Also, the trimming hack proposed in
> > > https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654724.html to address
> > > C++/PR114915 is no longer needed. We will add an assertion to ensure that
> > > the
> > > missing levels do not become negative.
> > >
> > > Substituting constraint arguments earlier will slightly alter error
> > > messages
> > > (see testsuite/g++.dg/cpp2a/concepts-placeholder3.C as an example).
> > >
> > > To summarise, I have made the following changes:
> > > - Modified tsubst to substitute the constraint args.
> > > - Fixed shallow checks for using the cache from TEMPLATE_TYPE_DESCENDANTS
> > > (pt.cc:16513).
> > > - Substituted the constraint args and the auto node itself, while
> > > retaining all
> > > other parameters as is (pt.cc:16533).
> > > - Removed now unnecessary code that attempted to find outer scope
> > > template info
> > > and args in various locations.
> > > - Updated the missing levels hack (pt.cc:31320) to work with the
> > > substituted
> > > constraints.
> > > - Used level instead of orig_level to find the missing levels.
> > > - Added an assertion for future safety.
> > >
> > > Details of these changes can be found in the patch "[PATCH v1] c++:
> > > Eagerly
> > > substitute auto constraint args in tsubst [PR115030]" that will be
> > > replied to
> > > this thread.
> > >
> > > This patch, in my opinion, improves code quality by removing an argument
> > > from
> > > do_auto_deduction and eliminating the out-of-scope task of finding
> > > outer_targs
> > > within do_auto_deduction. It simplifies the usage of do_auto_deduction and
> > > resolves C++/PR115030 without complex calculations for specialised args.
> > > It
> > > also enhances the consistency of tsubst behaviour by not leaving
> > > constraints
> > > un-substituted. However, these changes make args in
> > > constraints_satisfied_p
> > > (being called from do_auto_deduction) a bit misleading, as it will not
> > > carry
> > > the actual args of the constraint and can even be an empty vec.
> >
> > Your approach indeed seems like a nice simplification, and while it
> > won't change the behavior of the vast majority of code, eager
> > substitution into constraints versus waiting until satisfaction
> > can cause us to reject otherwise valid code in some edge cases.
> > Consider
> >
> > template<class T, class U>
> > concept C = __is_same(T, int) || __is_same(T, U);
> >
> > template<class T>
> > void f(T t) {
> > C<typename T::type> auto x = t;
> > }
> >
> > int main() {
> > f(0);
> > }
> >
> > With the eager approach we'd substitute T=int into 'typename T::type' during
> > instantiation of f<int>(), which fails and causes the program to be
> > ill-formed
> > since we're not in a SFINAE context.
> >
> > With the non-eager approach we pass the original constraint C<auto,
> > typename T::type>
> > and the full set of arguments to satisfaction, which instead substitutes and
> > evaluates each atom of the constraint in a SFINAE manner. And since the
> > first
> > term of the disjunction is satisfied, the entire constraint is satisfied,
> > and
> > there's no error.
> >
> > This is basically the motivation for deferring substitution into
> > constraints,
> > most importantly the associated constraints of template declaration.
> > Admittedly it's probably less important to do this for constrained autos,
> > but
> > IIUC we do so anyway for QoI/consistency. Clang and MSVC reject the above
> > testcase FWIW, indicating they do substitute into constrained autos eagerly.
> > I'm not sure if the standard is completely clear about what to do here.
> >
> > Jason would have more context, but IIUC we deliberately don't eagerly
> > substitute into constrained autos and accept the implementation
> > complexities that this approach entails.
> >
> > >
> > > I have added a testsuite for C++/PR115030, and as far as I have tested
> > > (only
> > > c++ dg.exp on x86_64-linux), there are no regressions.
> > >
> > > Extra:
> > > While doing this, I also realised that the hash function in ctp_hasher
> > > (for
> > > canonical type of template parameters) slightly differs from its equality
> > > function. Specifically, the equality function uses comptypes (typeck.cc)
> > > (with
> > > COMPARE_STRUCTURAL) and compares placeholder constraints (typeck.cc:1586),
> > > while the hash function ignores them (pt.cc:4528). As a result, we can
> > > have two
> > > types with equal hashes that are unequal. For example, assuming:
> > >
> > > template <typename T, typename U>
> > > concept C1 = ...
> > >
> > > template <typename T, typename U>
> > > concept C2 = ...
> > >
> > >
> > > "C1<U> auto" and "C2<V> auto" have the same hash value but are unequal.
> > > This
> > > issue does not cause any error (it is a hash collision and it is
> > > handled), but
> > > it can be avoided by using hash_placeholder_constraint
> > > (constraint.cc:1150).
> > >
> > > Therefore, I have made the following changes:
> > > - Fixed the hash function to calculate the hash of the constraint
> > > (pt.cc:4528).
> > > - Slightly modified the existing hash_placeholder_constraint
> > > (constraint.cc:1150) to accept the initial hash value as an argument.
> > >
> > > Details of these changes can be found in the patch "[PATCH v1] c++: Hash
> > > placeholder constraint in ctp_hasher" that will be replied to this email.
> > >
> > > Thanks. I really appreciate your comments and feedback on these proposed
> > > changes.
> > >
> > >
> >
>
>
