c++: memfn lookup consistency in incomplete class context

  When instantiating a call to a member function of a class template, we
repeat the member function lookup in order to obtain the corresponding
partially instantiated functions.  Within an incomplete-class context
however, we need to be more careful when repeating the lookup because we
don't want to introduce later-declared member functions that weren't
visible at template definition time.  We're currently not careful enough
in this respect, which causes us to reject memfn1.C below.

This patch fixes this issue by making tsubst_baselink filter out from
the instantiation-time lookup those member functions that were invisible
at template definition time.  This is really only necessary within an
incomplete-class context, so this patch adds a heuristic flag to BASELINK
to help us avoid needlessly performing this filtering step (which would
be a no-op) in complete-class contexts.

This is also necessary in order for the ahead-of-time pruning
implemented in r12-6075 to be effective for member functions within
class templates.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?

gcc/cp/ChangeLog:

	* call.c (build_new_method_call): Set
	BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P on the pruned baselink.
	* cp-tree.h (BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P): Define.
	* pt.c (filter_memfn_lookup): New subroutine of tsubst_baselink.
	(tsubst_baselink): Use filter_memfn_lookup on the new lookup
	result when BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P is set on the
	old baselink.  Remove redundant BASELINK_P check.
	* search.c (build_baselink): Set
	BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P appropriately.

gcc/testsuite/ChangeLog:

	* g++.dg/lookup/memfn1.C: New test.
	* g++.dg/template/non-dependent16b.C: New test.
---
 gcc/cp/call.c                                 |  1 +
 gcc/cp/cp-tree.h                              |  5 +
 gcc/cp/pt.c                                   | 91 ++++++++++++++++++-
 gcc/cp/search.c                               |  4 +
 gcc/testsuite/g++.dg/lookup/memfn1.C          | 16 ++++
 .../g++.dg/template/non-dependent16b.C        | 37 ++++++++
 6 files changed, 152 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/lookup/memfn1.C
 create mode 100644 gcc/testsuite/g++.dg/template/non-dependent16b.C
  

