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.

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

[Xamarin.Android.Build.Tasks] Set .NET6 defaults for bindings projects. #6066

Merged
merged 4 commits into from Jul 14, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jul 1, 2021

Fixes: #4657.

With .NET 6 we want to turn on the following binding project enhancements added with C# 8 by default:

  • Interface constants
  • Interface static methods
  • Interface default methods
  • Interface nested types

However, each of these features previously had a workaround that we performed so the code could be used by .NET. This means that users who are updating their existing bindings may find API breaks as we now use the new method instead of the workaround. Thus each of these needs a way for the user to opt out of the new behavior to preserve API compatibility.

New MSBuild Properties

  • <AndroidBoundInterfacesContainConstants> - Controls whether constants on interfaces will be supported, or the old workaround of creating a IMyInterfaceConsts class will be used.
  • <AndroidBoundInterfacesContainTypes> - Controls whether types nested in interfaces will be supported, or the old workaround of creating a non-nested type like IMyInterfaceMyNestedClass.
  • <AndroidBoundInterfacesContainStaticAndDefaultInterfaceMethods> - Controls whether default and static members on interfaces will be supported, or the old workaround of creating a sibling class containing static members like abstract class MyInterface.

.NET 6

For .NET 6, the above properties all default to true if not explicitly set by the user.

Legacy

For legacy, the above properties all default to false. The "legacy" way of enabling these features was a single preview flag <_EnableInterfaceMembers>. We will continue to support this flag for legacy only, and it will enable all the above flags, unless a user has explicitly set one of the flags.

AndroidCodegenTarget

.NET 6 binding projects will only support XAJavaInterop1, so other values are ignored.

@jpobst jpobst marked this pull request as ready for review July 2, 2021 13:52
Copy link
Contributor

@dellis1972 dellis1972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good :)
We do need to document the three new properties in https://github.com/xamarin/xamarin-android/blob/main/Documentation/guides/building-apps/build-properties.md though as they are public. This is in addition to the docs you added here already.

@jonpryor
Copy link
Member

jonpryor commented Jul 7, 2021

Time for our favorite past-time, bikeshedding names!

The names in play: $(AndroidEnableBindingInterfaceConstants), $(AndroidEnableBindingNestedInterfaceTypes), $(AndroidEnableBindingStaticAndDefaultInterfaceMethods).

The semantics are a tad odd with $(AndroidEnableBindingInterfaceConstants), as the implication is that "interface constants" weren't previously bound. But they were! This just changes where they're bound, from an "interface alternative" type that isn't the interface, to the interface itself; see also xamarin/java.interop@105d5443.

$(AndroidEnableBindingStaticAndDefaultInterfaceMethods) is in a somewhat similar boat, in that the "interface alternative" type could contain static methods, yet the name implies that they weren't supported at all.

Thus, I'm not fond of these names. The actual semantic changes aren't hinted at; it's impossible to know what they actually mean without reading the docs.

I would instead suggest:

  • $(AndroidBoundInterfacesContainConstants) instead of $(AndroidEnableBindingInterfaceConstants)
  • $(AndroidBoundInterfacesContainTypes) instead of $(AndroidEnableBindingNestedInterfaceTypes)
  • $(AndroidBoundInterfacesContainStaticAndDefaultInterfaceMethods) instead of $(AndroidEnableBindingStaticAndDefaultInterfaceMethods)

Though I do wonder if we need this much granularity in a "public-facing" manner. The real question is, are we emitting C#7-compatible interfaces, in which interfaces only contain properties, events and method declarations, or are we emitting C#8-required interfaces, in which interfaces can also contain fields, static methods, methods with bodies, nested types, etc.

Thus, should we "unify" them into a single property, e.g. $(AndroidBoundInterfacesMimicJava)? ("Mimic Java" because, as far as most are concerned, Java supported these language features before C#, and enabling these features cause our bindings to more closely mirror the original Java declarations.)

@jpobst
Copy link
Contributor Author

jpobst commented Jul 8, 2021

Capturing some discussion:

[3:24 PM] jpobst: i think the name changes are fine, i believe the granularity is helpful if you want to move forward to C# 8, but might need to disable parts for backwards compatbility
[3:44 PM] jonathanpeppers: Users probably wouldn't ever turn these off, would they?
They just get opted into them for .NET 6
[3:44 PM] jpobst: correct, unless they are migrating and need to maintain backwards compat
[3:44 PM] jonathanpeppers: Maybe the other names could just be private (as an escape hatch), and we have a single $(AndroidBoundInterfacesMimicJava) property that covers most cases
[3:51 PM] jpobst: so like for all the migration documentation: https://github.com/xamarin/xamarin-android/pull/6066/files#diff-d3fc71da91b99201132a14e3e1179d3b2fcb4aa49f920793246a2b661fb3beacR73, would we not mention that you can turn off individual features? or would we document the private flags?
[3:52 PM] jonp: indeed, good question.
[3:54 PM] jonp: @jpobst: alternate take: instead of controlling whether e.g. IFoo.Bar is emitted, it should "always" be emitted in a net6 context.  Instead, the question is whether the Foo.Bar "interface alternative" is generated.
[3:54 PM] jonp: could we have it so that both can be done, with Foo.Bar emitted for compat purposes, and IFoo.Bar emitted because It's Good And Proper
[3:54 PM] jpobst: i think each of these features has a different answer for that
[3:55 PM] jpobst: like constants, yes, but nested types, no
[3:55 PM] jonp: hm, indeed
[3:56 PM] jonp: which suggests we do want the fine-grained control

@jonpryor jonpryor merged commit d60e717 into main Jul 14, 2021
@jonpryor jonpryor deleted the net6-binding-defaults branch July 14, 2021 19:00
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 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.

[.NET 6] Binding projects should default enable C# 8 features
4 participants