c++: Extend -Wdangling-reference for std::minmax

  This patch extends -Wdangling-reference to also warn for

  auto v = std::minmax(1, 2);

which dangles because this overload of std::minmax returns
a std::pair<const int&, const int&> where the two references are
bound to the temporaries created for the arguments of std::minmax.
This is a common footgun, also described at
<https://en.cppreference.com/w/cpp/algorithm/minmax> in Notes.

It works by extending do_warn_dangling_reference to also warn when the
function returns a std::pair<const T&, const T&>.  std_pair_ref_ref_p
is a new helper to check that.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

gcc/cp/ChangeLog:

	* call.cc (std_pair_ref_ref_p): New.
	(do_warn_dangling_reference): Also warn when the function returns
	std::pair<const T&, const T&>.  Recurse into TARGET_EXPR_INITIAL.
	(maybe_warn_dangling_reference): Don't return early if we're
	initializing a std_pair_ref_ref_p.

gcc/ChangeLog:

	* doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst:
	Extend the description of -Wdangling-reference.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wdangling-reference6.C: New test.
---
 gcc/cp/call.cc                                | 52 ++++++++++++++++---
 .../options-controlling-c++-dialect.rst       | 10 ++++
 .../g++.dg/warn/Wdangling-reference6.C        | 38 ++++++++++++++
 3 files changed, 94 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C


base-commit: 1cdfd0e5cd5fc1f493d0832ed65d31320f9585b7
  

On 11/9/22 15:56, Marek Polacek wrote:
> This patch extends -Wdangling-reference to also warn for
> 
>    auto v = std::minmax(1, 2);
> 
> which dangles because this overload of std::minmax returns
> a std::pair<const int&, const int&> where the two references are
> bound to the temporaries created for the arguments of std::minmax.
> This is a common footgun, also described at
> <https://en.cppreference.com/w/cpp/algorithm/minmax> in Notes.
> 
> It works by extending do_warn_dangling_reference to also warn when the
> function returns a std::pair<const T&, const T&>.  std_pair_ref_ref_p
> is a new helper to check that.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (std_pair_ref_ref_p): New.
> 	(do_warn_dangling_reference): Also warn when the function returns
> 	std::pair<const T&, const T&>.  Recurse into TARGET_EXPR_INITIAL.
> 	(maybe_warn_dangling_reference): Don't return early if we're
> 	initializing a std_pair_ref_ref_p.
> 
> gcc/ChangeLog:
> 
> 	* doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst:
> 	Extend the description of -Wdangling-reference.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wdangling-reference6.C: New test.
> ---
>   gcc/cp/call.cc                                | 52 ++++++++++++++++---
>   .../options-controlling-c++-dialect.rst       | 10 ++++
>   .../g++.dg/warn/Wdangling-reference6.C        | 38 ++++++++++++++
>   3 files changed, 94 insertions(+), 6 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 492db9b59ad..bd3b64a7e26 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13527,6 +13527,34 @@ initialize_reference (tree type, tree expr,
>     return expr;
>   }
>   
> +/* Return true if T is std::pair<const T&, const T&>.  */
> +
> +static bool
> +std_pair_ref_ref_p (tree t)
> +{
> +  /* First, check if we have std::pair.  */
> +  if (!NON_UNION_CLASS_TYPE_P (t)
> +      || !CLASSTYPE_TEMPLATE_INSTANTIATION (t))
> +    return false;
> +  tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (t));
> +  if (!decl_in_std_namespace_p (tdecl))
> +    return false;
> +  tree name = DECL_NAME (tdecl);
> +  if (!name || !id_equal (name, "pair"))
> +    return false;
> +
> +  /* Now see if the template arguments are both const T&.  */
> +  tree args = CLASSTYPE_TI_ARGS (t);
> +  if (TREE_VEC_LENGTH (args) != 2)
> +    return false;
> +  for (int i = 0; i < 2; i++)
> +    if (!TYPE_REF_OBJ_P (TREE_VEC_ELT (args, i))
> +	|| !CP_TYPE_CONST_P (TREE_TYPE (TREE_VEC_ELT (args, i))))
> +      return false;
> +
> +  return true;
> +}
> +
>   /* Helper for maybe_warn_dangling_reference to find a problematic CALL_EXPR
>      that initializes the LHS (and at least one of its arguments represents
>      a temporary, as outlined in maybe_warn_dangling_reference), or NULL_TREE
> @@ -13556,11 +13584,6 @@ do_warn_dangling_reference (tree expr)
>   	    || warning_suppressed_p (fndecl, OPT_Wdangling_reference)
>   	    || !warning_enabled_at (DECL_SOURCE_LOCATION (fndecl),
>   				    OPT_Wdangling_reference)
> -	    /* If the function doesn't return a reference, don't warn.  This
> -	       can be e.g.
> -		 const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
> -	       which doesn't dangle: std::min here returns an int.  */
> -	    || !TYPE_REF_OBJ_P (TREE_TYPE (TREE_TYPE (fndecl)))
>   	    /* Don't emit a false positive for:
>   		std::vector<int> v = ...;
>   		std::vector<int>::const_iterator it = v.begin();
> @@ -13573,6 +13596,20 @@ do_warn_dangling_reference (tree expr)
>   		&& DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF)))
>   	  return NULL_TREE;
>   
> +	tree rettype = TREE_TYPE (TREE_TYPE (fndecl));
> +	/* If the function doesn't return a reference, don't warn.  This
> +	   can be e.g.
> +	     const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
> +	   which doesn't dangle: std::min here returns an int.
> +
> +	   If the function returns a std::pair<const T&, const T&>, we
> +	   warn, to detect e.g.
> +	     std::pair<const int&, const int&> v = std::minmax(1, 2);
> +	   which also creates a dangling reference, because std::minmax
> +	   returns std::pair<const T&, const T&>(b, a).  */
> +	if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))

