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

[Jason Merrill <jason@redhat.com>]

On 7/14/22 13:43, Marek Polacek wrote:
> 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.

Ah, I see: if expr is a prvalue, that counts as binding a reference to a 
temporary, so we (properly) don't think the reference is an improvement.

The function name reference_from_temporary seems to me to suggest a tree 
value rather than bool, maybe rename to ref_xes_from_temporary?

OK either way.

Jason