[generator] do not emit static properties as instance. #159

atsushieno · 2017-06-16T15:53:24Z

Without this, abstract classes that implement java.time.chrono.Chronology
fail to bind because they will try to implement those static properties
as instance (this check had been done for methods, but not for properties).

Without this, abstract classes that implement java.time.chrono.Chronology fail to bind because they will try to implement those static properties as instance (this check had been done for methods, but not for properties).

jonpryor · 2017-06-16T16:06:39Z

Tests? :-D

atsushieno · 2017-06-16T16:15:59Z

Tests are implemented as Mono.Android API api-diff; if things don't work as expected, changes to Mono.Android.dll build within xamarin-android will not build at all. If it works as expected, it will build.

Since xamarin-android is in separate repository, it is impossible to bring that test as part of this repository.

... that depended on generator fixes. The corresponding generator fixes are: dotnet/java-interop#153 dotnet/java-interop#159 With those changes, this can be brought back to the public API.

jonpryor · 2017-06-16T16:50:24Z

That doesn't mean we can't have a minimal test case. The problem, as I understand it, is if we bind:

public interface Fooable {
    public static int getFooableCount() {
        return 42;
    }
}

then our C# binding will incorrectly be:

public interface IFooable {
    public int FooableCount {get;}
}

If so, this can be easily tested by updating tools/generator/Tests/expected/TestInterface/TestInterface.xml so that TestInterface contains a static get method:

<method abstract="false" deprecated="not deprecated" final="true" name="getFooableCount" native="false" return="int" static="true" synchronized="false" visibility="public">
</method>

atsushieno · 2017-06-16T17:30:58Z

It is not as easy as keeping producivity.

jonpryor · 2017-06-16T20:57:34Z

It is not as easy as keeping producivity.

I believe that this is false equivalence, trading one "kind" of productivity for another.

Case in point: this PR is buggy. Given the change suggested earlier:

diff --git a/tools/generator/Tests/expected/TestInterface/TestInterface.xml b/tools/generator/Tests/expected/TestInterface/TestInterface.xml
index b490566..2f33295 100644
--- a/tools/generator/Tests/expected/TestInterface/TestInterface.xml
+++ b/tools/generator/Tests/expected/TestInterface/TestInterface.xml
@@ -21,6 +21,8 @@
 			}
 		-->
 		<interface abstract="true" deprecated="not deprecated" final="false" name="TestInterface" static="false" visibility="public">
+			<method abstract="false" deprecated="not deprecated" final="true" name="getFooableCount" native="false" return="int" static="true" synchronized="false" visibility="public">
+			</method>
 			<method abstract="false" deprecated="not deprecated" final="false" name="defaultInterfaceMethod" native="false" return="void" static="false" synchronized="false" visibility="public">
 			</method>
 			<method abstract="true" deprecated="not deprecated" final="false" name="getSpanFlags" native="false" return="int" static="false" synchronized="false" visibility="public">

The result of running:

$ xbuild tools/generator/Tests/generator-Tests.csproj
$ make run-tests TESTS=bin/TestDebug/generator-Tests.dll

is that there are no failures.

Whereas there should be failures. Specifically, there should be a new TestInterface.FooableCount property! But there isn't.

Were we to accept this PR as-is and bump xamarin-android, the result would be that we could now bind java.time.chrono.Chronology (yay), but it would be incomplete, and would not bind the Chronology.getAvailableChronologies() method in any way.

I don't consider this a "good" state of affairs, and adding the two line test to this PR would have caught that, as opposed to a probable alternative process of "accept the PR, merge to x-a, build, wait some time, have someone file a bug report that here is no Chronology.AvailableChronologies property, and then fix the oversight in generator."

Which process, again, is more productive?

