Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Feb 28, 2020

Traditionally we have not been able to support types nested inside an interface, as C# did not allow it. Cases like this:

My.Namespace.IParent.IChild

had to be bound as:

My.Namespace.IParentChild

With the addition of DIM in C#8, interfaces can now created with nested types. However this is a breaking changes if you've already been generating the old style types.

Therefore we add a new flag --lang-features=nested-interface-types that defaults to false to preserve compatibility. Opting in to this flag will nest all interface types.

Mixing Nested and Unnested

Additionally, users may have bindings that need to mix nested and unnested interface types. For example, Mono.Android.dll contains a decade of APIs that cannot be broken, however new APIs can be nested correctly, producing a binding that more closely mirrors its Java counterpart.

To support this there is a new attribute available for interface and class nodes in api.xml called unnest. This attribute is not set by tooling such as class-parse, but can be modified via metadata to force an interface-nested type to override the default setting. If the attribute is omitted, the default is used.

Example:

<!-- Do not nest type (legacy style) -->
<attr path="/api/package[@name='my.namespace']/interface[@name='parent']/interface[@name='child']" name="unnest">true</attr>

<!-- Nest type (new style) -->
<attr path="/api/package[@name='my.namespace']/interface[@name='parent']/interface[@name='child']" name="unnest">false</attr>

@jpobst jpobst force-pushed the unnest-interface-types branch from 2f6a20f to 8bc0710 Compare February 28, 2020 16:22
@jpobst jpobst marked this pull request as ready for review February 28, 2020 16:50
@jpobst jpobst requested a review from jonpryor February 28, 2020 16:51
@jonpryor
Copy link
Contributor

jonpryor commented Feb 28, 2020

I like the idea, I'm not fond of the naming (negatives usually don't make for good names) or the implications. Naming wise, I'd prefer nested instead of unnest, negating the logic, or declare-in-parent-package, preserving the logic. (Insert bike-shedding here.)

The implications being...how will this be used?

In the context of Issue #509, we want to enable use of nested types by default for all types added in API-R/API-30, while disabling use of nested types for all types present in or before API-29.

How will the unnest attribute be set? Is the intention to add 921 entries to Mono.Android's metadata file for each <interface/> present in xamarin-android/bin/BuildDebug/api/api-29.xml.in? 😱

Or just for the newly added <interface/> types in API-R?

Is the intention to "somehow" determine the API Level a type was added in, and provide a default unnest value based on that?

What should be done for non-android.jar binding projects? What will @moljac find usable, if anything?

I'd like to understand the broader integration scenarios.

@jpobst
Copy link
Contributor Author

jpobst commented Mar 2, 2020

nested vs unnest

I'm not a huge fan of unnest, but I feel the most common (perhaps only?) use of this will be:

  • Change an old library to --lang-features=nested-interface-types
  • Use this attribute to "un-nest" previously existing types for compatibility.

That is, the "default" is nested, so the override should be unnest or declare-in-parent-package.

android.jar

Integration is pretty easy for the android.jar case, because we have @merge.SourceFile, so we can either:

Leave the global flag off and add these metadata lines for each API platform going forward:

<attr api-since="30" path="/api/package/interface[contains(@merge.SourceFile,'api-30.xml.in')]" name="unnest">false</attr>
<attr api-since="30" path="/api/package/class[contains(@merge.SourceFile,'api-30.xml.in')]" name="unnest">false</attr>

Or we can turn the global flag on and add these metadata lines for each API platform previous to 30:

<!-- Handle everything that has always existed -->
<attr api-since="30" path="/api/package/interface[not(@merge.SourceFile)]" name="unnest">true</attr>
<attr api-since="30" path="/api/package/class[not(@merge.SourceFile)]" name="unnest">true</attr>

<!-- Need these 2 for each platform prior to 30 -->
<attr api-since="30" path="/api/package/interface[contains(@merge.SourceFile,'api-29.xml.in')]" name="unnest">true</attr>
<attr api-since="30" path="/api/package/class[contains(@merge.SourceFile,'api-29.xml.in')]" name="unnest">true</attr>

The first way is shorter, but must be done every year with a new binding. The second way is more upfront, but will never need to be updated going forward.

Third parties

This does not provide complete tooling for 3rd parties to use, because we do not currently provide any support for tracking changes across .jar versions. What is does provide is: if a consumer has (or builds) that change-tracking tooling, they have a place where they can integrate it into generator to create mixed-nesting bindings.

@jonpryor jonpryor merged commit 28b1fc9 into master Mar 2, 2020
@jonpryor jonpryor deleted the unnest-interface-types branch March 2, 2020 21:31
jonpryor pushed a commit that referenced this pull request Mar 3, 2020
Context: #509

Consider the following Java code:

	package example;
	public interface Parent {
	    public interface Child {
	    }
	}

Before C#8, interfaces could not contain types.  To bind
`example.Parent.Child`, we had to "un-nest" the interface, *prefixing*
the interface name with the name of the enclosing type:

	namespace Example {
	    public interface IParent {
	    }
	    public interface IParentChild {
	    }
	}

With C#8, interfaces can now contain types.  We can now emit:

	namespace Example {
	    public interface IParent {
	        public interface IChild {
		}
	    }
	}

Enable this new style of binding when
`generator --lang-features=nested-interface-types` is used.  This is
*disabled* by default.

However, even when `nested-interface-types` is enabled, not all
interfaces should follow this new strategy.  In the case of
`Mono.Android.dll`, nested interface types should *only* be used for
types added in API-R/API-30.

Add support for a new `unnest` Metadata attribute which can be placed
on nested types to explicitly emit the C#7-compatible bindings:

	<attr path="/api/package[@name='example']/interface[@name='Parent.Child']" name="unnest">true</attr>

In a future integration with xamarin-android,
[`src/Mono.Android/metadata`][0] can be updated so that all APIs
prior to API-30 are unnested, and all APIs starting with API-30 are
nested:

	<!-- Handle everything that has always existed -->
	<attr api-since="30" path="/api/package/interface[not(@merge.SourceFile)]" name="unnest">true</attr>
	<attr api-since="30" path="/api/package/class[not(@merge.SourceFile)]" name="unnest">true</attr>

	<!-- Need these 2 for each platform prior to 30 -->
	<attr api-since="30" path="/api/package/interface[contains(@merge.SourceFile,'api-29.xml.in')]" 	name="unnest">true</attr>
	<attr api-since="30" path="/api/package/class[contains(@merge.SourceFile,'api-29.xml.in')]" name="unnest">true</attr>

This will ensure we preserve API compatibility.

[0]: https://github.com/xamarin/xamarin-android/blob/b366ac1e6d411dc052dc8711d881d71b7e967685/src/Mono.Android/metadata
@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.

4 participants