[C++ Patch] PR 53158

[C++ Patch] PR 53158

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

Hi,

for this testcase:

int main()
{
auto a = []() { return true; };
auto b = []() { return a(); }; // { dg-error "'a' is not captured" }
int c, d;
while (b() && c < d)
{
}
}

we produce meaningless diagnostics for the while loop (stemming from b() 
but the caret is also in the wrong place, a separate issue):

error: invalid operands of types ‘void’ and ‘int’ to binary ‘operator!=’

because nothing notices in cp_parser_lambda_body that 
cp_parser_expression went wrong, thus finish_return_stmt (expr) is 
called on expr == error_mark_node. The way I'm handling the problem is 
by returning false to the caller, cp_parser_lambda_expression, which is 
already equipped to smoothly handle errors and return back error_mark_node.

Tested x86_64-linux.

Thanks,
Paolo.

///////////////////////

[-- Attachment #2: CL_53158 --]
[-- Type: text/plain, Size: 358 bytes --]

/cp
2012-05-07  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/53158
	* parser.c (cp_parser_lambda_body): Return a boolean, false if
	cp_parser_expression returns error_mark_node;
	(cp_parser_lambda_expression): Check the latter.

/testsuite
2012-05-07  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/53158
	* g++.dg/cpp0x/lambda/lambda-err2.C: New.

[-- Attachment #3: patch_53158 --]
[-- Type: text/plain, Size: 2377 bytes --]

Index: testsuite/g++.dg/cpp0x/lambda/lambda-err2.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-err2.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-err2.C	(revision 0)
@@ -0,0 +1,12 @@
+// PR c++/53158
+// { dg-do compile { target c++11 } }
+
+int main()
+{
+  auto a = []() { return true; };
+  auto b = []() { return a(); };  // { dg-error "'a' is not captured" }
+  int c, d;
+  while (b() && c < d)
+    {
+    }
+}
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 187249)
+++ cp/parser.c	(working copy)
@@ -1848,7 +1848,7 @@ static void cp_parser_lambda_introducer
   (cp_parser *, tree);
 static bool cp_parser_lambda_declarator_opt
   (cp_parser *, tree);
-static void cp_parser_lambda_body
+static bool cp_parser_lambda_body
   (cp_parser *, tree);
 
 /* Statements [gram.stmt.stmt]  */
@@ -8101,7 +8101,7 @@ cp_parser_lambda_expression (cp_parser* parser)
     ok = cp_parser_lambda_declarator_opt (parser, lambda_expr);
 
     if (ok)
-      cp_parser_lambda_body (parser, lambda_expr);
+      ok = cp_parser_lambda_body (parser, lambda_expr);
     else if (cp_parser_require (parser, CPP_OPEN_BRACE, RT_OPEN_BRACE))
       cp_parser_skip_to_end_of_block_or_statement (parser);
 
@@ -8466,11 +8466,13 @@ cp_parser_lambda_declarator_opt (cp_parser* parser
    but which requires special handling.
    LAMBDA_EXPR is the current representation of the lambda expression.  */
 
-static void
+static bool
 cp_parser_lambda_body (cp_parser* parser, tree lambda_expr)
 {
   bool nested = (current_function_decl != NULL_TREE);
   bool local_variables_forbidden_p = parser->local_variables_forbidden_p;
+  bool ok = true;
+
   if (nested)
     push_function_context ();
   else
@@ -8540,6 +8542,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lam
 	cp_parser_require_keyword (parser, RID_RETURN, RT_RETURN);
 
 	expr = cp_parser_expression (parser, /*cast_p=*/false, &idk);
+	if (expr == error_mark_node)
+	  ok = false;
 
 	cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
 	cp_parser_require (parser, CPP_CLOSE_BRACE, RT_CLOSE_BRACE);
@@ -8579,6 +8583,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lam
     pop_function_context();
   else
     --function_depth;
+
+  return ok;
 }
 
 /* Statements [gram.stmt.stmt]  */

Re: [C++ Patch] PR 53158

[Jason Merrill]

I don't think the diagnostic is meaningless; since a() is ill-formed, 
b() has type void.  But we ought to give a more helpful message when an 
expression used as a truth value has an unsuitable type.

Jason

Re: [C++ Patch] PR 53158

[Paolo Carlini]

On 05/07/2012 07:16 PM, Jason Merrill wrote:
> I don't think the diagnostic is meaningless; since a() is ill-formed, 
> b() has type void.
Sure we do understand it ;)
>   But we ought to give a more helpful message when an expression used 
> as a truth value has an unsuitable type.
Thus something much more general than the lambda context of the PR. Ok, 
I can see what I can do.

Thanks!
Paolo.

Re: [C++ Patch] PR 53158

[Paolo Carlini]

On 05/07/2012 07:18 PM, Paolo Carlini wrote:
> On 05/07/2012 07:16 PM, Jason Merrill wrote:
>> I don't think the diagnostic is meaningless; since a() is ill-formed, 
>> b() has type void.
> Sure we do understand it ;)
>>   But we ought to give a more helpful message when an expression used 
>> as a truth value has an unsuitable type.
> Thus something much more general than the lambda context of the PR. 
> Ok, I can see what I can do.
I think this is the minimal testcase we are looking for:

