[generator] better support for "package-private" classes

[jonathanpeppers]

Fixes #168

Java allows public classes to inherit from "package-private" classes, in
which case the public & protected members of the "package-private"
class are visible within the public class:

// Java
/* package */ abstract class SpannableStringInternal {
  public void getChars(int start, int end, char[] dest, int off);
  // ...
}
public class SpannableString extends SpannableStringInternal {
  // ...
}

The problem is that generator didn't properly support this construct,
and skips binding of "package-private" types, resulting in generated
C# code such as:

// C#
public partial class SpannableString : SpannableStringInternal {
  // CS0246: The type or namespace name `SpannableStringInternal` could not be found
}

This could be worked aroudn via Metadata by making the "package-private"
class public, but this means that if (when?) the "package-private"
class is removed, we have an ABI break.

Support this construct by updating generator to "copy" the members
from the "package-private" class into the declaring class:

// C#
public partial class SpannableString : Java.Lang.Object {
  public void GetChars(int start, int end, char[] dest, int off);
  // ...
}

This allows the generated code to compile without metadata fixup.

Specifics in implementing this:

• Add a FixupAccessModifiers method to GenBase, later this may
  need to be further extended for methods
• Call FixupAccessModifiers in the "validation" step
• Added a setter for BaseType so it can be modified
• In FixupAccessModifiers, lookup the base class of the current type
  and check if it is non-public
• Skip the package-private types, and replace them with that class's
  base type
• Look for each method on the base type, and copy it to the public type
  if it does not exist
• Added tests for this scenario

[jonathanpeppers]

I found that old binding project that ran into this issue, it was a sample from my Xamarin book.

Using generator from this PR allows me to remove this:
https://github.com/jonathanpeppers/GoogleAnalytics/blob/master/GoogleAnalytics.Droid/Transforms/Metadata.xml#L19

[jonpryor]

As with PR #216, this is going to have knock-on impacts of xamarin-android, particularly around Mono.Android.dll ABI.

In order to accept this patch, we're going to need to have the xamarin-android-side patch to allow things to continue to build.

[jonpryor]

Additionally, I do not want to merge this PR or PR #216 before the d15-6 branch date. We'll merge these for d15-7.

[atsushieno]

This should throw InvalidOperationException so that anyone who works on generator should immediately notice that it is inappropriately used and not hiding the actual issue.

[jonathanpeppers]

Yeah, I don't think we should merge this one or #216 until d15-7. Especially if I'm going to be on leave at the beginning of next year.

[jonathanpeppers]

Note the VSTS: Java.Interop Integration (Mac) status check.