It does not mean everything in this change is about enumification - actually only few changes are relevant. It is wrong attitute because it mixes different fixes, but that's what I was asked to do. Here is the list of the reported breaking changes: Perceptible = 130 230 Google changed the value! public RemoteInput.Builder SetAllowFreeFormInput (bool allowFreeFormInput allowFreeFormTextInput) The bare parameter name changes that we already resolved as "renaming to the new name is the way what we do". api-diff most likely needs fix to not report this kind of changes. public virtual void OnServiceAdded (ProfileState status, BluetoothGattService service); It is actually a bugfix: https://bugzilla.xamarin.com/show_bug.cgi?id=55473 public AdvertiseTx int TxPowerLevel { get; } It is also a bugfix for https://bugzilla.xamarin.com/show_bug.cgi?id=51293 public virtual Android.Database.ICursor Query (Android.Net.Uri url uri, string[] projection, string selection, string[] selectionArgs, string sortOrder, Android.OS.CancellationSignal cancellationSignal) public virtual void OnError (string itemId mediaId) public virtual bool EnableNetwork (int netId, bool disableOthers attemptConnect) public virtual SubscriptionInfo GetActiveSubscriptionInfoForSimSlotIndex (int slotIdx slotIndex) public virtual string GetDeviceId (int slotId slotIndex) public virtual void SetMaxEms (int maxems maxEms) public virtual void SetMaxHeight (int maxHeight maxPixels) (... and all those relevant methods on Android.Widget.TextView) public bool IsAnnotationPresent (Class annotationType annotationClass) public bool IsAssignableFrom (Class c cls) public bool IsInstance (Object object obj) public virtual bool IsAnnotationPresent (Class annotationType annotationClass) public virtual void TraceInstructions (bool enable on) public virtual void TraceMethodCalls (bool enable on) public virtual bool IsAnnotationPresent (Java.Lang.Class annotationType annotationClass) public Java.Lang.Object NewInstance (Java.Lang.Object[] args initargs) public string GetDisplayScript (Locale locale inLocale) public string GetDisplayVariant (Locale locale inLocale) public virtual int NextInt (int n bound) public CopyOnWriteArrayList (Java.Lang.Object[] array toCopyIn) (... and all those lines in Java.Util.Concurrent.CopyOnWriteArrayList) They are all parameter name fixes in Android API. MaskFlags = 4080 65520 Google changed the constant value here too. Well, it should. Java.Lang.IAutoCloseable This is a bogus report. It sure adds another interface but it does not bring in any breaking change because the derived interface XmlResourceParser also contains the only member in AutoCloseable i.e. close() ! api-check must not report this as a breaking change. This applies to all other interfaces that report the same error too. public virtual View KeyboardNavigationClusterSearch (View p0, FocusSearchDirection p1); Why is this happening? Because they are static methods that should not be part of the interface member. Why does this happen? Because this fix is not merged! dotnet/java-interop#159 That fix really needs to be merged otherwise we keep generating these API incomatibility forever. The issue on java.time.chrono API mentioned there is actually due to totally different issue from the static <-> instance changes, so there is no excuse to not fix the static issue. public IAppendable Append (string s, int start, int end); Fixed: see src/Mono.Android/Java.Lang/StringBuffer.cs and src/Mono.Android/Java.Lang/StringBuilder.cs for details. <h3>Removed Type Java.Lang.Reflect.Constructor.InterfaceConsts</h3> <h3>Removed Type Java.Lang.Reflect.Method.InterfaceConsts</h3> They must be brought be intrusive Google API changes due to the step-by-step migration from Harmony to OpenJDK. There are no members that used those consts and should be simply killed even without changes. We can take bugs IF ANYONE EVER used it. I don't waste my time on "fixing" this. public virtual bool IsAnnotationPresent (Java.Lang.Class annotationClass); public virtual void AddPropertyChangeListener (Java.Beans.IPropertyChangeListener listener); public virtual void RemovePropertyChangeListener (Java.Beans.IPropertyChangeListener listener); (in both Pack200.IPacker and Pack200.IUnpacker) public virtual bool IsDestroyed { get; } public virtual void Destroy (); They are all api-merge bug: why does api-merge preserve this method as a default interface method?? It had been non-default in earlier API Levels, therefore api-merge must generate this method as non-default. IAnnotatedElement This is new in java.lang.reflect.GenericDeclaration interface and should also be part of huge Harmony - OpenJDK migration. And unlike other new additional interfaces, those methods in AnnotatedElement are not default, so they are indeed breaking changes. At this state, I don't think there is anything we can help the situation. We cannot remove this base interface type because that will generate JCW that doesn't compile. The only idea I can think of is to completely kill this type. It's just like we give up EntityIterator. public virtual FileChannel Position (long newPosition); public virtual FileChannel Truncate (long size); public System.Threading.Tasks.Task<FileChannel> TruncateAsync (long size); This was actually newly introduced breakage for OpenJDK migration too. FileChannel now implements SeekableByteChannel, which never existed, and they require those methods to return ISeekableByteChannel in C#, not FileChannel whereas FileChannel **is** ISeekableByteChannel. So they are changed in the metadata fixup, but then this breakage. Now Position() and Truncate() are added as explicit interface implementation. However, what can we do for TruncateAsync() ? NOTHING! It is (semi-)automatically generated by `generateAsyncWrapper='true'` which is NOT defined in the interface and therefore it cannot be explicitly implemented. And it's all about return types. Javax.Security.Auth.IDestroyable This should not be reported because all those interface methods are default! <h3>Removed Type Android.Service.Quicksettings.TileState</h3></div>  I explicitly removed this. It had been marked as [Obsolete].

