Re: [PATCH v4] c++: Add gnu::diagnose_as attribute

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9585 bytes --]

On 7/23/21 4:58 AM, Matthias Kretz wrote:
> Hi Jason,

Hi, thanks for your patience; I've been out on PTO a lot in the last 
month, and will be again this week.

> I found a few regressions from the last patch in the meantime. Version 4 of
> the patch is attached.
> 
> Questions:
> 
> 1. I simplified the condition for calling dump_template_parms in
> dump_function_name. !DECL_FRIEND_PSEUDO_TEMPLATE_INSTANTIATION (t) is
> equivalent to DECL_USE_TEMPLATE (t) in this context; implying that
> dump_template_parms is unconditionally called with `primary = false`. Or am I
> missing something?

Ah, good catch.  That suggests that 
DECL_FRIEND_PSEUDO_TEMPLATE_INSTANTIATION gives false positives; 
DECL_USE_TEMPLATE is also 0 for template patterns themselves, which 
would be why for

template <class T>
void f() { foo; }

the error refers to 'void f()' rather than 'void f<T>()'

The macro should probably also check DECL_FRIEND_CONTEXT.

And then the use of !DECL_USE_TEMPLATE is intending to check whether t 
is a primary template pattern, and pass true in that case.  This would 
be more correct if it also checked instantiates_primary_template_p.


> 2. Given a DECL_TI_ARGS tree, can I query whether an argument was deduced or
> explicitly specified? I'm asking because I still consider diagnostics of
> function templates unfortunate. `template <class T> void f()` is fine, as is
> `void f(T) [with T = float]`, but `void f() [with T = float]` could be better.
> I.e. if the template parameter appears somewhere in the function parameter
> list, dump_template_parms would only produce noise. If, however, the template
> parameter was given explicitly, it would be nice if it could show up
> accordingly in diagnostics.

NON_DEFAULT_TEMPLATE_ARGS_COUNT has that information, though there are 
some issues with it.  Attached is my WIP from May to improve it 
somewhat, if that's interesting.

> 3. When parsing tentatively and the parse is rejected, input_location is not
> reset, correct? In the attached patch I therefore made
> cp_parser_namespace_alias_definition reset input_location on a failed
> tentative parse. But it feels wrong. Shouldn't input_location be restored on
> cp_parser_parse_definitely?

Makes sense, I guess cp_lexer_rollback_tokens should call 
cp_lexer_set_source_position_from_token.

I'll look at the patch soon.

