[class-parse] fix class visibility XML output.

[atsushieno]

As part of fixing MSBuild test regression[*1], I noticed that class-parse
does not generate correct visibility for class/interface elements, which
could affect further API metadata processing. It should not generate
"private" for nested types when it should be "" (package private).

[*1] it should be ported to OSS...

[jonpryor]

This results PR results in a unit test failure (yay unit tests!):

1) Test Failure : Xamarin.Android.Tools.BytecodeTests.JavaType_RNCTests.XmlDescription
  Expected: "...ic="false"\n      visibility="protected">\n      <typeParame..."
  But was:  "...ic="false"\n      visibility="public">\n      <typeParameter..."
  ---------------------------------------------^

• Test

• Expected output:

  <class
    abstract="true"
    deprecated="not deprecated"
    jni-extends="Ljava/lang/Object;"
    extends="java.lang.Object"
    extends-generic-aware="java.lang.Object"
    final="false"
    name="JavaType.RNC"
    jni-signature="Lcom/xamarin/JavaType$RNC;"
    static="false"
    visibility="protected">
  

The complaint is that visibility went from protected to public. What should it be? protected:

public class JavaType<T> ... {
    protected abstract class RNC<E2> {
        ...
    }
}

The test appears to be correct: JavaType.RNC should be protected.

[atsushieno]

Oops, the replacement value was wrong. It should be GetClassVisibility() result for the same argument.

[jonpryor]

I don't understand the bug or the fix. Perhaps we need a new unit test in JavaTest.java?

In particular, consider GetVisibility(ClassAccessFlags) and GetClassVisibility(ClassAccessFlags): they're identical. (Looks like this file could use some cleanup. Doh!)

Consequently, afaict, this change doesn't do anything, since the original method and the new called method both do the same thing.

[atsushieno]

damn, you're right. My update should have been twofolds: fix regression and fix the actual issue. The latter went away.

I'm going to push my fixity fix, but needs more time to understand how the tests are written and built, so without test right now.

[jonpryor]

I don't understand the updated PR. I don't understand why giving ClassAccessFlags.Private no visibility ("") instead of private visibility is a correct fix.

[atsushieno]

At least current class-parse behavior is a breaking change.

For this Java class and nested classes:

package com.xamarin.testing.bindings;

public class Test
{
    public class NestedPublic
    {
    }

    protected class NestedProtected
    {
    }

    private class NestedPrivate
    {
    }

    class Default
    {
    }
}

jar2xml generates this element in api.xml:

<class abstract="false" deprecated="not deprecated" extends="java.lang.Object" extends-generic-aware="java.lang.Object" final="false" name="Test.NestedPrivate" static="false" visibility=""/>

where class-parse generates visibility="private".

[atsushieno]

Having said that, jar2xml implemntation is not very logical on that context:
https://github.com/xamarin/jar2xml/blob/master/JavaClass.java#L601

The "cleaner" solution would be to discard this change and let class-parse keep current behavior, if we don't mind changes that possibly introduces breakage that could be caused by existing metadata fixup.

I tend to say we should accept the change for the right and cleaner solution (discard this PR).

[jonpryor]

I'll agree that this is a semantic change, but that's in part because jar2xml behavior here is stupid. ;-)

(package-private isn't private accessibility, and in all manner of contexts .class files distinguish between package-private and private.)

I also don't understand how this change breaks anything. By default, generator shouldn't emit bindings for visibility="" (package-private) or visibility="private" (private) classes, so how does this change break anything?

[atsushieno]

As my original commit message says, it is about API metadata difference. It is NOT about "breakage" (whatever you think is).

[atsushieno]

A very possible breakage scenario is, if you have something like this in your Metadata.xml:

<attr path="/path/to/class[@visibility='']" name="visibility">public</attr>

It will not hit the element anymore in the current class-parse land.

[jonpryor]

A very possible breakage scenario is, if you have something like this in your Metadata.xml:

This is very true. I'm not sure that we should care.

I would like class-parse output to be as close to the Bytecode declarations as possible -- which would imply rejecting this PR -- and I believe that this "breakage scenario" is unlikely to be hit (IMHO).

I don't think that we should merge this PR, unless we we see that the possible breakage is widespread and/or detrimental.

For example, if you're going to set visibility, why query it in the first place? Just set it:

<attr path="//class[@name='Whatever']" name="visibility">public</attr>

[atsushieno]

The concern is almost only about existing binding library sources. Your suggestion totally works for newly written libraries.

Anyhow I'm totally fine with this decision, everyone should embrace this good change in the new API XML outputs.