atsushieno · 2017-08-22T06:58:22Z

Why have you been keeping this pending while you have been asking me to merge a handful of your generator fixes without tests?

jonpryor · 2017-08-22T19:33:36Z

Why have you been keeping this pending while you have been asking me to merge a handful of your generator fixes without tests?

Good question. :-(

"The perfect is the enemy of the good"...

Changes: dotnet/android-tools@f4c44e2...fc3c2ac * dotnet/android-tools@fc3c2ac: Bump to Microsoft.VisualStudioEng.MicroBuild.Core 1.0.0 (dotnet#159)

Changes: dotnet/android-tools@f4c44e2...fc3c2ac * dotnet/android-tools@fc3c2ac: Bump to Microsoft.VisualStudioEng.MicroBuild.Core 1.0.0 (#159)

[generator] do not emit static properties as instance.

1e4330e

Without this, abstract classes that implement java.time.chrono.Chronology fail to bind because they will try to implement those static properties as instance (this check had been done for methods, but not for properties).

dnfclas added the cla-already-signed label Jun 16, 2017

atsushieno mentioned this pull request Jun 16, 2017

[Mono.Android] bring back some java.time.chrono API - dotnet/android#652

Closed

jonpryor mentioned this pull request Jun 27, 2017

API O enumification dotnet/android#662

Merged

jonpryor merged commit bac28ab into dotnet:master Aug 22, 2017

jonpryor mentioned this pull request Mar 16, 2022

Bump to xamarin/xamarin-android-tools/main@fc3c2ac1 #964

Merged

jonpryor added a commit that referenced this pull request Mar 16, 2022

Bump to xamarin/xamarin-android-tools/main@fc3c2ac1 (#964)

05bfece

Changes: dotnet/android-tools@f4c44e2...fc3c2ac * dotnet/android-tools@fc3c2ac: Bump to Microsoft.VisualStudioEng.MicroBuild.Core 1.0.0 (#159)

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

[generator] do not emit static properties as instance. #159

[generator] do not emit static properties as instance. #159

atsushieno commented Jun 16, 2017

jonpryor commented Jun 16, 2017

atsushieno commented Jun 16, 2017

jonpryor commented Jun 16, 2017

atsushieno commented Jun 16, 2017

jonpryor commented Jun 16, 2017

atsushieno commented Aug 22, 2017 •

edited

Loading

jonpryor commented Aug 22, 2017

[generator] do not emit static properties as instance. #159

[generator] do not emit static properties as instance. #159

Conversation

atsushieno commented Jun 16, 2017

jonpryor commented Jun 16, 2017

atsushieno commented Jun 16, 2017

jonpryor commented Jun 16, 2017

atsushieno commented Jun 16, 2017

jonpryor commented Jun 16, 2017

atsushieno commented Aug 22, 2017 • edited Loading

jonpryor commented Aug 22, 2017

atsushieno commented Aug 22, 2017 •

edited

Loading