> --
> 
> This attribute overrides the diagnostics output string for the entity it
> appertains to. The motivation is to improve QoI for library TS
> implementations, where diagnostics have a very bad signal-to-noise ratio
> due to the long namespaces involved.
> 
> With the attribute, it is possible to solve PR89370 and make
> std::__cxx11::basic_string<_CharT, _Traits, _Alloc> appear as
> std::string in diagnostic output without extra hacks to recognize the
> type in the C++ frontend.
> 
> Signed-off-by: Matthias Kretz <m.kretz@gsi.de>
> 
> gcc/ChangeLog:
> 
>          PR c++/89370
>          * doc/extend.texi: Document the diagnose_as attribute.
>          * doc/invoke.texi: Document -fno-diagnostics-use-aliases.
> 
> gcc/c-family/ChangeLog:
> 
>          PR c++/89370
>          * c.opt (fdiagnostics-use-aliases): New diagnostics flag.
> 
> gcc/cp/ChangeLog:
> 
>          PR c++/89370
>          * cp-tree.h: Add is_alias_template_p declaration.
>          * decl2.c (is_alias_template_p): New function. Determines
>          whether a given TYPE_DECL is actually an alias template that is
>          still missing its template_info.
>          (is_late_template_attribute): Decls with diagnose_as attribute
>          are early attributes only if they are alias templates.
>          * error.c (dump_scope): When printing the name of a namespace,
>          look for the diagnose_as attribute. If found, print the
>          associated string instead of calling dump_decl.
>          (dump_decl_name_or_diagnose_as): New function to replace
>          dump_decl (pp, DECL_NAME(t), flags) and inspect the tree for the
>          diagnose_as attribute before printing the DECL_NAME.
>          (dump_template_scope): New function. Prints the scope of a
>          template instance correctly applying diagnose_as attributes and
>          adjusting the list of template parms accordingly.
>          (dump_aggr_type): If the type has a diagnose_as attribute, print
>          the associated string instead of printing the original type
>          name. Print template parms only if the attribute was not applied
>          to the instantiation / full specialization. Delay call to
>          dump_scope until the diagnose_as attribute is found. If the
>          attribute has a second argument, use it to override the context
>          passed to dump_scope.
>          (dump_simple_decl): Call dump_decl_name_or_diagnose_as instead
>          of dump_decl.
>          (dump_decl): Ditto.
>          (lang_decl_name): Ditto.
>          (dump_function_decl): Walk the functions context list to
>          determine whether a call to dump_template_scope is required.
>          Ensure function templates diagnosed with pretty templates set
>          TFF_TEMPLATE_NAME to skip dump_template_parms.
>          (dump_function_name): Replace the function's identifier with the
>          diagnose_as attribute value, if set. Expand
>          DECL_FRIEND_PSEUDO_TEMPLATE_INSTANTIATION to DECL_USE_TEMPLATE
>          and consequently call dump_template_parms with primary = false.
>          (comparable_template_types_p): Consider the types not a template
>          if one carries a diagnose_as attribute.
>          (print_template_differences): Replace the identifier with the
>          diagnose_as attribute value on the most general template, if it
>          is set.
>          * name-lookup.c (handle_namespace_attrs): Handle the diagnose_as
>          attribute on namespaces. Ensure exactly one string argument.
>          Ensure previous diagnose_as attributes used the same name.
>          'diagnose_as' on namespace aliases are forwarded to the original
>          namespace. Support no-argument 'diagnose_as' on namespace
>          aliases.
>          (do_namespace_alias): Add attributes parameter and call
>          handle_namespace_attrs.
>          * name-lookup.h (do_namespace_alias): Add attributes tree
>          parameter.
>          * parser.c (cp_parser_declaration): If the next token is
>          RID_NAMESPACE, tentatively parse a namespace alias definition.
>          If this fails expect a namespace definition.
>          (cp_parser_namespace_alias_definition): Allow optional
>          attributes before and after the identifier. Fast exit, restoring
>          input_location, if the expected CPP_EQ token is missing. Pass
>          attributes to do_namespace_alias.
>          * tree.c (cxx_attribute_table): Add diagnose_as attribute to the
>          table.
>          (check_diagnose_as_redeclaration): New function; copied and
>          adjusted from check_abi_tag_redeclaration.
>          (handle_diagnose_as_attribute): New function; copied and
>          adjusted from handle_abi_tag_attribute. If the given *node is a
>          TYPE_DECL: allow no argument to the attribute, using DECL_NAME
>          instead; apply the attribute to the type on the RHS in place,
>          even if the type is complete. Allow 2 arguments when called from
>          handle_diagnose_as_attribute. For type aliases, append
>          CP_DECL_CONTEXT as second attribute argument when the RHS type
>          has a different context. Warn about alias templates without
>          matching template arguments. Apply the attribute to the primary
>          template type for alias templates.
> 
> gcc/testsuite/ChangeLog:
> 
>          PR c++/89370
>          * g++.dg/diagnostic/diagnose-as1.C: New test.
>          * g++.dg/diagnostic/diagnose-as2.C: New test.
>          * g++.dg/diagnostic/diagnose-as3.C: New test.
>          * g++.dg/diagnostic/diagnose-as4.C: New test.
>          * g++.dg/diagnostic/diagnose-as5.C: New test.
>          * g++.dg/diagnostic/diagnose-as6.C: New test.
> ---
>   gcc/c-family/c.opt                            |   4 +
>   gcc/cp/cp-tree.h                              |   1 +
>   gcc/cp/decl2.c                                |  45 ++++
>   gcc/cp/error.c                                | 247 ++++++++++++++++--
>   gcc/cp/name-lookup.c                          |  52 +++-
>   gcc/cp/name-lookup.h                          |   2 +-
>   gcc/cp/parser.c                               |  41 +--
>   gcc/cp/tree.c                                 | 148 +++++++++++
>   gcc/doc/extend.texi                           |  45 ++++
>   gcc/doc/invoke.texi                           |   9 +-
>   .../g++.dg/diagnostic/diagnose-as1.C          | 213 +++++++++++++++
>   .../g++.dg/diagnostic/diagnose-as2.C          | 144 ++++++++++
>   .../g++.dg/diagnostic/diagnose-as3.C          | 152 +++++++++++
>   .../g++.dg/diagnostic/diagnose-as4.C          | 158 +++++++++++
>   .../g++.dg/diagnostic/diagnose-as5.C          |  21 ++
>   .../g++.dg/diagnostic/diagnose-as6.C          |  45 ++++
>   16 files changed, 1291 insertions(+), 36 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/diagnose-as1.C
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/diagnose-as2.C
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/diagnose-as3.C
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/diagnose-as4.C
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/diagnose-as5.C
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/diagnose-as6.C
> 