void foo();

void bar(int a, int b)
{
   if (foo() && a < b)
     ;
}

the error message is the same we see in PR 53158, thus:

a.cc:5:20: error: invalid operands of types ‘void’ and ‘int’ to binary 
‘operator!=’
    if (foo() && a < b)

If I take out the 'a < b' subexpression, we get:

a.cc:5:12: error: could not convert ‘foo()’ from ‘void’ to ‘bool’
    if (foo())

Is this the error message we want to see for the former testcase, or we 
want something slightly different?

Thanks!
Paolo.

Re: [C++ Patch] PR 53158

[Manuel López-Ibáñez]

On 7 May 2012 19:40, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 05/07/2012 07:18 PM, Paolo Carlini wrote:
>
> a.cc:5:12: error: could not convert ‘foo()’ from ‘void’ to ‘bool’
>   if (foo())
>
> Is this the error message we want to see for the former testcase, or we want
> something slightly different?

If "foo()" is printed with %qE, and now that we have caret
diagnostics, I would expect the message to be:

error: could not convert expression from 'void' to 'bool'

For comparison, this is what Clang gives:

/tmp/webcompile/_8320_0.cc:7:6: error: value of type 'void' is not
contextually convertible to 'bool'
 if (foo() && a < b)
     ^~~~~
/tmp/webcompile/_8320_0.cc:7:12: error: invalid operands to binary
expression ('void' and 'bool')
 if (foo() && a < b)
     ~~~~~ ^  ~~~~~
2 errors generated.


G++ could do better in this case, foo() is likely to be some kind of
FUNCTION_DECL, so g++ could be smarter and say something like:

error: could not convert return type of 'foo' from 'void' to 'bool'
note: 'foo' declared here

Just a suggestion.

Cheers,

Manuel.

Re: [C++ Patch] PR 53158

[Paolo Carlini]

Hi,

On 05/07/2012 08:07 PM, Manuel López-Ibáñez wrote:
> On 7 May 2012 19:40, Paolo Carlini<paolo.carlini@oracle.com>  wrote:
>> On 05/07/2012 07:18 PM, Paolo Carlini wrote:
>>
>> a.cc:5:12: error: could not convert ‘foo()’ from ‘void’ to ‘bool’
>>    if (foo())
>>
>> Is this the error message we want to see for the former testcase, or we want
>> something slightly different?
> If "foo()" is printed with %qE, and now that we have caret
> diagnostics, I would expect the message to be:
>
> error: could not convert expression from 'void' to 'bool'
A basic fix seems reasonably easy (modulo the caret in the wrong place, 
I guess we have to pass down more locations): instead of just 
unconditionally forwarding to c_common_truthvalue_conversion from 
cp_truthvalue_conversion, which seems a bad idea, given this comment 
preceding the former:

