Re: [PATCH] c++: Add __reference_con{struc,ver}ts_from_temporary [PR104477]

[Marek Polacek <polacek@redhat.com>]

On Tue, Jul 12, 2022 at 04:15:00PM -0400, Jason Merrill wrote:
> On 7/12/22 16:10, Jason Merrill wrote:
> > On 7/8/22 13:41, Marek Polacek wrote:
> > > This patch implements C++23 P2255R2, which adds two new type traits to
> > > detect reference binding to a temporary.  They can be used to detect code
> > > like
> > > 
> > >    std::tuple<const std::string&> t("meow");
> > > 
> > > which is incorrect because it always creates a dangling reference,
> > > because
> > > the std::string temporary is created inside the selected constructor of
> > > std::tuple, and not outside it.
> > > 
> > > There are two new compiler builtins,
> > > __reference_constructs_from_temporary
> > > and __reference_converts_from_temporary.  The former is used to simulate
> > > direct- and the latter copy-initialization context.  But I had a
> > > hard time
> > > finding a test where there's actually a difference.  Under DR 2267, both
> > > of these are invalid:
> > > 
> > >    struct A { } a;
> > >    struct B { explicit B(const A&); };
> > >    const B &b1{a};
> > >    const B &b2(a);
> > > 
> > > so I had to peruse [over.match.ref], and eventually realized that the
> > > difference can be seen here:
> > > 
> > >    struct G {
> > >      operator int(); // #1
> > >      explicit operator int&&(); // #2
> > >    };
> > > 
> > > int&& r1(G{}); // use #2 (no temporary)
> > > int&& r2 = G{}; // use #1 (a temporary is created to be bound to int&&)
> > > 
> > > The implementation itself was rather straightforward because we already
> > > have conv_binds_ref_to_prvalue.  The main function here is
> > > reference_from_temporary.  The renaming to ref_conv_binds_to_temporary_p
> > > is because previously the function didn't distinguish between an invalid
> > > conversion and one that binds to a prvalue.
> > > 
> > > The patch also adds the relevant class and variable templates to
> > > <type_traits>.
> > > 
> > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > 
> > >     PR c++/104477
> > > 
> > > gcc/c-family/ChangeLog:
> > > 
> > >     * c-common.cc (c_common_reswords): Add
> > >     __reference_constructs_from_temporary and
> > >     __reference_converts_from_temporary.
> > >     * c-common.h (enum rid): Add RID_REF_CONSTRUCTS_FROM_TEMPORARY and
> > >     RID_REF_CONVERTS_FROM_TEMPORARY.
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > >     * call.cc (ref_conv_binds_directly_p): Rename to ...
> > >     (ref_conv_binds_to_temporary_p): ... this.  Add a new bool
> > >     parameter.  Return true only if the conversion is valid and
> > >     conv_binds_ref_to_prvalue returns true.
> > >     * constraint.cc (diagnose_trait_expr): Handle
> > >     CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > >     * cp-tree.h (enum cp_trait_kind): Add
> > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY
> > >     and CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > >     (ref_conv_binds_directly_p): Rename to ...
> > >     (ref_conv_binds_to_temporary_p): ... this.
> > >     (reference_from_temporary): Declare.
> > >     * cxx-pretty-print.cc (pp_cxx_trait_expression): Handle
> > >     CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > >     * method.cc (reference_from_temporary): New.
> > >     * parser.cc (cp_parser_primary_expression): Handle
> > >     RID_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > RID_REF_CONVERTS_FROM_TEMPORARY.
> > >     (cp_parser_trait_expr): Likewise.
> > >     (warn_for_range_copy): Adjust to call ref_conv_binds_to_temporary_p.
> > >     * semantics.cc (trait_expr_value): Handle
> > >     CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > >     (finish_trait_expr): Likewise.
> > > 
> > > libstdc++-v3/ChangeLog:
> > > 
> > >     * include/std/type_traits (reference_constructs_from_temporary,
> > >     reference_converts_from_temporary): New class templates.
> > >     (reference_constructs_from_temporary_v,
> > >     reference_converts_from_temporary_v): New variable templates.
> > >     (__cpp_lib_reference_from_temporary): Define for C++23.
> > >     * include/std/version (__cpp_lib_reference_from_temporary):
> > > Define for
> > >     C++23.
> > >     * testsuite/20_util/variable_templates_for_traits.cc: Test
> > >     reference_constructs_from_temporary_v and
> > >     reference_converts_from_temporary_v.
> > >     * testsuite/20_util/reference_from_temporary/value.cc: New test.
> > >     * testsuite/20_util/reference_from_temporary/value2.cc: New test.
> > >     * testsuite/20_util/reference_from_temporary/version.cc: New test.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > >     * g++.dg/ext/reference_constructs_from_temporary1.C: New test.
> > >     * g++.dg/ext/reference_converts_from_temporary1.C: New test.
> > > ---
> > >   gcc/c-family/c-common.cc                      |   4 +
> > >   gcc/c-family/c-common.h                       |   2 +
> > >   gcc/cp/call.cc                                |  14 +-
> > >   gcc/cp/constraint.cc                          |   8 +
> > >   gcc/cp/cp-tree.h                              |   7 +-
> > >   gcc/cp/cxx-pretty-print.cc                    |   6 +
> > >   gcc/cp/method.cc                              |  28 +++
> > >   gcc/cp/parser.cc                              |  14 +-
> > >   gcc/cp/semantics.cc                           |   8 +
> > >   .../reference_constructs_from_temporary1.C    | 214 ++++++++++++++++++
> > >   .../ext/reference_converts_from_temporary1.C  | 214 ++++++++++++++++++
> > >   libstdc++-v3/include/std/type_traits          |  39 ++++
> > >   libstdc++-v3/include/std/version              |   5 +-
> > >   .../20_util/reference_from_temporary/value.cc | 110 +++++++++
> > >   .../reference_from_temporary/value2.cc        |  28 +++
> > >   .../reference_from_temporary/version.cc       |  27 +++
> > >   .../20_util/variable_templates_for_traits.cc  |  14 ++
> > >   17 files changed, 730 insertions(+), 12 deletions(-)
> > >   create mode 100644
> > > gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
> > >   create mode 100644
> > > gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
> > >   create mode 100644
> > > libstdc++-v3/testsuite/20_util/reference_from_temporary/value.cc
> > >   create mode 100644
> > > libstdc++-v3/testsuite/20_util/reference_from_temporary/value2.cc
> > >   create mode 100644
> > > libstdc++-v3/testsuite/20_util/reference_from_temporary/version.cc
> > > 
> > > diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> > > index 1b8e73f7bc5..655c3aefee6 100644
> > > --- a/gcc/c-family/c-common.cc
> > > +++ b/gcc/c-family/c-common.cc
> > > @@ -537,6 +537,10 @@ const struct c_common_resword c_common_reswords[] =
> > >     { "__is_constructible", RID_IS_CONSTRUCTIBLE, D_CXXONLY },
> > >     { "__is_nothrow_assignable", RID_IS_NOTHROW_ASSIGNABLE, D_CXXONLY },
> > >     { "__is_nothrow_constructible", RID_IS_NOTHROW_CONSTRUCTIBLE,
> > > D_CXXONLY },
> > > +  { "__reference_constructs_from_temporary",
> > > RID_REF_CONSTRUCTS_FROM_TEMPORARY,
> > > +                    D_CXXONLY },
> > > +  { "__reference_converts_from_temporary",
> > > RID_REF_CONVERTS_FROM_TEMPORARY,
> > > +                    D_CXXONLY },
> > >     /* C++ transactional memory.  */
> > >     { "synchronized",    RID_SYNCHRONIZED, D_CXX_OBJC | D_TRANSMEM },
> > > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> > > index c0900848965..f9064393b4e 100644
> > > --- a/gcc/c-family/c-common.h
> > > +++ b/gcc/c-family/c-common.h
> > > @@ -184,6 +184,8 @@ enum rid
> > >     RID_IS_UNION,                RID_UNDERLYING_TYPE,
> > >     RID_IS_ASSIGNABLE,           RID_IS_CONSTRUCTIBLE,
> > >     RID_IS_NOTHROW_ASSIGNABLE,   RID_IS_NOTHROW_CONSTRUCTIBLE,
> > > +  RID_REF_CONSTRUCTS_FROM_TEMPORARY,
> > > +  RID_REF_CONVERTS_FROM_TEMPORARY,
> > >     /* C++11 */
> > >     RID_CONSTEXPR, RID_DECLTYPE, RID_NOEXCEPT, RID_NULLPTR,
> > > RID_STATIC_ASSERT,
> > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > > index fc98552fda2..1ba209f61f1 100644
> > > --- a/gcc/cp/call.cc
> > > +++ b/gcc/cp/call.cc
> > > @@ -9109,21 +9109,23 @@ conv_binds_ref_to_prvalue (conversion *c)
> > >     return conv_is_prvalue (next_conversion (c));
> > >   }
> > > -/* True iff converting EXPR to a reference type TYPE does not involve
> > > -   creating a temporary.  */
> > > +/* True iff converting EXPR to a reference type TYPE binds the
> > > reference to
> > > +   a temporary.  DIRECT_INIT_P says whether the conversion should
> > > be done
> > > +   in direct- or copy-initialization context.  */
> > >   bool
> > > -ref_conv_binds_directly_p (tree type, tree expr)
> > > +ref_conv_binds_to_temporary_p (tree type, tree expr,
> > > +                   bool direct_init_p /*= false*/)
> > >   {
> > >     gcc_assert (TYPE_REF_P (type));
> > >     /* Get the high-water mark for the CONVERSION_OBSTACK.  */
> > >     void *p = conversion_obstack_alloc (0);
> > > +  const int flags = direct_init_p ? LOOKUP_NORMAL : LOOKUP_IMPLICIT;
> > >     conversion *conv = implicit_conversion (type, TREE_TYPE (expr), expr,
> > > -                      /*c_cast_p=*/false,
> > > -                      LOOKUP_IMPLICIT, tf_none);
> > > -  bool ret = conv && !conv->bad_p && !conv_binds_ref_to_prvalue (conv);
> > > +                      /*c_cast_p=*/false, flags, tf_none);
> > > +  bool ret = conv && !conv->bad_p && conv_binds_ref_to_prvalue (conv);
> > >     /* Free all the conversions we allocated.  */
> > >     obstack_free (&conversion_obstack, p);
> > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> > > index 591155cee22..648cc9d176d 100644
> > > --- a/gcc/cp/constraint.cc
> > > +++ b/gcc/cp/constraint.cc
> > > @@ -3687,6 +3687,14 @@ diagnose_trait_expr (tree expr, tree args)
> > >       case CPTK_HAS_UNIQUE_OBJ_REPRESENTATIONS:
> > >         inform (loc, "  %qT does not have unique object
> > > representations", t1);
> > >         break;
> > > +    case CPTK_REF_CONSTRUCTS_FROM_TEMPORARY:
> > > +      inform (loc, "  %qT is not a reference that binds to a temporary "
> > > +          "object of type %qT (direct-initialization)", t1, t2);
> > > +      break;
> > > +    case CPTK_REF_CONVERTS_FROM_TEMPORARY:
> > > +      inform (loc, "  %qT is not a reference that binds to a temporary "
> > > +          "object of type %qT (copy-initialization)", t1, t2);
> > > +      break;
> > >       case CPTK_BASES:
> > >       case CPTK_DIRECT_BASES:
> > >       case CPTK_UNDERLYING_TYPE:
> > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > > index 2fde4f83b41..c3bed31e455 100644
> > > --- a/gcc/cp/cp-tree.h
> > > +++ b/gcc/cp/cp-tree.h
> > > @@ -1397,7 +1397,9 @@ enum cp_trait_kind
> > >     CPTK_IS_ASSIGNABLE,
> > >     CPTK_IS_CONSTRUCTIBLE,
> > >     CPTK_IS_NOTHROW_ASSIGNABLE,
> > > -  CPTK_IS_NOTHROW_CONSTRUCTIBLE
> > > +  CPTK_IS_NOTHROW_CONSTRUCTIBLE,
> > > +  CPTK_REF_CONSTRUCTS_FROM_TEMPORARY,
> > > +  CPTK_REF_CONVERTS_FROM_TEMPORARY
> > >   };
> > >   /* The types that we are processing.  */
> > > @@ -6520,7 +6522,7 @@ extern bool sufficient_parms_p
> > > (const_tree);
> > >   extern tree type_decays_to            (tree);
> > >   extern tree extract_call_expr            (tree);
> > >   extern tree build_trivial_dtor_call        (tree, bool = false);
> > > -extern bool ref_conv_binds_directly_p        (tree, tree);
> > > +extern bool ref_conv_binds_to_temporary_p    (tree, tree, bool = false);
> > >   extern tree build_user_type_conversion        (tree, tree, int,
> > >                            tsubst_flags_t);
> > >   extern tree build_new_function_call        (tree, vec<tree, va_gc> **,
> > > @@ -7105,6 +7107,7 @@ extern tree forward_parm            (tree);
> > >   extern bool is_trivially_xible            (enum tree_code, tree, tree);
> > >   extern bool is_nothrow_xible            (enum tree_code, tree, tree);
> > >   extern bool is_xible                (enum tree_code, tree, tree);
> > > +extern bool reference_from_temporary        (tree, tree, bool);
> > >   extern tree get_defaulted_eh_spec        (tree, tsubst_flags_t =
> > > tf_warning_or_error);
> > >   extern bool maybe_explain_implicit_delete    (tree);
> > >   extern void explain_implicit_non_constexpr    (tree);
> > > diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc
> > > index 7e4db2e413b..44590830a61 100644
> > > --- a/gcc/cp/cxx-pretty-print.cc
> > > +++ b/gcc/cp/cxx-pretty-print.cc
> > > @@ -2696,6 +2696,12 @@ pp_cxx_trait_expression (cxx_pretty_printer
> > > *pp, tree t)
> > >       case CPTK_IS_NOTHROW_CONSTRUCTIBLE:
> > >         pp_cxx_ws_string (pp, "__is_nothrow_constructible");
> > >         break;
> > > +    case CPTK_REF_CONSTRUCTS_FROM_TEMPORARY:
> > > +      pp_cxx_ws_string (pp, "__reference_constructs_from_temporary");
> > > +      break;
> > > +    case CPTK_REF_CONVERTS_FROM_TEMPORARY:
> > > +      pp_cxx_ws_string (pp, "__reference_converts_from_temporary");
> > > +      break;
> > >       default:
> > >         gcc_unreachable ();
> > > diff --git a/gcc/cp/method.cc b/gcc/cp/method.cc
> > > index 0dffd648b0b..dd9715b6725 100644
> > > --- a/gcc/cp/method.cc
> > > +++ b/gcc/cp/method.cc
> > > @@ -2211,6 +2211,34 @@ is_xible (enum tree_code code, tree to, tree from)
> > >     return !!expr;
> > >   }
> > > +/* Return true iff conjunction_v<is_reference<T>,
> > > is_constructible<T, U>> is
> > > +   true, and the initialization
> > > +     T t(VAL<U>); // DIRECT_INIT_P
> > > +   or
> > > +     T t = VAL<U>; // !DIRECT_INIT_P
> > > +   binds t to a temporary object whose lifetime is extended.
> > > +   VAL<T> is defined in [meta.unary.prop]:
> > > +   -- If T is a reference or function type, VAL<T> is an expression
> > > with the
> > > +   same type and value category as declval<T>().
> > > +   -- Otherwise, VAL<T> is a prvalue that initially has type T.   */
> > > +
> > > +bool
> > > +reference_from_temporary (tree to, tree from, bool direct_init_p)
> > > +{
> > > +  /* Check is_reference<T>.  */
> > > +  if (!TYPE_REF_P (to))
> > > +    return false;
> > > +  /* Check is_constructible<T, U>.
> > > +     ??? This check doesn't seem to be necessary; if T isn't
> > > constructible
> > > +     from U, we won't be able to create a conversion.  */
> > > +  if (!is_xible (INIT_EXPR, to, build_tree_list (NULL_TREE, from)))
> > > +    return false;
> > 
> > I agree with the comment, did you try leaving this out?  If it stays I'd
> > think it needs to consider direct_init_p.
> > 
> > > +  tree val = build_stub_object (from);
> > > +  if (!TYPE_REF_P (from) && TREE_CODE (from) != FUNCTION_TYPE)
> > > +    val = CLASS_TYPE_P (from) ? force_rvalue (val, tf_none) :
> > > rvalue (val);
> > > +  return ref_conv_binds_to_temporary_p (to, val, direct_init_p);
> > > +}
> > > +
> > >   /* Categorize various special_function_kinds.  */
> > >   #define SFK_CTOR_P(sfk) \
> > >     ((sfk) >= sfk_constructor && (sfk) <= sfk_move_constructor)
> > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> > > index bf9ea3685f8..edee94bda13 100644
> > > --- a/gcc/cp/parser.cc
> > > +++ b/gcc/cp/parser.cc
> > > @@ -5917,6 +5917,8 @@ cp_parser_primary_expression (cp_parser *parser,
> > >       case RID_IS_CONSTRUCTIBLE:
> > >       case RID_IS_NOTHROW_ASSIGNABLE:
> > >       case RID_IS_NOTHROW_CONSTRUCTIBLE:
> > > +    case RID_REF_CONSTRUCTS_FROM_TEMPORARY:
> > > +    case RID_REF_CONVERTS_FROM_TEMPORARY:
> > >         return cp_parser_trait_expr (parser, token->keyword);
> > >       // C++ concepts
> > > @@ -10988,6 +10990,14 @@ cp_parser_trait_expr (cp_parser* parser,
> > > enum rid keyword)
> > >         kind = CPTK_IS_NOTHROW_CONSTRUCTIBLE;
> > >         variadic = true;
> > >         break;
> > > +    case RID_REF_CONSTRUCTS_FROM_TEMPORARY:
> > > +      kind = CPTK_REF_CONSTRUCTS_FROM_TEMPORARY;
> > > +      binary = true;
> > > +      break;
> > > +    case RID_REF_CONVERTS_FROM_TEMPORARY:
> > > +      kind = CPTK_REF_CONVERTS_FROM_TEMPORARY;
> > > +      binary = true;
> > > +      break;
> > >       default:
> > >         gcc_unreachable ();
> > >       }
> > > @@ -13811,7 +13821,7 @@ warn_for_range_copy (tree decl, tree expr)
> > >     if (TYPE_REF_P (type))
> > >       {
> > > -      if (glvalue_p (expr) && !ref_conv_binds_directly_p (type, expr))
> > > +      if (glvalue_p (expr) && ref_conv_binds_to_temporary_p (type,
> > > expr))
> > >       {
> > >         auto_diagnostic_group d;
> > >         if (warning_at (loc, OPT_Wrange_loop_construct,
> > > @@ -13842,7 +13852,7 @@ warn_for_range_copy (tree decl, tree expr)
> > >     tree rtype = cp_build_reference_type (type, /*rval*/false);
> > >     /* If we could initialize the reference directly, it wouldn't
> > > involve any
> > >        copies.  */
> > > -  if (!ref_conv_binds_directly_p (rtype, expr))
> > > +  if (ref_conv_binds_to_temporary_p (rtype, expr))
> > >       return;
> > 
> > I think this case wants the old handling of invalid conversions you
> > mentioned in your intro; we don't want to suggest changing to a
> > reference if that's ill-formed.
> > 
> > In passing we might change the comment to "If we can initialize a
> > reference directly, suggest that to avoid the copy." and move it above
> > the rtype declaration.
> 
> Hmm, and I suspect we get false positives when expr is a prvalue, so
> initializing a non-reference variable of the same cv-unqualified type
> involves no extra copy?

I couldn't provoke a false positive here.  Note that ref_conv_binds_to_temporary_p
always gets a reference so copy elision isn't applicable here.

Marek