[-- Attachment #2: 0001-default.patch --]
[-- Type: text/x-patch, Size: 5751 bytes --]

From 857e349f43a59c88634664ec8635ae49dd7bd50f Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Thu, 27 May 2021 14:04:45 -0400
Subject: [PATCH] default
To: gcc-patches@gcc.gnu.org

---
 gcc/cp/error.c | 13 +++++++------
 gcc/cp/pt.c    | 44 +++++++++++++++++++++++++++-----------------
 2 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 9cb8bfc413e..cac4d3e2089 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -87,8 +87,6 @@ static void dump_exception_spec (cxx_pretty_printer *, tree, int);
 static void dump_template_argument (cxx_pretty_printer *, tree, int);
 static void dump_template_argument_list (cxx_pretty_printer *, tree, int);
 static void dump_template_parameter (cxx_pretty_printer *, tree, int);
-static void dump_template_bindings (cxx_pretty_printer *, tree, tree,
-                                    vec<tree, va_gc> *);
 static void dump_scope (cxx_pretty_printer *, tree, int);
 static void dump_template_parms (cxx_pretty_printer *, tree, int, int);
 static int get_non_default_template_args_count (tree, int);
@@ -375,7 +373,7 @@ dump_template_parameter (cxx_pretty_printer *pp, tree parm, int flags)
 
 static void
 dump_template_bindings (cxx_pretty_printer *pp, tree parms, tree args,
-                        vec<tree, va_gc> *typenames)
+			vec<tree, va_gc> *typenames, int flags)
 {
   /* Print "[with" and ']', conditional on whether anything is printed at all.
      This is tied to whether a semicolon is needed to separate multiple template
@@ -425,7 +423,8 @@ dump_template_bindings (cxx_pretty_printer *pp, tree parms, tree args,
       if (TMPL_ARGS_DEPTH (args) >= lvl)
 	lvl_args = TMPL_ARGS_LEVEL (args, lvl);
 
-      for (i = 0; i < TREE_VEC_LENGTH (p); ++i)
+      int len = get_non_default_template_args_count (lvl_args, flags);
+      for (i = 0; i < len; ++i)
 	{
 	  tree arg = NULL_TREE;
 
@@ -1663,7 +1662,8 @@ dump_substitution (cxx_pretty_printer *pp,
       && !(flags & TFF_NO_TEMPLATE_BINDINGS))
     {
       vec<tree, va_gc> *typenames = t ? find_typenames (t) : NULL;
-      dump_template_bindings (pp, template_parms, template_args, typenames);
+      dump_template_bindings (pp, template_parms, template_args, typenames,
+			      flags);
     }
 }
 
@@ -3449,7 +3449,8 @@ subst_to_string (tree p)
     return "";
 
   dump_template_decl (cxx_pp, TREE_PURPOSE (p), flags);
-  dump_substitution (cxx_pp, NULL, tparms, targs, /*flags=*/0);
+  dump_substitution (cxx_pp, NULL, tparms, targs,
+		     /*TFF_NO_OMIT_DEFAULT_TEMPLATE_ARGUMENTS*/0);
   return pp_ggc_formatted_text (cxx_pp);
 }
 
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 0870ccdc9f6..691f1b82535 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -2518,7 +2518,7 @@ determine_specialization (tree template_id,
   if (candidates)
     {
       tree fn = TREE_VALUE (candidates);
-      *targs_out = copy_node (DECL_TI_ARGS (fn));
+      *targs_out = copy_template_args (DECL_TI_ARGS (fn));
 
       /* Propagate the candidate's constraints to the declaration.  */
       if (tsk != tsk_template)
@@ -4816,11 +4816,16 @@ template_parms_level_to_args (tree parms)
 {
   tree a = copy_node (parms);
   TREE_TYPE (a) = NULL_TREE;
+  int nondefault = 0;
   for (int i = TREE_VEC_LENGTH (a) - 1; i >= 0; --i)
-    TREE_VEC_ELT (a, i) = template_parm_to_arg (TREE_VEC_ELT (a, i));
+    {
+      tree elt = TREE_VEC_ELT (a, i);
+      TREE_VEC_ELT (a, i) = template_parm_to_arg (elt);
+      if (!elt || elt == error_mark_node || !TREE_PURPOSE (elt))
+	++nondefault;
+    }
 
-  if (CHECKING_P)
-    SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (a, TREE_VEC_LENGTH (a));
+  SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (a, nondefault);
 
   return a;
 }
@@ -13276,8 +13281,9 @@ copy_template_args (tree t)
       TREE_VEC_ELT (new_vec, i) = elt;
     }
 
-  NON_DEFAULT_TEMPLATE_ARGS_COUNT (new_vec)
-    = NON_DEFAULT_TEMPLATE_ARGS_COUNT (t);
+  if (!TMPL_ARGS_HAVE_MULTIPLE_LEVELS (t))
+    NON_DEFAULT_TEMPLATE_ARGS_COUNT (new_vec)
+      = NON_DEFAULT_TEMPLATE_ARGS_COUNT (t);
 
   return new_vec;
 }
