dotnet / java-interop

Java.Interop provides open-source bindings of Java's Java Native Interface (JNI) for use with .NET managed languages such as C#
Other
200 stars 52 forks source link

'DownloadDrawablesAsync' does not implement inherited abstract member 'AsyncTask.DoInBackground(params Object[])' #647

Closed fernandovm closed 4 years ago

fernandovm commented 4 years ago

When I try compile a binding to google mediation mopub adapter I get the error 'DownloadDrawablesAsync' does not implement inherited abstract member 'AsyncTask.DoInBackground(params Object[])'

Steps to Reproduce

  1. Try compile the attached sample project Google.Ads.Mediation.Mopub.zip

Expected Behavior

A successful build.

Actual Behavior

1>C:\Users\fernando\source\repos\Google.Ads.Mediation.Mopub\Google.Ads.Mediation.Mopub\obj\Debug\generated\src\Com.Mopub.Mobileads.Dfp.Adapters.DownloadDrawablesAsync.cs(10,23,10,45): error CS0534: 'DownloadDrawablesAsync' does not implement inherited abstract member 'AsyncTask.DoInBackground(params Object[])'

Version Information

Microsoft Visual Studio Enterprise 2019 Version 16.5.5 VisualStudio.16.Release/16.5.5+30104.148 Microsoft .NET Framework Version 4.8.03752

Installed Version: Enterprise

Xamarin 16.5.000.533 (d16-5@9152e1b) Visual Studio extension to enable development for Xamarin.iOS and Xamarin.Android.

Xamarin Designer 16.5.0.470 (remotes/origin/d16-5@681de3fd6) Visual Studio extension to enable Xamarin Designer tools in Visual Studio.

Xamarin Templates 16.5.49 (0904f41) Templates for building iOS, Android, and Windows apps with Xamarin and Xamarin.Forms.

Xamarin.Android SDK 10.2.0.100 (d16-5/988c811) Xamarin.Android Reference Assemblies and MSBuild support. Mono: c0c5c78 Java.Interop: xamarin/java.interop/d16-5@fc18c54 ProGuard: xamarin/proguard/master@905836d SQLite: xamarin/sqlite/3.28.0@46204c4 Xamarin.Android Tools: xamarin/xamarin-android-tools/d16-5@9f4ed4b

Xamarin.iOS and Xamarin.Mac SDK 13.16.0.13 (b75deaf) Xamarin.iOS and Xamarin.Mac Reference Assemblies and MSBuild support.

moljac commented 4 years ago

@fernandovm If bindings project would be that easy... We are trying to make bindings projects easy, but it will work only in ideal case.

In your case you are inheriting a type and binding generator generates code w/o compiling (or even better semantically analysing the code), so the only way is to tweak class inheritance (metadata) with metadata transforms in metadata.xml:

  1. remove base class - NO-NO
  2. check if the method is generated and why c# has issues with it

So that method was generated:

        // Metadata.xml XPath method reference: path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
        [Register ("doInBackground", "([Ljava/lang/Object;)Ljava/util/HashMap;", "GetDoInBackground_arrayLjava_lang_Object_Handler")]
        protected virtual unsafe global::System.Collections.Generic.IDictionary<string, global::Android.Graphics.Drawables.Drawable> DoInBackground (params global::Java.Lang.Object[] @params)
        {
            const string __id = "doInBackground.([Ljava/lang/Object;)Ljava/util/HashMap;";
            IntPtr native__params = JNIEnv.NewArray (@params);
            try {
                JniArgumentValue* __args = stackalloc JniArgumentValue [1];
                __args [0] = new JniArgumentValue (native__params);
                var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
                return global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef);
            } finally {
                if (@params != null) {
                    JNIEnv.CopyArray (native__params, @params);
                    JNIEnv.DeleteLocalRef (native__params);
                }
            }
        }

So why does it not build?

Because Mopub version returns System.Collections.Generic.IDictionary<string, global::Android.Graphics.Drawables.Drawable>, while AsyncTask returns Java.Lang.Object

see: https://docs.microsoft.com/en-us/dotnet/api/android.os.asynctask.doinbackground?view=xamarin-android-sdk-9#Android_OS_AsyncTask_DoInBackground_Java_Lang_Object___

So if add this to your metadata.xml:

    <attr
        path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
        name="managedReturn"
        >
        Java.Lang.Object
    </attr>

It should have the same signature as method in the base class, but the error persists.

@jpobst @jonpryor

This will make your code build (at least it did in my code):

    <remove-node
        path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground']"
        />
    <add-node
        path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']"
        >
        <!--
        java.util.HashMap&lt;java.lang.String, android.graphics.drawable.Drawable&gt;
        -->
      <method 
            visibility="protected" static="false" abstract="false" return="java.lang.Object" name="doInBackground" 
            deprecated="not deprecated" final="false" bridge="false" native="false"
            synchronized="false" synthetic="false" 
            >
        <parameter type="java.lang.Object..."  name="params" >
        </parameter>
      </method>

    </add-node>