On 12/20/21 12:46, Patrick Palka wrote:
> When instantiating a call to a member function of a class template, we
> repeat the member function lookup in order to obtain the corresponding
> partially instantiated functions.  Within an incomplete-class context
> however, we need to be more careful when repeating the lookup because we
> don't want to introduce later-declared member functions that weren't
> visible at template definition time.  We're currently not careful enough
> in this respect, which causes us to reject memfn1.C below.
> 
> This patch fixes this issue by making tsubst_baselink filter out from
> the instantiation-time lookup those member functions that were invisible
> at template definition time.  This is really only necessary within an
> incomplete-class context, so this patch adds a heuristic flag to BASELINK
> to help us avoid needlessly performing this filtering step (which would
> be a no-op) in complete-class contexts.
> 
> This is also necessary in order for the ahead-of-time pruning
> implemented in r12-6075 to be effective for member functions within
> class templates.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> gcc/cp/ChangeLog:
> 
> 	* call.c (build_new_method_call): Set
> 	BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P on the pruned baselink.
> 	* cp-tree.h (BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P): Define.
> 	* pt.c (filter_memfn_lookup): New subroutine of tsubst_baselink.
> 	(tsubst_baselink): Use filter_memfn_lookup on the new lookup
> 	result when BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P is set on the
> 	old baselink.  Remove redundant BASELINK_P check.
> 	* search.c (build_baselink): Set
> 	BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P appropriately.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/lookup/memfn1.C: New test.
> 	* g++.dg/template/non-dependent16b.C: New test.
> ---
>   gcc/cp/call.c                                 |  1 +
>   gcc/cp/cp-tree.h                              |  5 +
>   gcc/cp/pt.c                                   | 91 ++++++++++++++++++-
>   gcc/cp/search.c                               |  4 +
>   gcc/testsuite/g++.dg/lookup/memfn1.C          | 16 ++++
>   .../g++.dg/template/non-dependent16b.C        | 37 ++++++++
>   6 files changed, 152 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/lookup/memfn1.C
>   create mode 100644 gcc/testsuite/g++.dg/template/non-dependent16b.C
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 1fbfc580a1e..bee367f57d7 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -11187,6 +11187,7 @@ build_new_method_call (tree instance, tree fns, vec<tree, va_gc> **args,
>   	    }
>   	  orig_fns = copy_node (orig_fns);
>   	  BASELINK_FUNCTIONS (orig_fns) = fn;
> +	  BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P (orig_fns) = true;
>   	}
>   
>   skip_prune:
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 8b5cfa230ba..4f175f491cb 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -464,6 +464,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
>         PACK_EXPANSION_SIZEOF_P (in *_PACK_EXPANSION)
>         OVL_USING_P (in OVERLOAD)
>         IMPLICIT_CONV_EXPR_NONTYPE_ARG (in IMPLICIT_CONV_EXPR)
> +      BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P (in BASELINK)
>      2: IDENTIFIER_KIND_BIT_2 (in IDENTIFIER_NODE)
>         ICS_THIS_FLAG (in _CONV)
>         DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (in VAR_DECL)
> @@ -1060,6 +1061,10 @@ struct GTY(()) tree_template_decl {
>   /* Nonzero if this baselink was from a qualified lookup.  */
>   #define BASELINK_QUALIFIED_P(NODE) \
>     TREE_LANG_FLAG_0 (BASELINK_CHECK (NODE))
> +/* Nonzero if the overload set for this baselink might be incomplete due
> +   to the lookup being performed from an incomplete class context.  */
> +#define BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P(NODE) \
> +  TREE_LANG_FLAG_1 (BASELINK_CHECK (NODE))
>   
>   struct GTY(()) tree_baselink {
>     struct tree_common common;
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 4f0ae6d5851..7293f4e7b12 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -16221,6 +16221,81 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>       }
>   }
>   
> +/* OLDFNS is a lookup set of member functions from some class template, and
> +   NEWFNS is a lookup set of member functions from a specialization of that
> +   class template.  Return the subset of NEWFNS which are specializations of
> +   some function from OLDFNS.  */
> +
> +static tree
> +filter_memfn_lookup (tree oldfns, tree newfns)
> +{
> +  /* Record all member functions from the old lookup set OLDFNS into
> +     VISIBLE_SET.  */
> +  hash_set<tree> visible_set;
> +  for (tree fn : lkp_range (oldfns))
> +    {
> +      if (TREE_CODE (fn) == USING_DECL)
> +	{
> +	  /* FIXME: Punt on (dependent) USING_DECL for now; mapping
> +	     a dependent USING_DECL to its instantiation seems
> +	     tricky.  */

I wouldn't expect it to be that bad; the instantiation would be any 
functions from bases of the class where the USING_DECL (and the lookup) 
occurs.  But that can be a follow-up; the patch is OK as is.

> +	  gcc_checking_assert (DECL_DEPENDENT_P (fn));
> +	  return newfns;
> +	}
> +      else if (TREE_CODE (fn) == TEMPLATE_DECL)
> +	/* A member function template.  */
> +	visible_set.add (fn);
> +      else if (TREE_CODE (fn) == FUNCTION_DECL)
> +	{
> +	  if (DECL_TEMPLATE_INFO (fn))
> +	    /* A non-template member function.  */
> +	    visible_set.add (DECL_TI_TEMPLATE (fn));
> +	  else
> +	    /* A non-template member function from a non-template base,
> +	       injected via a using-decl.  */
> +	    visible_set.add (fn);
> +	}
> +      else
> +	gcc_unreachable ();
> +    }
> +
> +  /* Returns true iff (a less specialized version of) FN appeared in
> +     the old lookup set OLDFNS.  */
> +  auto visible_p = [&visible_set] (tree fn) {
> +    if (TREE_CODE (fn) == FUNCTION_DECL
> +	&& !DECL_TEMPLATE_INFO (fn))
> +      return visible_set.contains (fn);
> +    else if (DECL_TEMPLATE_INFO (fn))
> +      return visible_set.contains (DECL_TI_TEMPLATE (fn));
> +    else
> +      gcc_unreachable ();
> +  };
> +
> +  bool lookup_changed_p = false;
> +  for (tree fn : lkp_range (newfns))
> +    if (!visible_p (fn))
> +      {
> +	lookup_changed_p = true;
> +	break;
> +      }
> +  if (!lookup_changed_p)
> +    return newfns;
> +
> +  /* Filter out from NEWFNS the member functions that weren't
> +     previously visible according to OLDFNS.  */
> +  unsigned filtered_size = 0;
> +  tree filtered_fns = NULL_TREE;
> +  for (tree fn : lkp_range (newfns))
> +    if (visible_p (fn))
> +      {
> +	filtered_fns = lookup_add (fn, filtered_fns);
> +	++filtered_size;
> +      }
> +  gcc_checking_assert (filtered_size == visible_set.elements ());
> +
> +  return filtered_fns;
> +}
> +
>   /* tsubst a BASELINK.  OBJECT_TYPE, if non-NULL, is the type of the
>      expression on the left-hand side of the "." or "->" operator.  We
>      only do the lookup if we had a dependent BASELINK.  Otherwise we
> @@ -16274,8 +16349,21 @@ tsubst_baselink (tree baselink, tree object_type,
>   	/* Treat as-if non-dependent below.  */
>   	dependent_p = false;
>   
> +      bool maybe_incomplete = BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P (baselink);
>         baselink = lookup_fnfields (qualifying_scope, name, /*protect=*/1,
>   				  complain);
> +      /* Filter out from the new lookup set those functions which didn't
> +	 appear in the original lookup set (in a less specialized form).
> +	 This is needed to preserve the consistency of member lookup performed
> +	 in an incomplete-class context, within which later-declared members
> +	 ought to remain invisible.  */
> +      if (maybe_incomplete)
> +	{
> +	  BASELINK_FUNCTIONS (baselink)
> +	    = filter_memfn_lookup (fns, BASELINK_FUNCTIONS (baselink));
> +	  BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P (baselink) = true;
> +	}
> +
>         if (!baselink)
>   	{
>   	  if ((complain & tf_error)
> @@ -16285,8 +16373,7 @@ tsubst_baselink (tree baselink, tree object_type,
>   	  return error_mark_node;
>   	}
>   
> -      if (BASELINK_P (baselink))
> -	fns = BASELINK_FUNCTIONS (baselink);
> +      fns = BASELINK_FUNCTIONS (baselink);
>       }
>     else
>       {
> diff --git a/gcc/cp/search.c b/gcc/cp/search.c
> index 943671acff8..b673db960e6 100644
> --- a/gcc/cp/search.c
> +++ b/gcc/cp/search.c
> @@ -1091,6 +1091,10 @@ build_baselink (tree binfo, tree access_binfo, tree functions, tree optype)
>     BASELINK_FUNCTIONS (baselink) = functions;
>     BASELINK_OPTYPE (baselink) = optype;
>   
> +  if (binfo == access_binfo
> +      && TYPE_BEING_DEFINED (BINFO_TYPE (access_binfo)))
> +    BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P (baselink) = true;
> +
>     return baselink;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/lookup/memfn1.C b/gcc/testsuite/g++.dg/lookup/memfn1.C
> new file mode 100644
> index 00000000000..1bf00119018
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/memfn1.C
> @@ -0,0 +1,16 @@
> +// Verify we preserve the consistency of member function lookup outside of a
> +// complete-class context.
> +// { dg-do compile { target c++11 } }
> +
> +template<class...>
> +struct A {
> +  template<class T> static int f(); // #1
> +  template<class T> static void g(decltype(f<T>()));
> +  template<class T> static int f(...); // #2
> +};
> +
> +int main() {
> +  A<>::g<int>(0); // OK, the later-declared #2 isn't considered when
> +		  // instantiating f<T>(), which would have otherwise
> +		  // led to ambiguity.
> +}
> diff --git a/gcc/testsuite/g++.dg/template/non-dependent16b.C b/gcc/testsuite/g++.dg/template/non-dependent16b.C
> new file mode 100644
> index 00000000000..eadaeb7f478
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/non-dependent16b.C
> @@ -0,0 +1,37 @@
> +// Like non-dependent16.C, but where A is a template.
> +
> +// { dg-do compile { target c++11 } }
> +
> +template<class...>
> +struct A {
> +  template<class T> static void f();
> +  template<class T> static auto f() -> decltype(f<void>(), 1, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 2, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 3, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 4, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 5, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 6, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 7, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 8, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 9, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 10, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 11, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 12, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 13, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 14, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 15, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 16, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 17, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 18, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 19, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 20, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 21, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 22, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 23, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 24, *T());
> +  template<class T> static auto f() -> decltype(f<void>(), 25, *T());
> +};
> +
> +int main() {
> +  A<>::f<void>();
> +}