Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[class-parse] fix class visibility XML output. #83

Closed

Conversation

@atsushieno
Copy link
Contributor

commented Sep 20, 2016

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 comment has been minimized.

Copy link
Member

commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

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

@atsushieno atsushieno force-pushed the atsushieno:class-parse-fix-visibility branch from 5bf462b to 63dc5db Sep 20, 2016

@jonpryor

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2016

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.

[class-parse] fix class visibility XML output.
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...

@atsushieno atsushieno force-pushed the atsushieno:class-parse-fix-visibility branch from 63dc5db to 9434fb9 Sep 21, 2016

@jonpryor

This comment has been minimized.

Copy link
Member

commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2016

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

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

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

@atsushieno

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

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

This comment has been minimized.

Copy link
Member

commented Oct 13, 2016

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>

@jonpryor jonpryor closed this Oct 13, 2016

@atsushieno

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.