I will create repro sample in the repo and post it here.

moljac commented 4 years ago

I managed to bind v.5.4.1 without any metadata.

There is something in v.5.12.0

OK mvnrepository does not have the latest version, but bintray:

https://bintray.com/mopub/mopub-android-sdk/mopub-android-sdk/_latestVersion

5.12.0 builds w/o issues and metadata too

https://github.com/HolisticWare/HolisticWareComponents/tree/master/XPlat/Mopub

Seems that new and old style projects (and tooling) do make the difference:

https://github.com/HolisticWare/HolisticWareComponents/blob/master/XPlat/Mopub/source/Mopub.XamarinAndroid/Mopub.XamarinAndroid.csproj

jpobst commented 4 years ago

I'm not sure why managedReturn isn't good enough. Switching it to return will build:

<attr
    path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
    name="return">
    Java.Lang.Object
</attr>

That's generally frowned upon as it messes with the type that gets sent to Java, so you will need to test it before you ship it. (This is effectively what @moljac's <remove-node>/<add-node> is doing.)

jpobst commented 4 years ago

For internal investigation:

There's 2 issues with what gets generated for managedReturn:

[Register ("doInBackground", "([Ljava/lang/Object;)Ljava/util/HashMap;", "GetDoInBackground_arrayLjava_lang_Object_Handler")]
protected virtual unsafe global::Java.Lang.Object DoInBackground (params global::Java.Lang.Object[] @params)
{
    const string __id = "doInBackground.([Ljava/lang/Object;)Ljava/util/HashMap;";
    IntPtr native__params = JNIEnv.NewArray (@params);
    try {
        JniArgumentValue* __args = stackalloc JniArgumentValue [1];
        __args [0] = new JniArgumentValue (native__params);
        var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
        return global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef);
    } finally {
        if (@params != null) {
            JNIEnv.CopyArray (native__params, @params);
            JNIEnv.DeleteLocalRef (native__params);
        }
    }
}

1) It is virtual rather than override. (https://github.com/xamarin/java.interop/issues/586) 2) The binding code is returning JavaDictionary<string, Drawable> instead of JLO.

jonpryor commented 4 years ago

@jpobst asked:

I'm not sure why managedReturn isn't good enough.

It's not good enough because managedReturn only changes the declared return type, and nothing in the body. The problem is that JavaDictionary<T>.FromJniHandle() returns an IDictionary<TKey, TValue>, which is not implicitly convertible to Java.Lang.Object, hence the compilation error.

jonpryor commented 4 years ago

The problem with the "just change the return type" approach, suggested by @jpobst and @moljac, is that it will break subclassing and overriding, because the return type is used to tell the Java Callable Wrapper generator what Java return type to use. By altering it to be java.lang.Object, the JCW return type will likewise be java.lang.Object, which is not the original java.util.HashMap or a subclass thereof.

The result will be Java Callable Wrapper generator errors from javac.

moljac commented 4 years ago

@jonpryor @jpobst

If such metadata fails (like my attempt to change managedReturn) would it be possible to emit warning or error in such case?

jonpryor commented 4 years ago

What we need is a (not fully specified or thought-out) generator "solution" which allows the Java & managed return types to "diverge" in more significant ways than what managedReturn currently allows.

For example, if there was an implicit conversion from IDictionary & other collection types to Java.Lang.Object, managedReturn would likely work as-is:

partial class /* Java.Lang */ Object {
    public static implicit operator Java.Lang.Object (IDictionary value) => new JavaDictionary (value);
    // ...
}

While this "works," is this something we want to support? (Though Java.Lang.Object already has a ton of implicit conversion operators, so perhaps this is "fine"…)

Alternatively, could we allow generator to detect that the types aren't implicitly convertible, e.g. that IDictionary isn't implicitly convertible to Java.Lang.Object, and in such instances insert an appropriate .JavaCast<T>?

return JavaDictionary<string, Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef)
    .JavaCast<Java.Lang.Object> ();

Though as-is, this would also require some additional "glue code" to add a new .JavaCast<T>() extension method that operates on IDictionary/etc.

jpobst commented 4 years ago

@moljac The thing is your metadata doesn't "fail". If it couldn't match anything it would have given you an error like: <attr path=\"{0}\"/> matched no nodes..

All metadata does is modify the api.xml using XPath, it doesn't understand what that actually is, and it is successfully adding the managedReturn attribute.

It just happens to be a successful change that doesn't fix the issue. 😄

fernandovm commented 4 years ago

