[C++,Patch/RFC] PR 83796 ("[6/7/8 Regression] Abstract classes allowed to be instantiated when initialised as default parameter to function or constructor")

Hi,

I'm looking into this rather mild regression, which should be relatively 
easy to fix. In short, Jason's fix for c++/54325 moved an 
abstract_virtuals_error_sfinae check from build_aggr_init_expr to 
build_cplus_new therefore the testcase in this new bug isn't rejected 
anymore because a special conditional for value-initialization from { } 
in convert_like_real simply calls build_value_init and quickly returns, 
thus build_cplus_new isn't involved. Thus I'm working on the best way to 
add back the check. The below, which also uses cp_unevaluated_operand, 
appears to work. Likewise something similar inside build_value_init 
itself, which however seems too generic to me (build_value_init is 
called in many other cases). I'm also not sure about 
cp_unevaluated_operand, whether we need something more precise.

Thanks! Paolo.

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

Hi again,

On 24/01/2018 16:58, Paolo Carlini wrote:
> Hi,
>
> I'm looking into this rather mild regression, which should be 
> relatively easy to fix. In short, Jason's fix for c++/54325 moved an 
> abstract_virtuals_error_sfinae check from build_aggr_init_expr to 
> build_cplus_new therefore the testcase in this new bug isn't rejected 
> anymore because a special conditional for value-initialization from { 
> } in convert_like_real simply calls build_value_init and quickly 
> returns, thus build_cplus_new isn't involved. Thus I'm working on the 
> best way to add back the check. The below, which also uses 
> cp_unevaluated_operand, appears to work. Likewise something similar 
> inside build_value_init itself, which however seems too generic to me 
> (build_value_init is called in many other cases). I'm also not sure 
> about cp_unevaluated_operand, whether we need something more precise.
I'm gently "pinging" this message of mine... Definitely not an high 
priority regression (in any case it's only a P3) but I'm still wondering 
if we want to do something about the issue at this time. Lately I 
noticed that in terms of testsuite even something as basic as the below 
passes testing, not sure if we could consider it safe from a theoretical 
point of view, however.

Thanks!
Paolo.

//////////////////////
Index: cp/call.c
===================================================================
--- cp/call.c	(revision 257241)
+++ cp/call.c	(working copy)
@@ -6765,6 +6765,8 @@ convert_like_real (conversion *convs, tree expr, t
 	    && TYPE_HAS_DEFAULT_CONSTRUCTOR (totype))
 	  {
 	    bool direct = CONSTRUCTOR_IS_DIRECT_INIT (expr);
+	    if (abstract_virtuals_error_sfinae (NULL_TREE, totype, complain))
+	      return error_mark_node;
 	    expr = build_value_init (totype, complain);
 	    expr = get_target_expr_sfinae (expr, complain);
 	    if (expr != error_mark_node)
Index: testsuite/g++.dg/cpp0x/abstract-default1.C
===================================================================
--- testsuite/g++.dg/cpp0x/abstract-default1.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/abstract-default1.C	(working copy)
@@ -0,0 +1,26 @@
+// PR c++/83796
+// { dg-do compile { target c++11 } }
+
+struct MyAbstractClass
+{
+  virtual int foo() const = 0;
+};
+
+struct TestClass
+{
+  TestClass(const MyAbstractClass& m = {})  // { dg-error "abstract type" }
+  : value_(m.foo()) {}
+
+  int value_;
+};
+
+int TestFunction(const MyAbstractClass& m = {})  // { dg-error "abstract type" }
+{
+  return m.foo();
+}
+
+int main()
+{
+  TestClass testInstance;  // { dg-error "abstract type" }
+  TestFunction();  // { dg-error "abstract type" }
+}

On Wed, Jan 31, 2018 at 2:55 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>
> On 24/01/2018 16:58, Paolo Carlini wrote:
>>
>> Hi,
>>
>> I'm looking into this rather mild regression, which should be relatively
>> easy to fix. In short, Jason's fix for c++/54325 moved an
>> abstract_virtuals_error_sfinae check from build_aggr_init_expr to
>> build_cplus_new therefore the testcase in this new bug isn't rejected
>> anymore because a special conditional for value-initialization from { } in
>> convert_like_real simply calls build_value_init and quickly returns, thus
>> build_cplus_new isn't involved. Thus I'm working on the best way to add back
>> the check. The below, which also uses cp_unevaluated_operand, appears to
>> work. Likewise something similar inside build_value_init itself, which
>> however seems too generic to me (build_value_init is called in many other
>> cases). I'm also not sure about cp_unevaluated_operand, whether we need
>> something more precise.
>
> I'm gently "pinging" this message of mine... Definitely not an high priority
> regression (in any case it's only a P3) but I'm still wondering if we want
> to do something about the issue at this time. Lately I noticed that in terms
> of testsuite even something as basic as the below passes testing, not sure
> if we could consider it safe from a theoretical point of view, however.

This version is OK; unevaluated context shouldn't affect this, so that
SFINAE tricks can check for it.

Jason

Hi,

On 01/02/2018 15:54, Jason Merrill wrote:
>> I'm gently "pinging" this message of mine... Definitely not an high priority
>> regression (in any case it's only a P3) but I'm still wondering if we want
>> to do something about the issue at this time. Lately I noticed that in terms
>> of testsuite even something as basic as the below passes testing, not sure
>> if we could consider it safe from a theoretical point of view, however.
> This version is OK; unevaluated context shouldn't affect this, so that
> SFINAE tricks can check for it.
You are of course totally right. For the specific testcase we got, not 
using templates, checking for unevaluated context was useful to cut some 
rather redundant diagnostic, that's what fooled me, at first. Anyway, I 
have now checked in the last version.

Thanks again,
Paolo.