[api-xml-adjuster] method overrides cannot expose non-public types. #94

atsushieno · 2016-10-11T07:47:38Z

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=44531

When a Java method is overriden, its parameter type or return type can be
variant, which possibly result in exposure of non-public types (returning
private class object which is derived from public type is totally valid).

It is because we blindly emit the parsed type from class-parse output
as the API XML adjuster output. It needs to be examined and adjusted to
be public (or protected) type.

jar2xml didn't have this problem because Java reflection API is clever
enough to cover that issue.

The fixes are... not cosmetic.

Basically now we use TypeReference.ToString() which used to not be
precise and ready for API XML output. So there are couple of fixes
for that. (We also want to track from which method base a parameter
or return type is bound, so we pass that to the .ctor now).

We hide extraneous public TypeParameter.ctor() now to reduce chances
for bugs. Tests need to be modified to reflect the changes.

Lastly, Mono.Android API XML will be updated to reflect this change.
There will be some removal of method overrides from api-*.xml.in but
there was no regression reported from api-diff.

…ster. There were non-breaking API definition files changes caused by dotnet/java-interop#94 This update reflects the changes.

jonpryor · 2016-10-11T14:02:08Z

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs

+			Parent = parent;
+		}
+
+		public JavaMethodBase Parent { get; set; }


Does this need a property setter?

It can be private.

jonpryor · 2016-10-11T18:34:19Z

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs

@@ -309,7 +315,7 @@ public JavaTypeParameter (JavaTypeParameters parent)

 		public override string ToString ()
 		{
-			return Name + (GenericConstraints == null ? null : GenericConstraints.ToString ());
+			return Name;


Why the removal of the constraints from JavaTypeParameter?

Basically we have right to choose what to output in this ToString() overrides.

There are places that should output generic constraints and that should NOT. When writing generic type parameter definitions they should be there. When writing USE of generic type parameters they should NOT. And this generic constraints part were always written.

An exceptional case that it SHOULD be written for USE of generics is that the name is "?" which is handled specially.

jonpryor · 2016-10-11T18:35:41Z

Are the changes in this xamarin-android PR from before or after this PR?

jonpryor · 2016-10-11T18:38:19Z

src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/TypeResolverTest.cs

@@ -18,19 +18,14 @@ public void SetupFixture ()
 		[Test]
 		public void TypeReferenceEquals ()
 		{
-			var intRef = new JavaTypeReference ("int");
-			Assert.AreEqual (intRef, new JavaTypeReference ("int"), "primitive types");


Why are these tests invalidated? (Presumably they're invalidated, otherwise why would they be removed?)

With the revised API (JavaTypeReference.ctor() being non-public), those removed lines don't compile, so they were deleted.

atsushieno · 2016-10-11T20:16:59Z

The changes for that xamarin-android PR is after this change.

jonpryor · 2016-10-12T01:37:46Z

The changes for that xamarin-android PR is after this change.

Which means it's responsible for this change, which I don't understand or like:

-        <parameter name="transformer" type="java.util.function.Function&lt;java.util.Map.Entry&lt;K, V&gt;, ? extends U&gt;">
+        <parameter name="transformer" type="java.util.function.Function&lt;java.util.Map.Entry&lt;K, V&gt;&gt;">

Why would we want to lose ? extends U?

atsushieno · 2016-10-12T08:46:33Z

Goooood catch, kind of ;-) I thought (I guess you did too) that it is only missing "? extends U" and thought it's harmless, but it is actually ", ? extends U" (note that the first comma), so it's just dropping subsequent generic arguments than the first one.

I'm rolling a fix, which might be pushed soon or might need more fixes.

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=44531 When a Java method is overriden, its parameter type or return type can be variant, which possibly result in exposure of non-public types (returning private class object which is derived from public type is totally valid). It is because we blindly emit the parsed type from class-parse output as the API XML adjuster output. It needs to be examined and adjusted to be public (or protected) type. jar2xml didn't have this problem because Java reflection API is clever enough to cover that issue. The fixes are... not cosmetic. Basically now we use TypeReference.ToString() which used to not be precise and ready for API XML output. So there are couple of fixes for that. (We also want to track from which method base a parameter or return type is bound, so we pass that to the .ctor now). We hide extraneous public TypeParameter.ctor() now to reduce chances for bugs. Tests need to be modified to reflect the changes. Lastly, Mono.Android API XML will be updated to reflect this change. There will be some removal of method overrides from api-*.xml.in but there was no regression reported from api-diff.

…ster. There were non-breaking API definition files changes caused by dotnet/java-interop#94 This update reflects the changes.

atsushieno · 2016-10-12T09:56:31Z

The PRs are updated to reflect the fixes.

jonpryor · 2016-10-13T15:56:57Z

src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/JavaApiTest.cs

+			Assert.AreEqual ("V", ga1.GenericArguments [1].FullNameNonGeneric, "genarg#0.2 name mismatch");
+			var ga2 = tn.GenericArguments [1];
+			Assert.AreEqual ("?", ga2.FullNameNonGeneric, "genarg#1 name mismatch");
+			Assert.AreEqual (" extends ", ga2.BoundsType, "genarg#1 incorrect bounds type");


This is a bit "odd", in that it means that ga2.BoundsType contains leading and trailing spaces, e.g. this is " extends ", not "extends".

Should the BoundsType property not contain spaces, while the spaces are included in e.g. ga2.ToString()?

Of course I thought about that, but I didn't find very reason to perform extra string stripping and cause more memory consumption.

…adjuster (#260) There were non-breaking API definition files changes caused by dotnet/java-interop#94 This update reflects the changes. Additionally, the pre-API-24 API descriptions were generated by `jar2xml`, not `class-parse`, which generated extraneous generic constraints. Update all API descriptions to be generated by `class-parse` and `api-xml-adjuster`.

Changes: dotnet/android-tools@3974fc3...f2af06f * dotnet/android-tools@f2af06f: [Xamarin.Android.Tools.AndroidSdk] Fix a few nullability warnings (#97) * dotnet/android-tools@5718cd2: Fix sort ordering for ndk-bundle, add macOS support (#91) * dotnet/android-tools@8e63795: [Xamarin.Android.Tools.AndroidSdk] Add API-29, API-30 to KnownVersions (#89) * dotnet/android-tools@a6a23bb: [Xamarin.Android.Tools.AndroidSdk] Default SDK component versions (#93) * dotnet/android-tools@32a1e2c: [build] fail macOS build if tests fail (#94) * dotnet/android-tools@79a0141: Return a default for unknown API levels (#90)

dnfclas added the cla-already-signed label Oct 11, 2016

jonpryor reviewed Oct 11, 2016

View reviewed changes

atsushieno force-pushed the generator-do-not-expose-nonpublic branch from f2b8f32 to a4d5583 Compare October 11, 2016 15:27

jonpryor reviewed Oct 11, 2016

View reviewed changes

atsushieno force-pushed the generator-do-not-expose-nonpublic branch from a4d5583 to 5e91472 Compare October 12, 2016 09:54

jonpryor reviewed Oct 13, 2016

View reviewed changes

jonpryor merged commit 8dac926 into dotnet:master Oct 13, 2016

github-actions bot locked and limited conversation to collaborators Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-xml-adjuster] method overrides cannot expose non-public types. #94

[api-xml-adjuster] method overrides cannot expose non-public types. #94

atsushieno commented Oct 11, 2016

jonpryor Oct 11, 2016

atsushieno Oct 11, 2016

jonpryor Oct 11, 2016

atsushieno Oct 12, 2016

jonpryor commented Oct 11, 2016

jonpryor Oct 11, 2016

atsushieno Oct 11, 2016

atsushieno commented Oct 11, 2016

jonpryor commented Oct 12, 2016 •

edited

Loading

atsushieno commented Oct 12, 2016

atsushieno commented Oct 12, 2016

jonpryor Oct 13, 2016 •

edited

Loading

atsushieno Oct 13, 2016

[api-xml-adjuster] method overrides cannot expose non-public types. #94

[api-xml-adjuster] method overrides cannot expose non-public types. #94

Conversation

atsushieno commented Oct 11, 2016

jonpryor Oct 11, 2016

Choose a reason for hiding this comment

atsushieno Oct 11, 2016

Choose a reason for hiding this comment

jonpryor Oct 11, 2016

Choose a reason for hiding this comment

atsushieno Oct 12, 2016

Choose a reason for hiding this comment

jonpryor commented Oct 11, 2016

jonpryor Oct 11, 2016

Choose a reason for hiding this comment

atsushieno Oct 11, 2016

Choose a reason for hiding this comment

atsushieno commented Oct 11, 2016

jonpryor commented Oct 12, 2016 • edited Loading

atsushieno commented Oct 12, 2016

atsushieno commented Oct 12, 2016

jonpryor Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

atsushieno Oct 13, 2016

Choose a reason for hiding this comment

jonpryor commented Oct 12, 2016 •

edited

Loading

jonpryor Oct 13, 2016 •

edited

Loading