It has a report of a master/master build of java.interop/xamarin-android of this PR. (More details on the private monodroid repo's wiki)

The results are:

• java.interop tests passed
• api-compatibility tests passed
• the diff of Mono.Android/obj/Debug/android-27/mcwwas identical

I'm still working on automating the same kind of build on some of Redth's components.

[jonpryor]

My general idea is to make PackageClass in the above example public in C#

I don't like this idea, as it creates a potential API compatibility problem. The whole point is that PackageClass is not public, and thus it's name could change over time.

Meaning this is entirely plausible:

// Version 1
/* package */ class Base1 {
    public void foo() {}
}

public class TheFooinator extends Base1 {
    /* foo() is automagically a public member */
}

and then for version 2:

// version 2
/* package */ class RefactoredBase {
    public void foo() {}
}

public class TheFooinator extends RefactoredBase {
    /* foo() is automagically a public member */
}

By making Base1 public, you're assuming that it'll actually stick around, which is not necessarily the case.

[jonathanpeppers]

I've started reworking this PR to just not emit the "package private" classes in C# at all.

I still need to:

• Check xamarin-android upstream (see Java.Interop Integration check, has api-compatibility-tests and diff's the mcw directory)
• Check the binding project I mentioned above (worked w/ changes from this PR)

[jonathanpeppers]

@jonpryor @atsushieno I've reworked this so it's ready for review again.

We basically just needed to prevent the "package private" types from being listed as base classes in C#.

[atsushieno]

It is not required (no need to do unless you are making further changes) but a suggested change is to use NotSupportedException instead of NIE. NIE means "we plan to implement" or "any derived classes are supposed to implement".

[jonpryor]

Please update the commit message/PR message to:

1. "Causes invalid C# to be generated" -- elaborate on how it causes "invalid C# to be generated." I know (now!) that it's because a type will be referenced that doesn't exist, but that's not explicitly mentioned, and we may very well forget that in 5 years time.

2. I don't know what the "fix" means. Provide an example. What (step-by-step) it does can be read from the commit itself; What it does and why it does it cannot. My guess as to "what" it's doing is "copy the members of the package-private class into the public class", which is easier to quickly understand than the bulleted list provided.

3. Examples of what the fix does.

Additionally, please don't rely on #number for issues/PRs/etc, use the full URL instead. This makes things easier to copy when doing "bump" commits within xamarin-android, and if (lol?) we move issue trackers again, it'll allow us to know which issue tracker it came from.

---

Suggested commit message:

Fixes: https://github.com/xamarin/java.interop/issues/168

Java allows public classes to inherit from "package-private" classes,
in which case the `public` & `protected` members of the
"package-private" class are visible within the public class:

	// Java
	/* package */ abstract class SpannableStringInternal {
	  public void getChars(int start, int end, char[] dest, int off);
	  // ...
	}
	public class SpannedString extends SpannableStringInternal {
	  // ...
	}

The problem is that `generator` didn't properly support this
construct, and *skips* binding of "package-private" types, resulting
in generated C# code such as:

	// C#
	public partial class SpannedString : SpannableStringInternal {
	  // CS0246: The type or namespace name `SpannableStringInternal' could not be found.
	}

This could be worked around via Metadata by making the
"package-private" class `public`, but this means that if (when?) the
"package-private" class is *removed*, we have an ABI break.

Support this construct by updating `generator` to "copy" the members
from the "package-private" class into the declaring class:

	// C#
	public partial class SpannedString : Java.Lang.Object {
	  public void GetChars(int start, int end, char[] dest, int off);
	  // ...
	}

This allows the generated code to compile without metadata fixup.

Also note that I personally don't like backtick-backtick-backtick blocks within commit messages, as they make it harder to skim & read the content. I prefer indentation, as shown above. YMMV.

[jonpryor]

Does this need to be "recursive"?

Consider:

public class A {
  public void a() {}
}
/* package */ class B extends A {
  public void b() {}
}

/* package */ class C extends B {
  public void c() {}
}

public class P extends C {
  public void p() {}
}

$ javap -cp . P
Compiled from "P.java"
public class P extends C {
  public P();
  public void p();
  public void c();
  public void b();
}

From our perspective, we can't have P inherit C, as C is package-private, but it should inherit from A, which is public:

public partial class A {
    public virtual void InvokeA() {...}
}
public partial class P : A {
    public virtual void B() {...}
    public virtual void C() {...}
    public virtual void InvokeP() {...}
}

I'm not certain that this statement will support the above.

---

Aside: the above javap output is bonkers: it's missing A.a()!

This despite the fact that it is callable!

public class App {
  public static void main (String[] args) {
    P p = new P();
    p.a();
  }
}

¯_(ツ)_/¯

[jonathanpeppers]

@jonpryor FYI this time I put the full issue URL in the commit message locally:

Is the web rendering the issue number shortened?

[jonpryor]

Is the web rendering the issue number shortened?

¯_(ツ)_/¯

I hit the "Edit comment" link/button on the PR message, and that contains:

Fixes #168

Normally that value is pre-filled with the commit-message contents...

[jonathanpeppers]

Ok next time I’ll edit the PR text after the fact, I bet that works.