/* Prepare expr to be an argument of a TRUTH_NOT_EXPR,
    or for an `if' or `while' statement or ?..: exp.  It should already
    have been validated to be of suitable type; otherwise, a bad
    diagnostic may result.

note in particular the last two lines ;) we could tell apart CALL_EXPRs 
and pass those through condition_conversion (tree expr). Seems ok also 
from the optimization point of view, becase CALL_EXPRs are not handled 
by c_common_truthvalue_conversion, and just forwarded to 
cp_build_binary_op to build an "appropriate" (incorrect in this specific 
case) NE_EXPR.

Something like the quick hack below gives:

a.cc:5:20: error: could not convert ‘foo()’ from ‘void’ to ‘bool’
    if (foo() && a < b)

Paolo.

//////////////////////

Index: typeck.c
===================================================================
--- typeck.c    (revision 187249)
+++ typeck.c    (working copy)
@@ -4782,7 +4782,11 @@ cp_truthvalue_conversion (tree expr)
        return ret;
      }
    else
-    return c_common_truthvalue_conversion (input_location, expr);
+    {
+      if (TREE_CODE (expr) == CALL_EXPR)
+       return condition_conversion (expr);
+      return c_common_truthvalue_conversion (input_location, expr);
+    }
  }

  /* Just like cp_truthvalue_conversion, but we want a 
CLEANUP_POINT_EXPR.  */

Re: [C++ Patch] PR 53158

[Paolo Carlini]

On 05/07/2012 08:23 PM, Paolo Carlini wrote:
> Index: typeck.c
> ===================================================================
> --- typeck.c    (revision 187249)
> +++ typeck.c    (working copy)
> @@ -4782,7 +4782,11 @@ cp_truthvalue_conversion (tree expr)
>        return ret;
>      }
>    else
> -    return c_common_truthvalue_conversion (input_location, expr);
> +    {
> +      if (TREE_CODE (expr) == CALL_EXPR)
> +       return condition_conversion (expr);
> +      return c_common_truthvalue_conversion (input_location, expr);
> +    }
>  }
>
>  /* Just like cp_truthvalue_conversion, but we want a 
> CLEANUP_POINT_EXPR.  */
Well, not exactly like this because it recurses forever when the code is 
fine ;) but I think you get the idea, we don't want to unconditionally 
use c_common_truthvalue_conversion and we want to stop *before* trying 
to synthesize a NE_EXPR.

About the error message proper, the locations are currently pretty wrong 
for these code paths - is always too advanced, at the end of the 
expression, I'm afraid fixing that will require touching quite a few 
lines of code.

Paolo.

Re: [C++ Patch] PR 53158

[Jason Merrill]

How do we even get to calling cp_truthvalue_conversion?

Jason

Re: [C++ Patch] PR 53158

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

On 05/08/2012 04:10 AM, Jason Merrill wrote:
> How do we even get to calling cp_truthvalue_conversion?
ocp_convert wants to do that, around line 750... And I have a brand new 
proposal ;)

The patchlet below (which passes testing) leads to very consistent error 
messages for the testcase in the PR, and:

void foo();

void bar(int a, int b)
{
if (foo())
;
}

and

void foo();

void bar(int a, int b)
{
if (foo() && a < b)
;
}

exactly the same actually. But maybe we want to differentiate somehow 
what we emit for lambdas, but I don't think we want the status quo for 
%qE for a lambda here, because it would be:

error: could not convert ‘b.main()::<lambda()>()’ from ‘void’ to ‘bool’

What do you think?

Thanks!
Paolo.

///////////////////////////

