Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)

[Marek Polacek <polacek at redhat dot com>]

On Wed, Mar 21, 2018 at 02:55:36PM -0400, Jason Merrill wrote:
> On Wed, Mar 21, 2018 at 2:37 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote:
> >> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
> >> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> >> > cxx_constant_value doesn't understand template codes, and neither it
> >> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash.  Here
> >> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
> >> >> > is_nondependent_constant_expression which checks type_unknown_p, it left the
> >> >> > expression as it was.  We can't use is_nondependent_constant_expression in
> >> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
> >> >> > not suitable here; we'd miss diagnostics.  So I did the following; I think we
> >> >> > should reject the testcase with an error.
> >> >> >
> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >> >> >
> >> >> > 2018-03-14  Marek Polacek  <polacek@redhat.com>
> >> >> >
> >> >> >         PR c++/84854
> >> >> >         * semantics.c (finish_if_stmt_cond): Give error if the condition
> >> >> >         is an overloaded function with no contextual type information.
> >> >> >
> >> >> >         * g++.dg/cpp1z/constexpr-if15.C: New test.
> >> >> >
> >> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
> >> >> > index fdf37bea770..a056e9445e9 100644
> >> >> > --- gcc/cp/semantics.c
> >> >> > +++ gcc/cp/semantics.c
> >> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
> >> >> >        && require_constant_expression (cond)
> >> >> >        && !value_dependent_expression_p (cond))
> >> >> >      {
> >> >> > -      cond = instantiate_non_dependent_expr (cond);
> >> >> > -      cond = cxx_constant_value (cond, NULL_TREE);
> >> >> > +      if (type_unknown_p (cond))
> >> >> > +       {
> >> >> > +         cxx_incomplete_type_error (cond, TREE_TYPE (cond));
> >> >> > +         cond = error_mark_node;
> >> >>
> >> >> I think I'd prefer to skip this block when type_unknown_p, and leave
> >> >> error handling up to the code shared with regular if.
> >> >
> >> > Like this?
> >>
> >> Yes, thanks.
> >>
> >> > It was my first version, but I thought we wanted the error;
> >>
> >> Getting an error is an improvement, but it should apply to
> >> non-constexpr if as well, so checking in maybe_convert_cond would be
> >> better.  Actually, if we deal with unknown type there, we shouldn't
> >> need this patch at all.
> >>
> >> ...except I also notice that since maybe_convert_cond doesn't do any
> >> conversion in a template, we're trying to extract the constant value
> >> without first converting to bool, which breaks this testcase:
> >>
> >> struct A
> >> {
> >>   constexpr operator bool () { return true; }
> >>   int i;
> >> };
> >>
> >> A a;
> >>
> >> template <class T> void f()
> >> {
> >>   constexpr bool b = a;  // works
> >>   if constexpr (a) { }   // breaks
> >> }
> >>
> >> int main()
> >> {
> >>   f<int>();
> >> }
> >>
> >> A simple fix would be to replace your type_unknown_p check here with a
> >> comparison to boolean_type_node, so we wait until instantiation time
> >> to require a constant value.
> >
> > Ok, that works.
> >
> > We should also make g++ accept the testcase with "static_assert(a)" instead of
> > "if constexpr (a) { }" probably.
> 
> > I guess the cxx_constant_value call in
> > finish_static_assert should happen earlier?
> 
> fold_non_dependent_expr should already have gotten the constant value,
> the call to cxx_constant_value is just for giving an error.

Oop, right.

> The bug seems to be that is_nondependent_constant_expression doesn't
> realize that the conversion to bool is OK because it uses a constexpr
> member function.

OK, I can look into this separately.

> >> Better would be to actually do the conversion.  Perhaps this could
> >> share code (for converting and getting a constant value) with
> >> finish_static_assert.
> >
> > But this I didn't manage to make to work.  If I call perform_implicit_conversion_flags
> > in maybe_convert_cond, I get
> > error: cannot resolve overloaded function ‘foo’ based on conversion to type ‘bool’
> > so I'm not sure how the conversion would help.
> 
> That looks like a good diagnostic to me, what's the problem?

Ugh, I got this wrong.  That diagnostic is fine, because we can reject
constexpr-if15.C, but with perform_implicit_conversion_flags we then
can't evaluate constexpr-if16.C:
error: the value of ‘a’ is not usable in a constant expression

> > Anyway, here's at least the boolean_type_node version.
> >
> > Bootstrapped/regtested on x86_64-linux.
> >
> > 2018-03-21  Marek Polacek  <polacek@redhat.com>
> >
> >         PR c++/84854
> >         * semantics.c (finish_if_stmt_cond): Check if the type of the condition
> >         is boolean.
> 
> OK.

Thanks, will commit this part along with the testcases.  Incrementally we should
then a) add constexpr-if15.C diagnostic, b) accept constexpr-if16.C with
static_assert, too.

> >         (finish_static_assert): Remove redundant variable.
> 
> But not this hunk; I like to be able to use the name "complain" even
> when it isn't a parameter.

I see, dropped.

Thanks,

	Marek