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