Re: C++ PATCH for c++/57891, narrowing conversions in non-type template arguments

[Marek Polacek <polacek at redhat dot com>]

On Tue, Jul 03, 2018 at 03:41:43PM -0400, Jason Merrill wrote:
> On Tue, Jul 3, 2018 at 2:58 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Tue, Jul 03, 2018 at 12:40:51PM -0400, Jason Merrill wrote:
> >> On Fri, Jun 29, 2018 at 3:58 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> > On Wed, Jun 27, 2018 at 07:35:15PM -0400, Jason Merrill wrote:
> >> >> On Wed, Jun 27, 2018 at 12:53 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> >> > This PR complains about us accepting invalid code like
> >> >> >
> >> >> >   template<unsigned int> struct A {};
> >> >> >   A<-1> a;
> >> >> >
> >> >> > Where we should detect the narrowing: [temp.arg.nontype] says
> >> >> > "A template-argument for a non-type template-parameter shall be a converted
> >> >> > constant expression ([expr.const]) of the type of the template-parameter."
> >> >> > and a converted constant expression can contain only
> >> >> > - integral conversions other than narrowing conversions,
> >> >> > - [...]."
> >> >> > It spurred e.g.
> >> >> > <https://stackoverflow.com/questions/28184888/how-implicit-conversion-works-for-non-type-template-parameters>
> >> >> > and has >=3 dups so it has some visibility.
> >> >> >
> >> >> > I think build_converted_constant_expr needs to set check_narrowing.
> >> >> > check_narrowing also always mentions that it's in { } but that is no longer
> >> >> > true; in the future it will also apply to <=>.  We'd probably have to add a new
> >> >> > flag to struct conversion if wanted to distinguish between these.
> >> >> >
> >> >> > This does not yet fix detecting narrowing in function templates (78244).
> >> >> >
> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >> >> >
> >> >> > 2018-06-27  Marek Polacek  <polacek@redhat.com>
> >> >> >
> >> >> >         PR c++/57891
> >> >> >         * call.c (build_converted_constant_expr): Set check_narrowing.
> >> >> >         * decl.c (compute_array_index_type): Add warning sentinel.  Use
> >> >> >         input_location.
> >> >> >         * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
> >> >> >         were reported.
> >> >> >         * typeck2.c (check_narrowing): Don't mention { } in diagnostic.
> >> >> >
> >> >> >         * g++.dg/cpp0x/Wnarrowing6.C: New test.
> >> >> >         * g++.dg/cpp0x/Wnarrowing7.C: New test.
> >> >> >         * g++.dg/cpp0x/Wnarrowing8.C: New test.
> >> >> >         * g++.dg/cpp0x/constexpr-data2.C: Add dg-error.
> >> >> >         * g++.dg/init/new43.C: Adjust dg-error.
> >> >> >         * g++.dg/other/fold1.C: Likewise.
> >> >> >         * g++.dg/parse/array-size2.C: Likewise.
> >> >> >         * g++.dg/other/vrp1.C: Add dg-error.
> >> >> >         * g++.dg/template/char1.C: Likewise.
> >> >> >         * g++.dg/ext/builtin12.C: Likewise.
> >> >> >         * g++.dg/template/dependent-name3.C: Adjust dg-error.
> >> >> >
> >> >> > diff --git gcc/cp/call.c gcc/cp/call.c
> >> >> > index 209c1fd2f0e..956c7b149dc 100644
> >> >> > --- gcc/cp/call.c
> >> >> > +++ gcc/cp/call.c
> >> >> > @@ -4152,7 +4152,10 @@ build_converted_constant_expr (tree type, tree expr, tsubst_flags_t complain)
> >> >> >      }
> >> >> >
> >> >> >    if (conv)
> >> >> > -    expr = convert_like (conv, expr, complain);
> >> >> > +    {
> >> >> > +      conv->check_narrowing = !processing_template_decl;
> >> >>
> >> >> Why !processing_template_decl?  This needs a comment.
> >> >
> >> > Otherwise we'd warn for e.g.
> >> >
> >> > template<int N> struct S { char a[N]; };
> >> > S<1> s;
> >> >
> >> > where compute_array_index_type will try to convert the size of the array (which
> >> > is a template_parm_index of type int when parsing the template) to size_type.
> >> > So I guess I can say that we need to wait for instantiation?
> >>
> >> We certainly shouldn't give a narrowing diagnostic about a
> >> value-dependent expression.  It probably makes sense to check that at
> >> the top of check_narrowing, with all the other early exit conditions.
> >> But if we do know the constant value in the template, it's good to
> >> complain then rather than wait for instantiation.
> >
> > Makes sense; how about this then?  (Regtest/bootstrap running.)
> >
> > 2018-07-03  Marek Polacek  <polacek@redhat.com>
> >
> >         PR c++/57891
> >         * call.c (build_converted_constant_expr): Set check_narrowing.
> >         * decl.c (compute_array_index_type): Add warning sentinel.  Use
> >         input_location.
> >         * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
> >         were reported.
> >         * typeck2.c (check_narrowing): Don't warn for instantiation-dependent
> >         expressions or non-constants in a template.  Don't mention { } in
> >         diagnostic.
> >
> >         * g++.dg/cpp0x/Wnarrowing6.C: New test.
> >         * g++.dg/cpp0x/Wnarrowing7.C: New test.
> >         * g++.dg/cpp0x/Wnarrowing8.C: New test.
> >         * g++.dg/cpp0x/Wnarrowing9.C: New test.
> >         * g++.dg/cpp0x/Wnarrowing10.C: New test.
> >         * g++.dg/cpp0x/constexpr-data2.C: Add dg-error.
> >         * g++.dg/init/new43.C: Adjust dg-error.
> >         * g++.dg/other/fold1.C: Likewise.
> >         * g++.dg/parse/array-size2.C: Likewise.
> >         * g++.dg/other/vrp1.C: Add dg-error.
> >         * g++.dg/template/char1.C: Likewise.
> >         * g++.dg/ext/builtin12.C: Likewise.
> >         * g++.dg/template/dependent-name3.C: Adjust dg-error.
> >
> > diff --git gcc/cp/call.c gcc/cp/call.c
> > index 209c1fd2f0e..4fb0fa8774b 100644
> > --- gcc/cp/call.c
> > +++ gcc/cp/call.c
> > @@ -4152,7 +4152,10 @@ build_converted_constant_expr (tree type, tree expr, tsubst_flags_t complain)
> >      }
> >
> >    if (conv)
> > -    expr = convert_like (conv, expr, complain);
> > +    {
> > +      conv->check_narrowing = true;
> > +      expr = convert_like (conv, expr, complain);
> > +    }
> >    else
> >      expr = error_mark_node;
> >
> > diff --git gcc/cp/decl.c gcc/cp/decl.c
> > index c04b9b7d457..8da63fa2aaa 100644
> > --- gcc/cp/decl.c
> > +++ gcc/cp/decl.c
> > @@ -9508,6 +9508,8 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
> >        else
> >         {
> >           size = instantiate_non_dependent_expr_sfinae (size, complain);
> > +         /* Don't warn about narrowing for VLAs.  */
> > +         warning_sentinel s (warn_narrowing, !TREE_CONSTANT (osize));
> >           size = build_converted_constant_expr (size_type_node, size, complain);
> 
> Hmm, perhaps the underlying issue is that we only want
> build_converted_constant_expr to check for narrowing of constant
> values; if the value isn't constant, it isn't any kind of constant
> expression.  So perhaps the checking needs to happen as part of
> constexpr evaluation.

Not quite sure if I follow.  My very first attempt was to call check_narrowing
in build_converted_constant_expr but that then doesn't detect narrowing in
Wnarrowing8.C with user-defined conversion represented as a TARGET_EXPR that
is not TREE_CONSTANT.

I think I even tried putting check_narrowing to constexpr evaluation but
that was I think too late in that the expression had already been evaluated
and converted and the narrowing conversion was lost.

> > @@ -6669,9 +6669,12 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
> >           /* C++17: A template-argument for a non-type template-parameter shall
> >              be a converted constant expression (8.20) of the type of the
> >              template-parameter.  */
> > +         int errs = errorcount;
> >           expr = build_converted_constant_expr (type, expr, complain);
> >           if (expr == error_mark_node)
> > -           return error_mark_node;
> > +           /* Make sure we return NULL_TREE only if we have really issued
> > +              an error, as described above.  */
> > +           return errorcount > errs ? NULL_TREE : error_mark_node;
> 
> Is this still needed?

With the current patch it is, to avoid redundant errors.

> > +      /* If we're in a template and we know the constant value, we can
> > +        warn.  Otherwise wait for instantiation.  */
> > +      || (processing_template_decl && !TREE_CONSTANT (init)))
> 
> I don't think we want this condition.  If the value is non-constant
> but also not dependent, it'll never be constant, so we can go ahead
> and complain.

That was because of constexpr-ex4.C with a VLA in a class template.
check_narrowing gets a VAR_DECL 'a' and fold_non_dependent_expr-ing it
fails with
error: no matching function for call to ‘A::operator int(const A*)’
I didn't think tweaking 'complain' was the way to go.

	Marek