Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generate static interface fields when DIM is enabled #821

Closed
jpobst opened this issue Apr 5, 2021 · 0 comments · Fixed by #831
Closed

Generate static interface fields when DIM is enabled #821

jpobst opened this issue Apr 5, 2021 · 0 comments · Fixed by #831
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jpobst
Copy link
Contributor

jpobst commented Apr 5, 2021

Context: dotnet/android#5818
Context: #459
Context: #509

When we began generating static methods and properties on interfaces when running with DIM enabled, it looks like we did not include static fields that cannot be treated as constants.

Thus, given the following example from dotnet/android#5818:

public interface EGL10 {
  public static final int EGL_ALPHA_FORMAT = 12424;
  public static final EGLSurface EGL_NO_SURFACE;
}

we only generate:

public partial interface IEGL10 {
  public const int EglAlphaFormat = (int) 12424;
}

This is an issue because we [Obsolete]'d the version in the *class* EGL10 pointing to the non-existent interface version:

[Obsolete ("Use 'Javax.Microedition.Khronos.Egl.IEGL10.EglNoSurface'. This class will be removed in a future release.")]
public static Javax.Microedition.Khronos.Egl.EGLSurface? EglNoSurface {
  ...
}
@jpobst jpobst mentioned this issue Apr 5, 2021
Closed
jonpryor pushed a commit to dotnet/android that referenced this issue May 3, 2021
@jpobst
[Mono.Android] Make protected constants interfaces public (#5897)
0e2321d 
Fixes: #5868

Context: dotnet/java-interop#821
Context: dotnet/java-interop@105d544

Originally, C# `interface`s couldn't contain non-"method" members,
nor did C# support interface default members.

Java did not have these restrictions:

	// Java; https://developer.android.com/reference/android/provider/CalendarContract.EventsColumns
	package android.provider;
	/* partial */ class CalendarContract {
	    protected static interface EventsColumns {
	        public static final int ACCESS_LEVEL = "accesslevel";
	    }
	}

Before C# 8, we bound these Java types by emitting a "proxy" class
which contained the constants (along with enumification):

	// C# binding
	namespace Android.Provider {
	    public partial class CalendarContract {
	        public abstract class EventsColumns {
	            public const string AccessLevel = (string) "accessLevel";
	        }
	    }
	}

With the introduction of C# 8, which allowed `interfaces` to contain
non-"method" members, along with interface default methods, we
did two things:

 1. We started emitting the constants into the interface declaration,
 2. We `[Obsolete]`d the "proxy" class, referring developers to use
    the interface type instead.

This resulted in:

	// C# Binding
	namespace Android.Provider {
	    public sealed partial class CalendarContract {
	        [Obsolete ("Use the 'Android.Provider.CalendarContract.IEventsColumns' type. This class will be removed in a future release.")]
	        public abstract partial class EventsColumns : Java.Lang.Object {
	            [Obsolete ("Use 'Android.Provider.CalendarContract.IEventsColumns.AccessLevel'. This class will be removed in a future release.")]
	            public const string AccessLevel = (string) "accessLevel";
	        }
	        protected internal partial interface IEventsColumns {
	            public const string AccessLevel = (string) "accessLevel";
	        }
	    }
	}

There was a minor problem with this, however: the semantics for
nested types with `protected` visibility differs between Java & C#:
Java allows
`android.provider.CalendarContract.EventsColumns.ACCESS_LEVEL` to be
accessed from anywhere.  C# *only* allows access to
`Android.Provider.CalendarContract.IEventDaysColumns.AccessLevel`
from subclasses of `CalendarContract`, and as `CalendarContract` is
`sealed`, it thus cannot be used from *anywhere*.

Fix this oversight by updating `src/Mono.Android/metadata` to change
the visibility of `android.provider.*Contract.*Columns` nested types
to be *public*.  This allows the constants to be used from everywhere:


	namespace Android.Provider {
	    public sealed partial class CalendarContract {
	        public partial interface IEventsColumns {
	            public const string AccessLevel = (string) "accessLevel";
	        }
	    }
	}

The following interface types are now `public`:

  * `Android.Provider.CalendarContract/IAttendeesColumns`
  * `Android.Provider.CalendarContract/ICalendarAlertsColumns`
  * `Android.Provider.CalendarContract/ICalendarCacheColumns`
  * `Android.Provider.CalendarContract/ICalendarColumns`
  * `Android.Provider.CalendarContract/ICalendarSyncColumns`
  * `Android.Provider.CalendarContract/IColorsColumns`
  * `Android.Provider.CalendarContract/IEventDaysColumns`
  * `Android.Provider.CalendarContract/IEventsColumns`
  * `Android.Provider.CalendarContract/IExtendedPropertiesColumns`
  * `Android.Provider.CalendarContract/IRemindersColumns`
  * `Android.Provider.CalendarContract/ISyncColumns`
  * `Android.Provider.ContactsContract/CommonDataKinds/ICommonColumns`
  * `Android.Provider.ContactsContract/IBaseSyncColumns`
  * `Android.Provider.ContactsContract/IContactNameColumns`
  * `Android.Provider.ContactsContract/IContactOptionsColumns`
  * `Android.Provider.ContactsContract/IContactStatusColumns`
  * `Android.Provider.ContactsContract/IDataColumns`
  * `Android.Provider.ContactsContract/IDataUsageStatColumns`
  * `Android.Provider.ContactsContract/IDeletedContactsColumns`
  * `Android.Provider.ContactsContract/IGroupsColumns`
  * `Android.Provider.ContactsContract/IPhoneLookupColumns`
  * `Android.Provider.ContactsContract/IPresenceColumns`
  * `Android.Provider.ContactsContract/IRawContactsColumns`
  * `Android.Provider.ContactsContract/ISettingsColumns`
  * `Android.Provider.ContactsContract/IStatusColumns`
  * `Android.Provider.ContactsContract/IStreamItemPhotosColumns`
  * `Android.Provider.ContactsContract/IStreamItemsColumns`
  * `Android.Provider.ContactsContract/ISyncColumns`
@jpobst jpobst mentioned this issue May 4, 2021
Merged
@jpobst jpobst added bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.) labels May 5, 2021
jonpryor pushed a commit that referenced this issue May 5, 2021
@jpobst
[generator] Support binding non-const Java interface fields (#831)
12e670a 
Fixes: #821

Context: 29f9707
Context: #509
Context: dotnet/android#5818

When we started generating static methods and properties on interfaces
when running with C#8 default interface members enabled, it looks like
we did not include static fields that cannot be treated as constants.

Thus, consider the [EGL10][0] interface mentioned in
dotnet/android#5818:

	// Java
	/* partial */ interface EGL10 {
	    public static final int EGL_ALPHA_FORMAT = 12424;
	    public static final EGLSurface EGL_NO_SURFACE = …
	}

The [`EGL10.EGL_NO_SURFACE`][1] field is not a compile-time constant.
In C# parlance, it's `readonly`, *not* `const`.

In commit 29f9707, we *ignored* `interface` fields which:

 1. Were read+write, or
 2. Did not contain compile-time constant values.

Thus, we generated:

	// C# binding
	partial interface IGL10 {
	    public const int EglAlphaFormat = (int) 12424;
	}

This is an issue because we `[Obsolete]`'d the members in the `EGL10`
"proxy class", *and* referred to a non-existent `interface` member:

	// C# binding; note class, not interface
	[Obsolete ("Use the 'Javax.Microedition.Khronos.Egl.IEGL10' type. This class will be removed in a future release.")]
	public abstract partial class EGL10 {
	    [Obsolete ("Use 'Javax.Microedition.Khronos.Egl.IEGL10.EglAlphaFormat'. …")]
	    public const int EglAlphaFormat = 12424;

	    [Obsolete ("Use 'Javax.Microedition.Khronos.Egl.IEGL10.EglNoSurface'. …")]
	    public static Javax.Microedition.Khronos.Egl.EGLSurface? EglNoSurface => …
	}

`EGL10.EglNoSurface` directs us to `IEGL10.EglNoSurface`, while
`IEGL10.EglNoSurface` doesn't exist!

This is a bad user experience.

Fix generator so that when
`generator -lang-features=default-interface-methods` is used,
non-`const` interface members are also emitted, allowing:

	// C# binding; fixed
	partial interface IEGL10 {
	    public static EGLSurface? EglNoSurface => …
	}

Should a Java interface contain a mutable field, those can now be
bound as well:

	// Java
	interface Example {
	    public static int MUTABLE;
	}

	// C# binding
	partial interface IExample {
	    public static int Mutable {
	        get => …
	        set => …
	    }
	}

[0]: https://developer.android.com/reference/javax/microedition/khronos/egl/EGL10
[1]: https://developer.android.com/reference/javax/microedition/khronos/egl/EGL10#EGL_NO_SURFACE
@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.
Assignees
No one assigned
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Milestone
No milestone
1 participant
@jpobst