@@ -22339,9 +22345,17 @@ type_unification_real (tree tparms,
 	     be NULL_TREE or ERROR_MARK_NODE, so we do not need
 	     to explicitly check cxx_dialect here.  */
 	  if (TREE_PURPOSE (TREE_VEC_ELT (tparms, i)))
-	    /* OK, there is a default argument.  Wait until after the
-	       conversion check to do substitution.  */
-	    continue;
+	    {
+	      /* The position of the first default template argument,
+		 is also the number of non-defaulted arguments in TARGS.
+		 Record that.  */
+	      if (!NON_DEFAULT_TEMPLATE_ARGS_COUNT (targs))
+		SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (targs, i);
+
+	      /* OK, there is a default argument.  Wait until after the
+		 conversion check to do substitution.  */
+	      continue;
+	    }
 
 	  /* If the type parameter is a parameter pack, then it will
 	     be deduced to an empty parameter pack.  */
@@ -22444,14 +22458,7 @@ type_unification_real (tree tparms,
 	  if (arg == error_mark_node)
 	    return 1;
 	  else if (arg)
-	    {
-	      TREE_VEC_ELT (targs, i) = arg;
-	      /* The position of the first default template argument,
-		 is also the number of non-defaulted arguments in TARGS.
-		 Record that.  */
-	      if (!NON_DEFAULT_TEMPLATE_ARGS_COUNT (targs))
-		SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (targs, i);
-	    }
+	    TREE_VEC_ELT (targs, i) = arg;
 	}
 
       if (saw_undeduced++ == 1)
@@ -24821,6 +24828,9 @@ get_partial_spec_bindings (tree tmpl, tree spec_tmpl, tree args)
   if (!template_template_parm_bindings_ok_p (tparms, deduced_args))
     return NULL_TREE;
 
+  if (CHECKING_P)
+    SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (deduced_args, ntparms);
+
   return deduced_args;
 }
 
-- 
2.27.0