Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Jun 25, 2019

This adds support for interface constants to be generated using C# 8.0's new default interface member support. This is feature-flagged behind the --lang-features=interface-constants argument (off by default).

Here is an example of what is generated from Mono.Android.dll:

// Metadata.xml XPath interface reference: path="/api/package[@name='android.os']/interface[@name='Parcelable']"
[Register ("android/os/Parcelable", "", "Android.OS.IParcelableInvoker", ApiSince = 1)]
public partial interface IParcelable : IJavaObject {

+	// Metadata.xml XPath field reference: path="/api/package[@name='android.os']/interface[@name='Parcelable']/field[@name='CONTENTS_FILE_DESCRIPTOR']"
+	[Register ("CONTENTS_FILE_DESCRIPTOR")]
+	public const int ContentsFileDescriptor = (int) 1;

	// Metadata.xml XPath method reference: path="/api/package[@name='android.os']/interface[@name='Parcelable']/method[@name='describeContents' and count(parameter)=0]"
	[Register ("describeContents", "()I", "GetDescribeContentsHandler:Android.OS.IParcelableInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null")]
	int DescribeContents ();

	// Metadata.xml XPath method reference: path="/api/package[@name='android.os']/interface[@name='Parcelable']/method[@name='writeToParcel' and count(parameter)=2 and parameter[1][@type='android.os.Parcel'] and parameter[2][@type='int']]"
	[Register ("writeToParcel", "(Landroid/os/Parcel;I)V", "GetWriteToParcel_Landroid_os_Parcel_IHandler:Android.OS.IParcelableInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null")]
	void WriteToParcel (Android.OS.Parcel dest, [global::Android.Runtime.GeneratedEnum] Android.OS.ParcelableWriteFlags flags);
}

There are several Android interfaces defined that only contain constants. Without DIM, we have not been generating these interfaces because there was nothing we could generate for them. (These are referred to as IsConstSugar). Now we generate them, but they do not have [Register] on the interface and do not inherit from IJavaObject since they are not used for Java interop.

// Metadata.xml XPath interface reference: path="/api/package[@name='android.icu.lang']/interface[@name='UProperty.NameChoice']"
public partial interface IUPropertyNameChoice {

	// Metadata.xml XPath field reference: path="/api/package[@name='android.icu.lang']/interface[@name='UProperty.NameChoice']/field[@name='LONG']"
	[Register ("LONG")]
	public const int Long = (int) 1;

	// Metadata.xml XPath field reference: path="/api/package[@name='android.icu.lang']/interface[@name='UProperty.NameChoice']/field[@name='SHORT']"
	[Register ("SHORT")]
	public const int Short = (int) 0;
}

Here is the diff of this change run on Mono.Android.dll:
https://gist.github.com/jpobst/7aa0bb1a01975a56038cb7ab12ecdb1c

@jpobst jpobst marked this pull request as ready for review June 26, 2019 14:48
@jpobst jpobst requested a review from jonpryor June 26, 2019 19:06
@jonpryor
Copy link
Contributor

One important question: should WriteInterfaceImplementedMembersAlternative() ever be optional?

There are potentially (at least?) two overlapping use cases of interest:

  1. C#8+ only bindings?
  2. C#7 bindings

Assume we're at some point in the future, in which C#8 is common. In such a world, would it make sense to support bindings which omit the "alternative" members classes? Mono.Android.dll couldn't make use of this, but perhaps 3rd party bindings could (Support library?), if they don't have any backward compatibility API constraints.

It may make sense to turn generator --default-interface-methods into generator --csharp8-features=dim,iface-consts,nullability (plus others?) so that C#8 features can be independently selected, so that a "C#8 only" binding would be generator --csharp8-features=iface-consts to "skip" default-interface methods & nullable-reference types, but emit interface constants within the interface and prevent generation of the "alternate" FooConsts/etc. class.

However, does that really make sense? Or is this a silly proposition?

@jpobst
Copy link
Contributor Author

jpobst commented Jun 27, 2019

Let's leave turning off our pre-C#8 hacks as a discussion for another day. I think it's going to need to be configured independent from enabling these new features. (For example, some people may want just the hacks, some may want hacks with [Obsolete] and the new features, and some may want just the new features.)

I changed the "enable new features" flag to be more granular, though of course currently there is only one supported value: --lang-features=interface-constants.

@jonpryor jonpryor merged commit a3b7456 into master Jun 28, 2019
@jonpryor
Copy link
Contributor

A thought occurs to me, which would be slightly more invasive (which is why I merged the existing PR).

Consider the android.icu.lang.UProperty.NameChoice; it's doubly nested: UProperty is an interface, and UProperty.NameChoice is a nested type.

C# < 8 didn't support any types nested within interfaces, hence the IUPropertyNameChoice interface name in the PR description:

// Metadata.xml XPath interface reference: path="/api/package[@name='android.icu.lang']/interface[@name='UProperty.NameChoice']"
public partial interface IUPropertyNameChoice {

	// Metadata.xml XPath field reference: path="/api/package[@name='android.icu.lang']/interface[@name='UProperty.NameChoice']/field[@name='LONG']"
	[Register ("LONG")]
	public const int Long = (int) 1;

	// Metadata.xml XPath field reference: path="/api/package[@name='android.icu.lang']/interface[@name='UProperty.NameChoice']/field[@name='SHORT']"
	[Register ("SHORT")]
	public const int Short = (int) 0;
}

However, as part of C#8 default interface methods interfaces may also declare nested types. This means that we could in fact bind UProperty.NameChoice as IUProperty.INameChoice:

namespace Android.Icu.Lang {
    public partial interface IUProperty {
        public const int Age = 16384;
        // ...

        public partial interface INameChoice {
            public const int Long = (int) 1;
            // ...
        }
    }
}

Is this something we should consider?

@jpobst jpobst deleted the dim-consts branch June 28, 2019 18:20
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 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.

3 participants