The patch is OK, but do you want to check reference to const for the 
single ref case as well, while you're changing this?

> +	  return NULL_TREE;
> +
>   	/* Here we're looking to see if any of the arguments is a temporary
>   	   initializing a reference parameter.  */
>   	for (int i = 0; i < call_expr_nargs (expr); ++i)
> @@ -13614,6 +13651,8 @@ do_warn_dangling_reference (tree expr)
>         return do_warn_dangling_reference (TREE_OPERAND (expr, 2));
>       case PAREN_EXPR:
>         return do_warn_dangling_reference (TREE_OPERAND (expr, 0));
> +    case TARGET_EXPR:
> +      return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr));
>       default:
>         return NULL_TREE;
>       }
> @@ -13640,7 +13679,8 @@ maybe_warn_dangling_reference (const_tree decl, tree init)
>   {
>     if (!warn_dangling_reference)
>       return;
> -  if (!TYPE_REF_P (TREE_TYPE (decl)))
> +  if (!(TYPE_REF_OBJ_P (TREE_TYPE (decl))

And here.

> +	|| std_pair_ref_ref_p (TREE_TYPE (decl))))
>       return;
>     /* Don't suppress the diagnostic just because the call comes from
>        a system header.  If the DECL is not in a system header, or if
> diff --git a/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst b/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst
> index 5b05d31aae9..8d2a2789ef6 100644
> --- a/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst
> +++ b/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst
> @@ -855,6 +855,16 @@ In addition, these warning options have meanings only for C++ programs:
>       const T& foo (const T&) { ... }
>       #pragma GCC diagnostic pop
>   
> +  :option:`-Wdangling-reference` also warns about code like
> +
> +  .. code-block:: c++
> +
> +    auto p = std::minmax(1, 2);
> +
> +  where ``std::minmax`` returns ``std::pair<const int&, const int&>``, and
> +  both references dangle after the end of the full expression that contains
> +  the call to `std::minmax``.
> +
>     This warning is enabled by :option:`-Wall`.
>   
>   .. option:: -Wno-dangling-reference
> diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference6.C b/gcc/testsuite/g++.dg/warn/Wdangling-reference6.C
> new file mode 100644
> index 00000000000..bf849e290d9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference6.C
> @@ -0,0 +1,38 @@
> +// { dg-do compile { target c++17 } }
> +// { dg-options "-Wdangling-reference" }
> +// Test -Wdangling-reference with std::minmax.
> +
> +#include <algorithm>
> +
> +using U = std::pair<const int&, const int&>;
> +
> +int
> +fn1 ()
> +{
> +  std::pair<const int&, const int&> v = std::minmax(1, 2); // { dg-warning "dangling reference" }
> +  U v2 = std::minmax(1, 2); // { dg-warning "dangling reference" }
> +  auto v3 = std::minmax(1, 2); // { dg-warning "dangling reference" }
> +  return v.first + v2.second + v3.first;
> +}
> +
> +int
> +fn2 ()
> +{
> +  int n = 1;
> +  auto p = std::minmax(n, n + 1); // { dg-warning "dangling reference" }
> +  int m = p.first; // ok
> +  int x = p.second; // undefined behavior
> +
> +  // Note that structured bindings have the same issue
> +  auto [mm, xx] = std::minmax(n, n + 1); // { dg-warning "dangling reference" }
> +  (void) xx; // undefined behavior
> +
> +  return m + x;
> +}
> +
> +int
> +fn3 ()
> +{
> +  auto v = std::minmax({1, 2});
> +  return v.first;
> +}
> 
> base-commit: 1cdfd0e5cd5fc1f493d0832ed65d31320f9585b7
  

On Thu, Nov 10, 2022 at 08:07:25AM -1000, Jason Merrill wrote:
> On 11/9/22 15:56, Marek Polacek wrote:
> > This patch extends -Wdangling-reference to also warn for
> > 
> >    auto v = std::minmax(1, 2);
> > 
> > which dangles because this overload of std::minmax returns
> > a std::pair<const int&, const int&> where the two references are
> > bound to the temporaries created for the arguments of std::minmax.
> > This is a common footgun, also described at
> > <https://en.cppreference.com/w/cpp/algorithm/minmax> in Notes.
> > 
> > It works by extending do_warn_dangling_reference to also warn when the
> > function returns a std::pair<const T&, const T&>.  std_pair_ref_ref_p
> > is a new helper to check that.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* call.cc (std_pair_ref_ref_p): New.
> > 	(do_warn_dangling_reference): Also warn when the function returns
> > 	std::pair<const T&, const T&>.  Recurse into TARGET_EXPR_INITIAL.
> > 	(maybe_warn_dangling_reference): Don't return early if we're
> > 	initializing a std_pair_ref_ref_p.
> > 
> > gcc/ChangeLog:
> > 
> > 	* doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst:
> > 	Extend the description of -Wdangling-reference.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/warn/Wdangling-reference6.C: New test.
> > ---
> >   gcc/cp/call.cc                                | 52 ++++++++++++++++---
> >   .../options-controlling-c++-dialect.rst       | 10 ++++
> >   .../g++.dg/warn/Wdangling-reference6.C        | 38 ++++++++++++++
> >   3 files changed, 94 insertions(+), 6 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C
> > 
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index 492db9b59ad..bd3b64a7e26 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -13527,6 +13527,34 @@ initialize_reference (tree type, tree expr,
> >     return expr;
> >   }
> > +/* Return true if T is std::pair<const T&, const T&>.  */
> > +
> > +static bool
> > +std_pair_ref_ref_p (tree t)
> > +{
> > +  /* First, check if we have std::pair.  */
> > +  if (!NON_UNION_CLASS_TYPE_P (t)
> > +      || !CLASSTYPE_TEMPLATE_INSTANTIATION (t))
> > +    return false;
> > +  tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (t));
> > +  if (!decl_in_std_namespace_p (tdecl))
> > +    return false;
> > +  tree name = DECL_NAME (tdecl);
> > +  if (!name || !id_equal (name, "pair"))
> > +    return false;
> > +
> > +  /* Now see if the template arguments are both const T&.  */
> > +  tree args = CLASSTYPE_TI_ARGS (t);
> > +  if (TREE_VEC_LENGTH (args) != 2)
> > +    return false;
> > +  for (int i = 0; i < 2; i++)
> > +    if (!TYPE_REF_OBJ_P (TREE_VEC_ELT (args, i))
> > +	|| !CP_TYPE_CONST_P (TREE_TYPE (TREE_VEC_ELT (args, i))))
> > +      return false;
> > +
> > +  return true;
> > +}
> > +
> >   /* Helper for maybe_warn_dangling_reference to find a problematic CALL_EXPR
> >      that initializes the LHS (and at least one of its arguments represents
> >      a temporary, as outlined in maybe_warn_dangling_reference), or NULL_TREE
> > @@ -13556,11 +13584,6 @@ do_warn_dangling_reference (tree expr)
> >   	    || warning_suppressed_p (fndecl, OPT_Wdangling_reference)
> >   	    || !warning_enabled_at (DECL_SOURCE_LOCATION (fndecl),
> >   				    OPT_Wdangling_reference)
> > -	    /* If the function doesn't return a reference, don't warn.  This
> > -	       can be e.g.
> > -		 const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
> > -	       which doesn't dangle: std::min here returns an int.  */
> > -	    || !TYPE_REF_OBJ_P (TREE_TYPE (TREE_TYPE (fndecl)))
> >   	    /* Don't emit a false positive for:
> >   		std::vector<int> v = ...;
> >   		std::vector<int>::const_iterator it = v.begin();
> > @@ -13573,6 +13596,20 @@ do_warn_dangling_reference (tree expr)
> >   		&& DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF)))
> >   	  return NULL_TREE;
> > +	tree rettype = TREE_TYPE (TREE_TYPE (fndecl));
> > +	/* If the function doesn't return a reference, don't warn.  This
> > +	   can be e.g.
> > +	     const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
> > +	   which doesn't dangle: std::min here returns an int.
> > +
> > +	   If the function returns a std::pair<const T&, const T&>, we
> > +	   warn, to detect e.g.
> > +	     std::pair<const int&, const int&> v = std::minmax(1, 2);
> > +	   which also creates a dangling reference, because std::minmax
> > +	   returns std::pair<const T&, const T&>(b, a).  */
> > +	if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))
> 
> The patch is OK, but do you want to check reference to const for the single
> ref case as well, while you're changing this?

Thanks.  Yes, I plan to do that soon, but I didn't want to do it in
a single patch, because I want a dedicated test for 'int&' v. 'const int&'
and that felt like a follow-up patch.
 
> > +	  return NULL_TREE;
> > +
> >   	/* Here we're looking to see if any of the arguments is a temporary
> >   	   initializing a reference parameter.  */
> >   	for (int i = 0; i < call_expr_nargs (expr); ++i)
> > @@ -13614,6 +13651,8 @@ do_warn_dangling_reference (tree expr)
> >         return do_warn_dangling_reference (TREE_OPERAND (expr, 2));
> >       case PAREN_EXPR:
> >         return do_warn_dangling_reference (TREE_OPERAND (expr, 0));
> > +    case TARGET_EXPR:
> > +      return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr));
> >       default:
> >         return NULL_TREE;
> >       }
> > @@ -13640,7 +13679,8 @@ maybe_warn_dangling_reference (const_tree decl, tree init)
> >   {
> >     if (!warn_dangling_reference)
> >       return;
> > -  if (!TYPE_REF_P (TREE_TYPE (decl)))
> > +  if (!(TYPE_REF_OBJ_P (TREE_TYPE (decl))
> 
> And here.

Right.

Marek