[-- Attachment #2: patch_53158_2 --]
[-- Type: text/plain, Size: 1745 bytes --]

Index: testsuite/g++.dg/cpp0x/lambda/lambda-err2.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-err2.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-err2.C	(revision 0)
@@ -0,0 +1,12 @@
+// PR c++/53158
+// { dg-do compile { target c++11 } }
+
+int main()
+{
+  auto a = []() { return true; };
+  auto b = []() { return a(); };  // { dg-error "'a' is not captured" }
+  int c, d;
+  while (b() && c < d) // { dg-error "could not convert 'b\\(\\)' from 'void' to 'bool'" }
+    {
+    }
+}
Index: cp/error.c
===================================================================
--- cp/error.c	(revision 187249)
+++ cp/error.c	(working copy)
@@ -1897,6 +1897,13 @@ dump_expr (tree t, int flags)
 	    if (TREE_CODE (ob) == ADDR_EXPR)
 	      {
 		dump_expr (TREE_OPERAND (ob, 0), flags | TFF_EXPR_IN_PARENS);
+		/* Handle lambdas specially.  */
+		if (TREE_CODE (fn) == FUNCTION_DECL
+		    && DECL_NAME (fn) && LAMBDA_FUNCTION_P (fn))
+		  {
+		    dump_call_expr_args (t, flags, true);
+		    return;
+		  }
 		pp_cxx_dot (cxx_pp);
 	      }
 	    else if (TREE_CODE (ob) != PARM_DECL
Index: cp/cvt.c
===================================================================
--- cp/cvt.c	(revision 187249)
+++ cp/cvt.c	(working copy)
@@ -743,6 +743,12 @@ ocp_convert (tree type, tree expr, int convtype, i
 	}
       if (code == BOOLEAN_TYPE)
 	{
+	  if (TREE_CODE (intype) == VOID_TYPE)
+	    {
+	      error ("could not convert %qE from %<void%> to %<bool%>", expr);
+	      return error_mark_node;
+	    }
+
 	  /* We can't implicitly convert a scoped enum to bool, so convert
 	     to the underlying type first.  */
 	  if (SCOPED_ENUM_P (intype) && (convtype & CONV_STATIC))

Re: [C++ Patch] PR 53158

[Jason Merrill]

On 05/07/2012 11:28 PM, Paolo Carlini wrote:
> error: could not convert ‘b.main()::<lambda()>()’ from ‘void’ to ‘bool’

It wouldn't say "operator()"?

I think I'd leave that alone; it is somewhat more informative (about 
what b() expands to) and we're moving toward replacing %qE with caret 
anyway.

The patch to ocp_convert is OK.

Jason

Re: [C++ Patch] PR 53158

[Paolo Carlini]

On 05/08/2012 03:00 PM, Jason Merrill wrote:
> On 05/07/2012 11:28 PM, Paolo Carlini wrote:
>> error: could not convert ‘b.main()::<lambda()>()’ from ‘void’ to ‘bool’
>
> It wouldn't say "operator()"?
Nope, it says exactly the above. If you tell me what would be more 
sensible which remaining informative, I can see if I can quickly prepare 
something... like b.operator()? Would it be better? That would be very 
quick to implement: b is for free, and the arguments too, if there is 
something better we want to print in the middle, just let me know.
> I think I'd leave that alone; it is somewhat more informative (about 
> what b() expands to) and we're moving toward replacing %qE with caret 
> anyway.
... or anyway.
> The patch to ocp_convert is OK.
Great, thanks.

Paolo.

Re: [C++ Patch] PR 53158

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

Hi again,

On 05/08/2012 03:00 PM, Jason Merrill wrote:
> On 05/07/2012 11:28 PM, Paolo Carlini wrote:
>> error: could not convert ‘b.main()::<lambda()>()’ from ‘void’ to ‘bool’
>
> It wouldn't say "operator()"?
>
> I think I'd leave that alone; it is somewhat more informative (about 
> what b() expands to) and we're moving toward replacing %qE with caret 
> anyway.
>
> The patch to ocp_convert is OK.
As you may have noticed, the patchlet is still unapplied (I'm attaching 
below what I have tested and ready to get in per your approval).