Hi @moljac, @jpobst and @jonpryor.. Then, I didn't go as far as you guys, but I have made some tries using the guide on https://gist.github.com/JonDouglas/dda6d8ace7d071b0e8cb before post the issue. My difficulties and points of attention were:

1) Its not sufficient build the binding, it need works on runtime; Mainly because I think that DownloadDrawablesAsync class will be need on runtime.

2) I tried change the base class of DownloadDrawablesAsync, I thought that using AsyncTask<> maybe it's the most correct, based in https://github.com/googleads/googleads-mobile-android-mediation/blob/master/ThirdPartyAdapters/mopub/mopub/src/main/java/com/mopub/mobileads/dfp/adapters/DownloadDrawablesAsync.java. But I was unable to adjust the generic parameters correctly. :((

3) I also did not find ways to force the override for the DoInBackground method, it was always generated with virtual.

jpobst commented 4 years ago

One thing to try would be use @moljac's <remove-node> above to not automatically bind this method.

Then manually bind it in an Addition:

public partial class DownloadDrawablesAsync : global::Android.OS.AsyncTask
{
    static Delegate cb_doInBackground_arrayLjava_lang_Object_;
    #pragma warning disable 0169
    static Delegate GetDoInBackground_arrayLjava_lang_Object_Handler ()
    {
        if (cb_doInBackground_arrayLjava_lang_Object_ == null)
            cb_doInBackground_arrayLjava_lang_Object_ = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, IntPtr, IntPtr>) n_DoInBackground_arrayLjava_lang_Object_);
        return cb_doInBackground_arrayLjava_lang_Object_;
    }

    static IntPtr n_DoInBackground_arrayLjava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native__params)
    {
        global::Com.Mopub.Mobileads.Dfp.Adapters.DownloadDrawablesAsync __this = global::Java.Lang.Object.GetObject<global::Com.Mopub.Mobileads.Dfp.Adapters.DownloadDrawablesAsync> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
        global::Java.Lang.Object[] @params = (global::Java.Lang.Object[]) JNIEnv.GetArray (native__params, JniHandleOwnership.DoNotTransfer, typeof (global::Java.Lang.Object));
        IntPtr __ret = global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.ToLocalJniHandle ((IDictionary<string, global::Android.Graphics.Drawables.Drawable>)__this.DoInBackground (@params));
        if (@params != null)
            JNIEnv.CopyArray (@params, native__params);
        return __ret;
    }
    #pragma warning restore 0169

    // Metadata.xml XPath method reference: path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
    [Register ("doInBackground", "([Ljava/lang/Object;)Ljava/util/HashMap;", "GetDoInBackground_arrayLjava_lang_Object_Handler")]
    protected override unsafe Java.Lang.Object DoInBackground (params global::Java.Lang.Object[] @params)
    {
        const string __id = "doInBackground.([Ljava/lang/Object;)Ljava/util/HashMap;";
        IntPtr native__params = JNIEnv.NewArray (@params);
        try {
            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
            __args [0] = new JniArgumentValue (native__params);
            var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
            return (Java.Lang.Object)global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef);
        } finally {
            if (@params != null) {
                JNIEnv.CopyArray (native__params, @params);
                JNIEnv.DeleteLocalRef (native__params);
            }
        }
    }
}

This is the generated code, with the following changes:

Note this is untested, you should ensure it actually works.

fernandovm commented 4 years ago

