Comment 1 Andrew Eidsness 2014-01-15 20:52:14 EST

The difference seems to be the instanceof checks in AccessContext.isAccessible( IBinding ).

public boolean isAccessible(IBinding binding) {
    int bindingVisibility;
    if (binding instanceof ICPPMember) {
        bindingVisibility = ((ICPPMember) binding).getVisibility();
    } else {
        while (binding instanceof ICPPSpecialization) {
            binding = ((ICPPSpecialization) binding).getSpecializedBinding();
        }
        if (binding instanceof ICPPClassTemplatePartialSpecialization) {
        // A class template partial specialization inherits the visibility of its primary class template. 
            binding = ((ICPPClassTemplatePartialSpecialization) binding).getPrimaryClassTemplate();
        }
        if (binding instanceof ICPPAliasTemplateInstance) {
            binding = ((ICPPAliasTemplateInstance) binding).getTemplateDefinition();
        }
        IBinding owner = binding.getOwner();
        if (owner instanceof ICPPClassType) {
            bindingVisibility = ((ICPPClassType) owner).getVisibility(binding);
        } else {
            bindingVisibility = v_public;
        }
    }
    return isAccessible(binding, bindingVisibility);
}

When the class definition contains "int I;", then the IBinding is a CompositeCPPField, which is an instance of ICPPMember, so the first condition is used.

When the class definition contains "typedef int I;", then the IBinding is a CompositeCPPTypedef then it goes all the way down to using the owner and the call to ICPPClassType.getVisibility throws the exception.

The annotations on this method point to Bug 418479, which has a very similar description.  That bug is focused on templates, but this example does not use any.

Comparing that behaviour between the the working and not working cases, the difference is that in the non-working case owner is an instance of CompositeCPPClassType.  In the working case it is an instance of CPPClassType.

In the working case the owner is treated as a ICPPInternalClassTypeMixinHost, but that interface is not visible to the definition of CompositeCPPClassType.

----

I'm not sure if the right fix is to change CompositeCPPClassType so that it can be treated as an ICPPInternalClassTypeMixinHost or to resolve the CompositeCPPTypedef to the PDOMCPPTypedef to avoid the IllegalArgumentException.

Comment 2 Andrew Eidsness 2014-01-15 20:59:34 EST

(In reply to Andrew Eidsness from comment #1)

> Comparing that behaviour between the the working and not working cases, the
> difference is that in the non-working case owner is an instance of
> CompositeCPPClassType.  In the working case it is an instance of
> CPPClassType.

Just to clarify this part.  I meant the behaviour between working Cls2 and non-working Cls1 (when both contain typedefs).  Previous explanations in this bug were comparing two definitions for Cls1:

    class Cls1 { public: typedef int I; };
and class Cls1 { public:         int I; };

Comment 3 Sergey Prigogin 2014-01-15 21:06:48 EST

(In reply to Andrew Eidsness from comment #2)
A more meaningful comparison is between the case with dependent projects and the case with a single project.

Comment 4 Nathan Ridge 2014-01-15 21:27:08 EST

> I'm not sure if the right fix is to change CompositeCPPClassType so that it
> can be treated as an ICPPInternalClassTypeMixinHost 

I don't think we can do this. The method "ICPPASTCompositeTypeSpecifier getCompositeTypeSpecifier()" in ICPPInternalClassTypeMixinHost cannot be implemented for an index binding because there is no AST to work with, and CPPClassType.getVisibility() relies on this method.

> or to resolve the
> CompositeCPPTypedef to the PDOMCPPTypedef to avoid the
> IllegalArgumentException.

That sounds reasonable to me. Composite bindings have a getRawBinding() method (defined in CompositeIndexBinding) that returns the underlying PDOM binding. We can probably call this in PDOMCPPClassType.getVisibility().

Comment 5 Nathan Ridge 2014-01-15 21:30:07 EST

(In reply to Nathan Ridge from comment #4)
> > or to resolve the
> > CompositeCPPTypedef to the PDOMCPPTypedef to avoid the
> > IllegalArgumentException.
> 
> That sounds reasonable to me. Composite bindings have a getRawBinding()
> method (defined in CompositeIndexBinding) that returns the underlying PDOM
> binding. We can probably call this in PDOMCPPClassType.getVisibility().

Actually, it would probably be even better for CompositeCPPClassType.getVisibility() to unwrap the member binding prior to passing it to PDOMCPPClassType.getVisibility().

Comment 6 Sergey Prigogin 2014-01-15 21:44:14 EST

(In reply to Nathan Ridge from comment #4)
> That sounds reasonable to me. Composite bindings have a getRawBinding()
> method (defined in CompositeIndexBinding) that returns the underlying PDOM
> binding. We can probably call this in PDOMCPPClassType.getVisibility().

Better to change PDOMCPPMemberBlock.getVisibility(PDOMCPPMemberBlock, IBinding) by replacing

    if (!(member instanceof PDOMNode))
        return -1;
		
    PDOMNode memberNode = (PDOMNode) member;
    if (memberNode.getPDOM() != block.getPDOM())
        return -1;

by

    PDOMNode memberNode = (PDOMNode) block.getPDOM().adaptBinding(member);
    if (memberNode == null)
        return -1;

Comment 7 Nathan Ridge 2014-01-15 22:03:36 EST

(In reply to Sergey Prigogin from comment #6)
> (In reply to Nathan Ridge from comment #4)
> > That sounds reasonable to me. Composite bindings have a getRawBinding()
> > method (defined in CompositeIndexBinding) that returns the underlying PDOM
> > binding. We can probably call this in PDOMCPPClassType.getVisibility().
> 
> Better to change PDOMCPPMemberBlock.getVisibility(PDOMCPPMemberBlock,
> IBinding) by replacing
> 
>     if (!(member instanceof PDOMNode))
>         return -1;
> 		
>     PDOMNode memberNode = (PDOMNode) member;
>     if (memberNode.getPDOM() != block.getPDOM())
>         return -1;
> 
> by
> 
>     PDOMNode memberNode = (PDOMNode) block.getPDOM().adaptBinding(member);
>     if (memberNode == null)
>         return -1;

Oh neat! I didn't know adaptBinding() could do a Composite -> PDOM adaptation, I thought it was for AST -> PDOM only.

Comment 8 CDT Genie 2014-01-15 23:23:01 EST

*** cdt git genie on behalf of Sergey Prigogin ***

    Bug 425833 - IllegalArgumentException I is not a member of Cls1
    Change-Id: I61d52d2585142c15be21638e614d5b8daa13dfae

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=8d14ab8793d3a50857bcb159242146f654a77af3

Comment 9 Sergey Prigogin 2014-01-15 23:28:22 EST

(In reply to CDT Genie from comment #8)
Andrew, please check if the issue is fixed now.

Comment 10 Andrew Eidsness 2014-01-16 07:36:58 EST

Perfect, thanks.

Its fixed in both my toy sample and in the original, large project.