Is unapplied because I was really nervous due to the wrong location 
(thus caret) of the error call, at the end of the whole condition. Now, 
I'm wondering, shall we consistently use error_at (location_of (expr), 
... for the error messages produced by the *convert* functions? The 
below quick fix makes me *much* more happy, the caret points to the 
closed round brace of the b() call. Can I trust all the exprs to come 
with an embedded location *at least* as accurate as input_location, 
normally better? In case I can do a pass through all of cvt.c etc and 
repost a largish patch...

Thanks!

Paolo.

////////////////

[-- Attachment #2: patch_53158_3 --]
[-- Type: text/plain, Size: 1119 bytes --]

Index: testsuite/g++.dg/cpp0x/lambda/lambda-err2.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-err2.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-err2.C	(revision 0)
@@ -0,0 +1,12 @@
+// PR c++/53158
+// { dg-do compile { target c++11 } }
+
+int main()
+{
+  auto a = []() { return true; };
+  auto b = []() { return a(); };  // { dg-error "'a' is not captured" }
+  int c, d;
+  while (b() && c < d) // { dg-error "could not convert" }
+    {
+    }
+}
Index: cp/cvt.c
===================================================================
--- cp/cvt.c	(revision 187280)
+++ cp/cvt.c	(working copy)
@@ -743,6 +743,12 @@ ocp_convert (tree type, tree expr, int convtype, i
 	}
       if (code == BOOLEAN_TYPE)
 	{
+	  if (TREE_CODE (intype) == VOID_TYPE)
+	    {
+	      error ("could not convert %qE from %<void%> to %<bool%>", expr);
+	      return error_mark_node;
+	    }
+
 	  /* We can't implicitly convert a scoped enum to bool, so convert
 	     to the underlying type first.  */
 	  if (SCOPED_ENUM_P (intype) && (convtype & CONV_STATIC))

[-- Attachment #3: patchlet_proposal --]
[-- Type: text/plain, Size: 612 bytes --]

Index: cvt.c
===================================================================
--- cvt.c	(revision 187290)
+++ cvt.c	(working copy)
@@ -743,6 +743,14 @@ ocp_convert (tree type, tree expr, int convtype, i
 	}
       if (code == BOOLEAN_TYPE)
 	{
+	  if (TREE_CODE (intype) == VOID_TYPE)
+	    {
+	      error_at (location_of (expr),
+			"could not convert %qE from %<void%> to %<bool%>",
+			expr);
+	      return error_mark_node;
+	    }
+
 	  /* We can't implicitly convert a scoped enum to bool, so convert
 	     to the underlying type first.  */
 	  if (SCOPED_ENUM_P (intype) && (convtype & CONV_STATIC))

Re: [C++ Patch] PR 53158

[Jason Merrill]

On 05/09/2012 08:21 AM, Paolo Carlini wrote:
> Is unapplied because I was really nervous due to the wrong location
> (thus caret) of the error call, at the end of the whole condition. Now,
> I'm wondering, shall we consistently use error_at (location_of (expr),
> ... for the error messages produced by the *convert* functions?

Sure.  Note that as an alternative to error_at (location_of (expr) you 
can just use %q+E; the + means "use the location of this argument".

> The
> below quick fix makes me *much* more happy, the caret points to the
> closed round brace of the b() call. Can I trust all the exprs to come
> with an embedded location *at least* as accurate as input_location,
> normally better?

I would think so, yes.  If an expression doesn't have a location, 
location_of/%q+E will just use input_location.

Jason

Re: [C++ Patch] PR 53158

[Manuel López-Ibáñez]

On 9 May 2012 14:59, Jason Merrill <jason@redhat.com> wrote:
> On 05/09/2012 08:21 AM, Paolo Carlini wrote:
>>
>> Is unapplied because I was really nervous due to the wrong location
>> (thus caret) of the error call, at the end of the whole condition. Now,
>> I'm wondering, shall we consistently use error_at (location_of (expr),
>> ... for the error messages produced by the *convert* functions?
>
>
> Sure.  Note that as an alternative to error_at (location_of (expr) you can
> just use %q+E; the + means "use the location of this argument".

This far less clear than error_at(location, "whatever"). And it
requires the diagnostics machinery to know about input_location. I
removed %H precisely for those reasons. Please, let's stop using "+"
in diagnostics and always use explicit locations.

Cheers,

Manuel.

Re: [C++ Patch] PR 53158

[Paolo Carlini]

Hi,

On 05/09/2012 02:59 PM, Jason Merrill wrote:
> On 05/09/2012 08:21 AM, Paolo Carlini wrote:
>> Is unapplied because I was really nervous due to the wrong location
>> (thus caret) of the error call, at the end of the whole condition. Now,
>> I'm wondering, shall we consistently use error_at (location_of (expr),
>> ... for the error messages produced by the *convert* functions?
>
> Sure.  Note that as an alternative to error_at (location_of (expr) you 
> can just use %q+E; the + means "use the location of this argument".
A great, very concise, didn't know that (but maybe we want to make 
Manuel happier ;)
>> The
>> below quick fix makes me *much* more happy, the caret points to the
>> closed round brace of the b() call. Can I trust all the exprs to come
>> with an embedded location *at least* as accurate as input_location,
>> normally better?
> I would think so, yes.  If an expression doesn't have a location, 
> location_of/%q+E will just use input_location.
In the meanwhile I found a "counterexample" ;) Consider:

enum E { e1 };

E e = e1;

void bar(int a)
{
   if (e = a)
     ;
}

right now the location for the "invalid conversion from ‘int’ to ‘E’" 
error message seems pretty good, points to the a in  "if (e = a)". If I do:

Index: call.c
===================================================================
--- call.c      (revision 187290)
+++ call.c      (working copy)
@@ -5704,7 +5704,7 @@ convert_like_real (conversion *convs, tree expr, t
             break;
         }

-      permerror (input_location, "invalid conversion from %qT to %qT",
+      permerror (location_of (expr), "invalid conversion from %qT to %qT",
                  TREE_TYPE (expr), totype);
        if (fn)
         permerror (DECL_SOURCE_LOCATION (fn),

then an horrible thing happen: the error message points to the b in bar 
in "void bar(int a)" !!! What the heck has that to do with the actual 
conversion two lines below?!? Isn't even pointing to the a in "void 
bar(int a)"!

So... I guess I could start experimenting with my idea, but we should be 
ready to fix regressions... Or we can already take from the 
"counterexample" a general lesson? Or maybe a subset of the conversion* 
functions, those in cvt.c?, seems first blush safer to tweak?

Paolo.

Re: [C++ Patch] PR 53158

[Jason Merrill]

I think the problem is you really want to use EXPR_LOC_OR_HERE rather 
than location_of; if the argument happens to be a DECL, location_of will 
give you the declaration location rather than the use location.

Jason

Re: [C++ Patch] PR 53158

[Jason Merrill]

On 05/09/2012 09:04 AM, Manuel López-Ibáñez wrote:
> This far less clear than error_at(location, "whatever"). And it
> requires the diagnostics machinery to know about input_location. I
> removed %H precisely for those reasons. Please, let's stop using "+"
> in diagnostics and always use explicit locations.

OK.

Jason

Re: [C++ Patch] PR 53158

[Paolo Carlini]

On 05/09/2012 03:20 PM, Jason Merrill wrote:
> I think the problem is you really want to use EXPR_LOC_OR_HERE rather 
> than location_of; if the argument happens to be a DECL, location_of 
> will give you the declaration location rather than the use location.
Ah! Thanks a lot, now all those small details I always see in the 
diagnostics are becoming much more clear ;) Let's see what I can do...

Paolo.