Fixing generator code to validate name collision between Methods and Fields

[gugavaro]

We need to check that methods that starts with Get can be "promoted" to a property only if there is no field already with that name, otherwise that collision will cause the field to be drop.
For example:

int Item;
GetITem();

that would cause GetItem to be converted to Item property and the field Item to not be included on the Api.

• Tests were added to cover the scenario.

[gugavaro]

Issue tracking this bug:
dotnet/android#3632

[jpobst]

Do we need to check this when looking at setters too? ie:

void SetItem (int item);
int item;

---

I agree that this is the correct fix, but just a heads up that we might need to do additional metadata work when we apply it to dotnet/android#3632.

If I'm reading this right, this will add GetIcon () for API < 23, but then that method will disappear for 23+. I suspect we will need to add it for 23+ as deprecated so it still works for people spanning versions.

[jonpryor]

Do we need to check this when looking at setters too? ie:

A setItem() Java method without a getItem() method is bound as a SetItem() C# method, as set-only properties are to be avoided:

X DO NOT provide set-only properties or properties with the setter having broader accessibility than the getter.

That said, adding a setItem() method to the example may be a good idea, to further ensure that method-backed properties don't hide fields.

[jonpryor]

If I'm reading this right, this will add GetIcon() for API < 23, but then that method will disappear for 23+

This should be checked & verified, but the desired outcome by default is that we add GetIcon() for API >= API-23, and Icon would be the field for all API levels:

partial class Action {
    public int Icon {get; set;}
    public virtual Icon GetIcon ();
}

Which, yes, would be an API break.

However, we already have that API break, between v5.1 and v6.0, and we're ignoring it!

https://github.com/xamarin/xamarin-android-api-compatibility/blob/a61271e046db989b4c9ec9f4d00c4358e07a2930/inter-api-extra-v5.1-v6.0.txt#L5

Meaning today, if you have code using v5.1 which uses that member, and run it in an app targeting API-29, you're boned. :-(

(...yet another reason why we need better API compatibility checking; see also dotnet/android#1118).

[jonpryor]

This looks good.

One last concern: versioning!

Consider that you have v1 of the API:

// Java v1
public class Example {
    public int getValue();
    public void setValue(int value);
}

We'd binding this as a Value property:

// C# v1
public partial class Example {
    public int Value {get; set;}
}

Now, in Java v2, a value field is added:

// Java v2
public class Example {
    public string value;
    public int getValue();
    public void setValue(int value);
}

What should C# do?

---

For starters, is this scenario supportable? Yes, but really only for Mono.Android.dll: https://github.com/xamarin/xamarin-android/blob/e887eff9811b630fcbe35479d426013ae2acb024/src/Mono.Android/Mono.Android.targets#L64-L73

api-merge.exe sets a //@merge.SourceFile attribute, which is the filename that the member came from. See e.g. https://github.com/xamarin/java.interop/blob/73096d9f60b34a23e9e608304de52ec22732da96/tools/generator/Tests-Core/api.xml#L35

generator uses SourceApiLevel in order to determine the order of added members.

---

Back to what generator should do: as written, what this PR will do is replace the Value property with the Example.value field:

// C# v2 -- bad?
public partial class Example {
    public String Value {get; set;}
    public int GetValue();
    public void SetValue(int value);
}

This results in an API & ABI break with previous versions.

Thus, two questions:

1. What should C# do in this versioning scenario?
2. Should this scenario be fixed in this PR, or in a separate PR?

[jpobst]

My opinion is fixing the fallout from that scenario is on the user.

We will need to review any changes this causes to Mono.Android.dll and fix them via metadata.

[gugavaro]

@jonpryor, @jpobst

The problem is little bigger than I/we thought

look this example:
https://www.android-doc.com/reference/org/apache/http/HttpEntity.html

Interface IHttpEntity has an abstract method called GetContent
We generate the interface with:
System.IO.Stream Content {}

then on AbstractHttpEntity class,
public abstract System.IO.Stream Content {}

but then on StringEntity class
https://www.android-doc.com/reference/org/apache/http/entity/StringEntity.html
it has a final protected field called content
protected final byte[] content

and a method called GetContent
InputStream getContent()

We were ignoring the protected content field and generating an override property Content.
public override unsafe System.IO.Stream Content {

Derrived classes could never access the original "protected field"

Now, with the generator changes it sees there is a field and generates the method with the original name trying to override it
public override unsafe System.IO.Stream GetContent ()

However the base class does not have that method available
Also the abstract property (Content) is not implemented on the derived class.

Here is the build error:
obj/Debug/android-29/mcw/Org.Apache.Http.Entity.StringEntity.cs(214,43): error CS0115: 'StringEntity.GetContent()': no suitable method found to override [/Users/gugavaro/Work/microsoft/Xamarin/xamarin-android/src/Mono.Android/Mono.Android.csproj]

obj/Debug/android-29/mcw/Org.Apache.Http.Entity.StringEntity.cs(11,23): error CS0534: 'StringEntity' does not implement inherited abstract member 'AbstractHttpEntity.Content.get' [/Users/gugavaro/Work/microsoft/Xamarin/xamarin-android/src/Mono.Android/Mono.Android.csproj]

I am not sure we should fix this issue.

thoughts?