Hi @jpobst, I already have applied the @moljac's suggestion and I'm trying to test, but now I have another issue. The MoPub's SDK is needed, together with google mediation mopub adapter, to try the AdMob mediation. However, when I build a binding project to MoPub's SDK (https://bintray.com/mopub/mopub-android-sdk/mopub-android-sdk/5.12.0) it does not import/generate any class.. :(

jpobst commented 4 years ago

If you run Rebuild, what messages are you seeing in the log? There's generally pretty good warnings telling you exactly why it is skipping over each type/method it doesn't bind.

fernandovm commented 4 years ago

Hi @jpobst, yes, generally, but unfortunately seems is not be this case. In normal output msbuild verbosity is generated just one: warning CS0109: The member 'BuildConfig.class_ref' does not hide an accessible member. The new keyword is not required.

The diagnostico verbosity output log: build-output.txt

jpobst commented 4 years ago

Are the types listed in:

These are the output as the process moves through in order (parsing the jar, resolving the java types, applying metadata), so if we can find where the types disappear it should tell us what step is failing.

If they aren't in any of the files, you can use a Java decompiler to see if they exist in the .jar: obj\Debug\library_project_jars\classes.jar.

fernandovm commented 4 years ago

Hi guys, excuse me by delay in reply. So @jpobst, indeed, I don't know why (maybe it is just some kind of gradle wrapper), but the mopub android sdk .aar file downloaded from bintray does not has the need classes inside the classes.jar file.

Well, I built the mopub android sdk from github sources and create a binding library from the .jar generated file. Several adjustments were needed in metadata file to build with succesfull, but okay, it worked.

However, when I add a reference to this binding library in my android app project I have new compilation errors, such as:

obj\Off\90\android\src\mono\com\mopub\mobileads\VideoDownloader_VideoDownloaderListenerImplementor.java:8: error: VideoDownloaderListener is not public in VideoDownloader; cannot be accessed from outside package com.mopub.mobileads.VideoDownloader.VideoDownloaderListener

So, the main problem is that in generated file from obj\Debug\generated\src the interface already appears as public:

// Metadata.xml XPath class reference: path="/api/package[@name='com.mopub.mobileads']/class[@name='VideoDownloader']"
[global::Android.Runtime.Register ("com/mopub/mobileads/VideoDownloader", DoNotGenerateAcw=true)]
public partial class VideoDownloader : global::Java.Lang.Object {

    // Metadata.xml XPath interface reference: path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']"
    [Register ("com/mopub/mobileads/VideoDownloader$VideoDownloaderListener", "", "Com.Mopub.Mobileads.VideoDownloader/IVideoDownloaderListenerInvoker")]
    public partial interface IVideoDownloaderListener : IJavaObject, IJavaPeerable {

        // Metadata.xml XPath method reference: path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']/method[@name='onComplete' and count(parameter)=1 and parameter[1][@type='boolean']]"
        [Register ("onComplete", "(Z)V", "GetOnComplete_ZHandler:Com.Mopub.Mobileads.VideoDownloader/IVideoDownloaderListenerInvoker, Higgx.Library.AdSource.Mopub.Sdk.Android")]
        void OnComplete (bool p0);

    }

   //removed code..

}

Would appreciate if you point to me what's the problem that I can't see. :((

Thank you!

jpobst commented 4 years ago

That error is coming from Java, not C#. So you'll need to look at the .jar that it is trying to compile against and see if the interface is public there.

Since you do this to change the C# version I suspect it is not public in Java:

<attr path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']" name="visibility">public</attr>

I'm not sure how you're intended to implement that interface, even if you were consuming this directly from Java, if it's not public.

fernandovm commented 4 years ago

Hi @jpobst, this change in C# was just a test (one of them), but yes, it is not public in Java (see here), it is visible only inside its package.

The problem seems me is that in binding library it is generated as public, but, even I changing that using metatada (to force the internal visibility) I get erros when compiling my app that reference this binding library.

jpobst commented 4 years ago

I'm afraid I don't understand what you are saying, it is explicitly being marked public in C# by this metadata:

<attr path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']" name="visibility">public</attr>

What error do you get when you remove that?

fernandovm commented 4 years ago

Ok, sorry, let's go.. The interface VideoDownloaderListener is not public in the java code, but I get the error described above, even without to do any visibility change in the metadata.

Then, I have made several tests changing the visibility of this interface using metadata, just attempts to try understand how the generator would can interpret the case, but, I was not successful.

You can see this problem in the updated sample project Google.Ads.Mediation.Mopub.zip.

jpobst commented 4 years ago

When I compile the sample project using VS2019 16.6.0 I get:

error: MraidBridge_MraidBridgeListenerImplementor is not abstract and does not override abstract method onSetOrientationProperties(boolean,MraidOrientation) in MraidBridgeListener
\mono\com\mopub\mraid\MraidBridge_MraidBridgeListenerImplementor.java

This is because onSetOrientationProperties in the interface MraidBridgeListener requires a parameter of type MraidOrientation, which is not a public type in Java.

However this does not seem to be the error you are getting. Is this an error you have gotten previously and have a workaround for?

fernandovm commented 4 years ago

When I compile it I also get this error, in really, I get seven errors:

image

I didn't mentioned before because I wanted solve the other six problems, they seems me the same thing.

About MraidBridgeListener, I haven't tested, but as it requires the type MraidOrientation which is not a public, I think that there isn't to do, unless remove it using metadata. This will compile, but after will see if works in runtime.

jpobst commented 4 years ago

I think those 6 will go away if you update to 16.6+, due to this change: https://github.com/xamarin/java.interop/issues/572.

Granted, the fix is that the listeners are going to go away, but they are package-private in Java, and shouldn't be bound.

That only leaves the error I mentioned.

fernandovm commented 4 years ago

I think those 6 will go away if you update to 16.6+, due to this change: #572.

Granted, the fix is that the listeners are going to go away, but they are package-private in Java, and shouldn't be bound.

Indeed, great!

That only leaves the error I mentioned.

I will remove the MraidBridge class using metadata and to test at runtime.

<remove-node path="/api/package[@name='com.mopub.mraid']/class[@name='MraidBridge']" />

jpobst commented 4 years ago

Hopefully this fixed your issue! If not, please comment with updated details and we can re-open.