Alternatively, rather than passing the most general template + args to
push_tinst_level, we can pass the partially instantiated template +
innermost args via just:
gcc/cp/ChangeLog:
* constraint.cc (satisfy_declaration_constraints): Pass the
original T and ARGS to push_tinst_level.
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 2f1678ce4ff9..52768972da43 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -2704,6 +2704,8 @@ satisfy_declaration_constraints (tree t, sat_info info)
static tree
satisfy_declaration_constraints (tree t, tree args, sat_info info)
{
+ tree orig_t = t, orig_args = args;
+
/* Update the declaration for diagnostics. */
info.in_decl = t;
@@ -2732,7 +2734,7 @@ satisfy_declaration_constraints (tree t, tree args,
sat_info info)
tree result = boolean_true_node;
if (tree norm = get_normalized_constraints_from_decl (t, info.noisy ()))
{
- if (!push_tinst_level (t, args))
+ if (!push_tinst_level (orig_t, orig_args))
return result;
tree pattern = DECL_TEMPLATE_RESULT (t);
push_to_top_level ();
So that for diagnostic20.C in question we emit:
In substitution of '... void A<int>::f<U>() [with U = char]'.
compared to (with the previous approach)
In substitution of '... void A<T>::f<U>() [with U = char; T = int]'.
or (wrongly, with the status quo)
In substitution of '... void A<int>::f<U>() [with U = int]'
Would this be preferable? I'd be good with either.
On Wed, 9 Apr 2025, Patrick Palka wrote:
> On Wed, 9 Apr 2025, Patrick Palka wrote:
>
> > On Wed, 5 Mar 2025, Jason Merrill wrote:
> >
> > > On 3/5/25 10:13 AM, Patrick Palka wrote:
> > > > On Tue, 4 Mar 2025, Jason Merrill wrote:
> > > >
> > > > > On 3/4/25 2:49 PM, Patrick Palka wrote:
> > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
> > > > > > for trunk/14?
> > > > > >
> > > > > > -- >8 --
> > > > > >
> > > > > > In the three-parameter version of satisfy_declaration_constraints,
> > > > > > when
> > > > > > 't' isn't the most general template, then 't' won't correspond with
> > > > > > 'args' after we augment the latter via add_outermost_template_args,
> > > > > > and
> > > > > > so the instantiation context that we push via push_tinst_level isn't
> > > > > > quite correct: 'args' is a complete set of template arguments, but
> > > > > > 't'
> > > > > > is not necessarily the most general template. This manifests as
> > > > > > misleading diagnostic context lines when issuing a hard error (or a
> > > > > > constraint recursion error) that occurred during satisfaction, e.g.
> > > > > > for
> > > > > > the below testcase without this patch we emit:
> > > > > > In substitution of '... void A<int>::f<U>() [with U = int]'
> > > > > > and with this patch we emit:
> > > > > > In substitution of '... void A<T>::f<U>() [with U = char; T =
> > > > > > int]'.
> > > > > >
> > > > > > This patch fixes this by always passing the most general template to
> > > > > > push_tinst_level.
> > > > >
> > > > > That soungs good, but getting it by passing it back from
> > > > > get_normalized_constraints_from_decl seems confusing; I'd think we
> > > > > should
> > > > > calculate it in parallel to changing args to correspond to that
> > > > > template.
> > > >
> > > > Hmm, won't that mean duplicating the template adjustment logic in
> > > > get_normalized_constraints_from_decl, which seems undesirable? The
> > > > function has many callers, some of which are for satisfaction where
> > > > targs are involved, and the rest are for subsumption where no targs are
> > > > involved, so I don't see a clean way of refactoring the code to avoid
> > > > duplication of the template adjustment logic. Right now the targ
> > > > adjustment logic is unfortunately duplicated across both overloads
> > > > of satisfy_declaration_constraints and it seems undesirable to add
> > > > more duplication.
> > >
> > > Fair enough. Incidentally, I wonder why the two-parm overload doesn't
> > > call
> > > the three-parm overload?
> > >
> > > > Maybe one way to reduce the duplication would be to go the other way and
> > > > move the targ adjustment logic to get_normalized_constraints_from_decl
> > > > as well (so that it has two out-parameters, 'gen_d' and 'gen_args').
> > > > The proposed patch then would be an incremental step towards that.
> > >
> > > That makes sense, passing back something suitable for
> > > add_outermost_template_args.
> >
> > I tried combining the two overloads, and/or moving the targ adjustment
> > logic to get_normalized_constraints_from_decl, but I couldn't arrive at
> > a formulation that worked and I was happy with (i.e. didn't lead to more
> > code duplication than the original appproach).
> >
> > In the meantime I noticed that this bug is more pervasive than I
> > thought, and leads to wrong diagnostic context lines printed even in the
> > case of ordinary satisfaction failure -- however the wrong diagnostic
> > lines are more annoying/noticable during a hard error or constraint
> > recursion where there's likely no other useful diagnostic lines that
> > might have the correct args printed.
> >
> > So I adjusted the testcase in the original patch accordingly. Could the
> > following go in for now?
> >
> > I also attached a diff of the output of all our concepts testcases
> > currently, before/after this patch. Each change seems like a clear
> > improvement/correction to me.
>
> Oops, that was not a complete diff of all the concepts tests, here is a
> more complete one.
>
> >
> > -- >8 --
> >
> > Subject: [PATCH] c++: wrong targs in satisfaction diagnostic context line
> > [PR99214]
> >
> > In the three-parameter version of satisfy_declaration_constraints, when
> > 't' isn't the most general template, then 't' won't correspond with
> > 'args' after we augment the latter via add_outermost_template_args, and
> > so the instantiation context that we push via push_tinst_level isn't
> > quite correct: 'args' is a complete set of template arguments, but 't'
> > is not necessarily the most general template. This manifests as
> > misleading diagnostic context lines when issuing a satisfaction failure
> > error, e.g. the below testcase without this patch we emit:
> > In substitution of '... void A<int>::f<U>() [with U = int]'
> > and with this patch we emit:
> > In substitution of '... void A<T>::f<U>() [with U = char; T = int]'.
> >
> > This patch fixes this by always passing the most general template to
> > push_tinst_level.
> >
> > PR c++/99214
> >
> > gcc/cp/ChangeLog:
> >
> > * constraint.cc (get_normalized_constraints_from_decl): New
> > optional out-parameter GEN_D.
> > (satisfy_declaration_constraints): Use it to pass the most
> > general version of T to push_tinst_level.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/concepts/diagnostic20.C: New test.
> > ---
> > gcc/cp/constraint.cc | 15 +++++++++++----
> > gcc/testsuite/g++.dg/concepts/diagnostic20.C | 14 ++++++++++++++
> > 2 files changed, 25 insertions(+), 4 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic20.C
> >
> > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> > index a9caba8e2cc7..f688a99c5fd7 100644
> > --- a/gcc/cp/constraint.cc
> > +++ b/gcc/cp/constraint.cc
> > @@ -648,10 +648,13 @@ get_normalized_constraints_from_info (tree ci, tree
> > in_decl, bool diag = false)
> > return t;
> > }
> >
> > -/* Returns the normalized constraints for the declaration D. */
> > +/* Returns the normalized constraints for the declaration D.
> > + If GEN_D is non-NULL, sets *GEN_D to the most general version
> > + of D that ultimately owns its constraints. */
> >
> > static tree
> > -get_normalized_constraints_from_decl (tree d, bool diag = false)
> > +get_normalized_constraints_from_decl (tree d, bool diag = false,
> > + tree *gen_d = nullptr)
> > {
> > tree tmpl;
> > tree decl;
> > @@ -716,6 +719,8 @@ get_normalized_constraints_from_decl (tree d, bool diag
> > = false)
> > tmpl = most_general_template (tmpl);
> >
> > d = tmpl ? tmpl : decl;
> > + if (gen_d)
> > + *gen_d = d;
> >
> > /* If we're not diagnosing errors, use cached constraints, if any. */
> > if (!diag)
> > @@ -2730,9 +2735,11 @@ satisfy_declaration_constraints (tree t, tree args,
> > sat_info info)
> > return boolean_true_node;
> >
> > tree result = boolean_true_node;
> > - if (tree norm = get_normalized_constraints_from_decl (t, info.noisy ()))
> > + tree gen_t;
> > + if (tree norm = get_normalized_constraints_from_decl (t, info.noisy (),
> > + &gen_t))
> > {
> > - if (!push_tinst_level (t, args))
> > + if (!push_tinst_level (gen_t, args))
> > return result;
> > tree pattern = DECL_TEMPLATE_RESULT (t);
> > push_to_top_level ();
> > diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic20.C
> > b/gcc/testsuite/g++.dg/concepts/diagnostic20.C
> > new file mode 100644
> > index 000000000000..d88000b342c3
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/concepts/diagnostic20.C
> > @@ -0,0 +1,14 @@
> > +// PR c++/99214
> > +// { dg-do compile { target c++20 } }
> > +
> > +template <class T>
> > +struct A {
> > + template <class U> static void f() requires requires { T::fail; };
> > +};
> > +
> > +int main() {
> > + A<int>::f<char>(); // { dg-error "no match" }
> > +}
> > +
> > +// This matches the context line "In substitution of '... [with U = char;
> > T = int]'"
> > +// { dg-message "U = char; T = int" "" { target *-*-* } 0 }
> > --
> > 2.49.0.221.g485f5f8636
> >
