Re: [PATCH v2] c++: wrong ambiguity in accessing static field [PR112744]

[Jason Merrill <jason@redhat.com>]

On 11/29/23 17:01, Marek Polacek wrote:
> On Wed, Nov 29, 2023 at 03:28:44PM -0500, Jason Merrill wrote:
>> On 11/29/23 12:43, Marek Polacek wrote:
>>> On Wed, Nov 29, 2023 at 12:23:46PM -0500, Patrick Palka wrote:
>>>> On Wed, 29 Nov 2023, Marek Polacek wrote:
>>>>
>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>>>
>>>>> Now that I'm posting this patch, I think you'll probably want me to use
>>>>> ba_any unconditionally.  That works too; g++.dg/tc1/dr52.C just needs
>>>>> a trivial testsuite tweak:
>>>>>     'C' is not an accessible base of 'X'
>>>>> v.
>>>>>     'C' is an inaccessible base of 'X'
>>>>> We should probably unify those messages...
>>>>>
>>>>> -- >8 --
>>>>> Given
>>>>>
>>>>>     struct A { constexpr static int a = 0; };
>>>>>     struct B : A {};
>>>>>     struct C : A {};
>>>>>     struct D : B, C {};
>>>>>
>>>>> we give the "'A' is an ambiguous base of 'D'" error for
>>>>>
>>>>>     D{}.A::a;
>>>>>
>>>>> which seems wrong: 'a' is a static data member so there is only one copy
>>>>> so it can be unambiguously referred to even if there are multiple A
>>>>> objects.  clang++/MSVC/icx agree.
>>>>>
>>>>> 	PR c++/112744
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> 	* typeck.cc (finish_class_member_access_expr): When accessing
>>>>> 	a static data member, use ba_any for lookup_base.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> 	* g++.dg/lookup/scoped11.C: New test.
>>>>> 	* g++.dg/lookup/scoped12.C: New test.
>>>>> 	* g++.dg/lookup/scoped13.C: New test.
>>>>> ---
>>>>>    gcc/cp/typeck.cc                       | 21 ++++++++++++++++++---
>>>>>    gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++
>>>>>    gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++
>>>>>    gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++
>>>>>    4 files changed, 60 insertions(+), 3 deletions(-)
>>>>>    create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C
>>>>>    create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C
>>>>>    create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C
>>>>>
>>>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
>>>>> index e995fb6ddd7..c4de8bb2616 100644
>>>>> --- a/gcc/cp/typeck.cc
>>>>> +++ b/gcc/cp/typeck.cc
>>>>> @@ -3476,7 +3476,7 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
>>>>>    			   name, scope);
>>>>>    		  return error_mark_node;
>>>>>    		}
>>>>> -	
>>>>> +
>>>>>    	      if (TREE_SIDE_EFFECTS (object))
>>>>>    		val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val);
>>>>>    	      return val;
>>>>> @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
>>>>>    	      return error_mark_node;
>>>>>    	    }
>>>>> +	  /* NAME may refer to a static data member, in which case there is
>>>>> +	     one copy of the data member that is shared by all the objects of
>>>>> +	     the class.  So NAME can be unambiguously referred to even if
>>>>> +	     there are multiple indirect base classes containing NAME.  */
>>>>> +	  const base_access ba = [scope, name] ()
>>>>> +	    {
>>>>> +	      if (identifier_p (name))
>>>>> +		{
>>>>> +		  tree m = lookup_member (scope, name, /*protect=*/0,
>>>>> +					  /*want_type=*/false, tf_none);
>>>>> +		  if (!m || VAR_P (m))
>>>>> +		    return ba_any;
>>>>
>>>> I wonder if we want to return ba_check_bit instead of ba_any so that we
>>>> still check access of the selected base?
>>>
>>> That would certainly make sense to me.  I didn't do that because
>>> I'd not seen ba_check_bit being used except as part of ba_check,
>>> but that may not mean much.
>>>
>>> So either I can tweak the lambda to return ba_check_bit rather
>>> than ba_any or use ba_check_bit unconditionally.  Any opinions on that?
>>
>> The relevant passage seems to be
>> https://eel.is/c++draft/class.access.base#6
>> after DR 52, which seems to have clarified that the pointer conversion only
>> applies to non-static members.
>>
>>>>     struct A { constexpr static int a = 0; };
>>>>     struct D : private A {};
>>>>
>>>>     void f() {
>>>>       D{}.A::a; // #1 GCC (and Clang) currently rejects
>>>>     }
>>
>> I see that MSVC also rejects it, while EDG accepts.
>>
>> https://eel.is/c++draft/class.access.base#5.1 seems to say that a is
>> accessible when named in A.
>>
>> https://eel.is/c++draft/expr.ref#7 also only constrains references to
>> non-static members.
>>
>> But first we need to look up A in D, and A's injected-class-name looked up
>> as a member of D is not accessible; it's private, and f() is not a friend,
>> and we correctly complain about that.
>>
>> If we avoid the lookup of A in D with
>>
>> D{}.::A::a;
>>
>> clang accepts it, which is consistent with accepting the template version,
>> and seems correct.
>>
>> So, I think ba_any is what we want here.
> 
> Wow, that is not intuitive (to me at least).  So I had it right but
> only by accident.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> Given
> 
>    struct A { constexpr static int a = 0; };
>    struct B : A {};
>    struct C : A {};
>    struct D : B, C {};
> 
> we give the "'A' is an ambiguous base of 'D'" error for
> 
>    D{}.A::a;
> 
> which seems wrong: 'a' is a static data member so there is only one copy
> so it can be unambiguously referred to even if there are multiple A
> objects.  clang++/MSVC/icx agree.
> 
> The rationale for using ba_any is explained at
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638676.html>.

I'd prefer not to cite the mailing list for rationales.

To summarize:
[class.access.base] requires conversion to a unique base subobject for 
non-static data members, but it does not require that the base be unique 
or accessible for static data members.

> 	PR c++/112744
> 
> gcc/cp/ChangeLog:
> 
> 	* typeck.cc (finish_class_member_access_expr): When accessing
> 	a static data member, use ba_any for lookup_base.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/lookup/scoped11.C: New test.
> 	* g++.dg/lookup/scoped12.C: New test.
> 	* g++.dg/lookup/scoped13.C: New test.
> 	* g++.dg/lookup/scoped14.C: New test.
> 	* g++.dg/lookup/scoped15.C: New test.
> ---
>   gcc/cp/typeck.cc                       | 21 ++++++++++++++++++---
>   gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++
>   gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++
>   gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++
>   gcc/testsuite/g++.dg/lookup/scoped14.C | 14 ++++++++++++++
>   gcc/testsuite/g++.dg/lookup/scoped15.C | 20 ++++++++++++++++++++
>   6 files changed, 94 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C
>   create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C
>   create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C
>   create mode 100644 gcc/testsuite/g++.dg/lookup/scoped14.C
>   create mode 100644 gcc/testsuite/g++.dg/lookup/scoped15.C
> 
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 0839d0a4167..bf8ffaa7e75 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -3467,7 +3467,7 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
>   			   name, scope);
>   		  return error_mark_node;
>   		}
> -	
> +
>   	      if (TREE_SIDE_EFFECTS (object))
>   		val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val);
>   	      return val;
> @@ -3484,9 +3484,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
>   	      return error_mark_node;
>   	    }
>   
> +	  /* NAME may refer to a static data member, in which case there is
> +	     one copy of the data member that is shared by all the objects of
> +	     the class.  So NAME can be unambiguously referred to even if
> +	     there are multiple indirect base classes containing NAME.  */
> +	  const base_access ba = [scope, name] ()
> +	    {
> +	      if (identifier_p (name)) > +		{
> +		  tree m = lookup_member (scope, name, /*protect=*/0,
> +					  /*want_type=*/false, tf_none);
> +		  if (!m || shared_member_p (m))
> +		    return ba_any;
> +		}
> +	      return ba_check;
> +	    } ();
> +
>   	  /* Find the base of OBJECT_TYPE corresponding to SCOPE.  */
> -	  access_path = lookup_base (object_type, scope, ba_check,
> -				     NULL, complain);
> +	  access_path = lookup_base (object_type, scope, ba, NULL, complain);
>   	  if (access_path == error_mark_node)
>   	    return error_mark_node;
>   	  if (!access_path)
> diff --git a/gcc/testsuite/g++.dg/lookup/scoped11.C b/gcc/testsuite/g++.dg/lookup/scoped11.C
> new file mode 100644
> index 00000000000..be743522fce
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/scoped11.C
> @@ -0,0 +1,14 @@
> +// PR c++/112744
> +// { dg-do compile }
> +
> +struct A { const static int a = 0; };
> +struct B : A {};
> +struct C : A {};
> +struct D : B, C {};
> +
> +int main()
> +{
> +  D d;
> +  (void) d.a;
> +  (void) d.A::a;
> +}
> diff --git a/gcc/testsuite/g++.dg/lookup/scoped12.C b/gcc/testsuite/g++.dg/lookup/scoped12.C
> new file mode 100644
> index 00000000000..ffa145598fd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/scoped12.C
> @@ -0,0 +1,14 @@
> +// PR c++/112744
> +// { dg-do compile }
> +
> +class A { const static int a = 0; };
> +struct B : A {};
> +struct C : A {};
> +struct D : B, C {};
> +
> +int main()
> +{
> +  D d;
> +  (void) d.a;	  // { dg-error "private" }
> +  (void) d.A::a;  // { dg-error "private" }
> +}
> diff --git a/gcc/testsuite/g++.dg/lookup/scoped13.C b/gcc/testsuite/g++.dg/lookup/scoped13.C
> new file mode 100644
> index 00000000000..970e1aa833e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/scoped13.C
> @@ -0,0 +1,14 @@
> +// PR c++/112744
> +// { dg-do compile }
> +
> +struct A { const static int a = 0; };
> +struct B : A {};
> +struct C : A {};
> +struct D : B, C {};
> +
> +int main()
> +{
> +  D d;
> +  (void) d.x;	  // { dg-error ".struct D. has no member named .x." }
> +  (void) d.A::x;  // { dg-error ".struct A. has no member named .x." }
> +}
> diff --git a/gcc/testsuite/g++.dg/lookup/scoped14.C b/gcc/testsuite/g++.dg/lookup/scoped14.C
> new file mode 100644
> index 00000000000..141aa0d2b1a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/scoped14.C
> @@ -0,0 +1,14 @@
> +// PR c++/112744
> +// { dg-do compile { target c++11 } }
> +
> +struct A { int a = 0; };
> +struct B : A {};
> +struct C : A {};
> +struct D : B, C {};
> +
> +int main()
> +{
> +  D d;
> +  (void) d.a;	  // { dg-error "request for member .a. is ambiguous" }
> +  (void) d.A::a;  // { dg-error ".A. is an ambiguous base of .D." }
> +}
> diff --git a/gcc/testsuite/g++.dg/lookup/scoped15.C b/gcc/testsuite/g++.dg/lookup/scoped15.C
> new file mode 100644
> index 00000000000..d450a41a617
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/scoped15.C
> @@ -0,0 +1,20 @@
> +// PR c++/112744
> +// { dg-do compile { target c++11 } }
> +
> +struct A { constexpr static int a = 0; };
> +struct D : private A {};
> +
> +// See <https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638676.html>
> +// for rationale.

The injected-class-name of A is private when named in D, but if A is 
named some other way, there is no requirement in [class.access.base] for 
static data members that it be an accessible base.

OK with those comment adjustments.

Jason