Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Feb 11, 2021

Fixes #797.

In #780 we made a change to only apply the ConstSugar logic to Mono.Android.dll. However there are of course other libraries written by Google that use the same pattern, namely AndroidX. I still think removing ConstSugar is the correct move for libraries other than Mono.Android because it causes more problems than it solves. (Also ConstSugar is irrelevant in the DIM world we want to move the ecosystem to.)

However, there is an issue when other libraries try to create interfaces that inherit Mono.Android's Android.Provider.BaseColumns.

Mono.Android contains:

[Register ("android.provider.BaseColumns")] public abstract class BaseColumns
[Register ("android.provider.BaseColumns")] public interface IBaseColumns

When we build our Java -> C# typemap, we only support a single map per key, and it just happens that abstract class BaseColumns wins. Thus when we look for the Symbol that matches implements android.provider.BaseColumns we end up with a class instead of a type. We then cast that as an InterfaceGen which is null and hit an NRE.

There are probably some deep, risky changes that should happen that would fix this, like supporting multiple entries in our typemap, or not emitting [Register] on our synthetic classes we create to allow access to Java interface constants.

But for today the easiest fix is to bail if we hit this case. The bindings will still be fine since these interfaces are not meant to be implemented, just used to provide access to constants.

@jpobst jpobst marked this pull request as ready for review February 11, 2021 21:00
@jonpryor
Copy link
Contributor

…like supporting multiple entries in our typemap,

Supporting multiple entries in the typemap sounds good, but isn't; for managed-to-java maps, there's no problem. Java-to-managed is the problem, and eventually you hit an "untyped" code path:

var list = new Java.Util.ArrayList(); // holds Java.Lang.Objects
using (var e = new SomeType()) {
    list.Add (e);
}
var o = list.Get (0);

What's the runtime type of o? This is where the Java-to-managed map comes in, and if SomeType happens to have a [Register] value shared by other types -- such as Android.Runtime.JavaList() & Java.Util.ArrayList (same [Register] value) -- then how do you choose which type to use?

Multiple Java-to-managed mappings doesn't help matters. A choice must eventually be made.

@jonpryor
Copy link
Contributor

… or not emitting [Register] on our synthetic classes we create to allow access to Java interface constants.

I rather like this idea, and cannot remember why we wanted/needed [Register] on the BaseColumns type in the first place. Particularly when that type has an internal constructor, and thus cannot be subclassed.

We should explore this option.

We should also accept this PR (#798).

@jpobst
Copy link
Contributor Author

jpobst commented Feb 12, 2021

Multiple Java-to-managed mappings doesn't help matters. A choice must eventually be made.

I was referring to generator's SymbolTable, not the typemap we bake into assemblies. You are correct that we eventually have to make a choice, but for this case we could choose one that is an interface rather than a class.

@jonpryor
Copy link
Contributor

I was referring to generator's SymbolTable

My mistake, I didn't realize that was the context.

@jonpryor jonpryor merged commit a4a2c13 into main Feb 12, 2021
@jonpryor jonpryor deleted the constsugar-nre branch February 12, 2021 18:17
@jpobst jpobst added this to the 11.3 (16.10 / 8.10) milestone Mar 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NRE in generator

2 participants