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

[Jason Merrill]

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.

Jason