Skip to content

[generator] Generate more [SupportedOSPlatform] attributes to fix warnings. #868

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
Merged
merged 1 commit into from
Aug 27, 2021
Merged

[generator] Generate more [SupportedOSPlatform] attributes to fix warnings. #868

merged 1 commit into from
Aug 27, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 18, 2021 •
edited
Loading

Fixes #863

We began emitting [SupportedOSPlatformAttribute] for net6.0 Mono.Android.dll public API:

[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android23.0")]
[global::Android.Runtime.Register ("android/telecom/InCallService", DoNotGenerateAcw=true, ApiSince = 23)]
public abstract partial class InCallService : Android.App.Service { ... }

However our invoking machinery calls this API without having the same guards:

[global::Android.Runtime.Register ("android/telecom/InCallService", DoNotGenerateAcw=true, ApiSince = 23)]
internal partial class InCallServiceInvoker : InCallService { ... }

This results in warnings:

…/xamarin-android/src/Mono.Android/obj/Release/net6.0/android-30/mcw/Android.Telecom.InCallService.cs(1287,76):
warning CA1416: This call site is reachable on all platforms. 'InCallService' is only supported on: 'android' 23.0 and later.

We need to emit the same [SupportedOSPlatformAttribute] for these methods as well.

This PR removes 4,699 CA1416 warnings from our Mono.Android.dll build. The remaining 61 CA1416 warnings are of the form:

C:\code\xamarin-android\src\Mono.Android\obj\Debug\net6.0\android-31\mcw\Java.Lang.Reflect.Method.cs(35,71): warning CA1416: This call site is reachable on all platforms. 'Executable' is only supported on: 'android' 26.0 and later. [C:\code\xamarin-android\src\Mono.Android\Mono.Android.csproj]

The problem here is that Java.Lang.Reflect.Method class is available since API-1, but it inherits from the Java.Lang.Reflect.Executable class which was added in API-26.

See:

This seems like a Mono.Android.dll issue and not something we should attempt to fix in generator.

@jpobst jpobst marked this pull request as ready for review August 19, 2021 14:29
jonpryor
jonpryor reviewed Aug 19, 2021
View reviewed changes
tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs
AddInterfaceListenerEventsAndProperties (tw, iface, target, name, setter,
string.Format ("__v => {0} = __v", prop.Name),
string.Format ("__v => {0} = null", prop.Name), opt);
string.Format ("__v => {0} = null", prop.Name), opt, prop.Getter);
Copy link
Contributor

Choose a reason for hiding this comment

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

The last AddInterfaceListenerEventsAndProperties() parameter name is setMethod, yet prop.Getter is used here. Is this "mismatch" correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to setListenerMethod to help indicate it is not about property getters/setters, it is a reference to the method that is used to set the listener, like addOnRoutingChangedListener. (Also added a comment in the code for future reference.)

@jonpryor
Copy link
Contributor

The lack of changes to unit tests tells me that we don't have any unit tests which test C# codegen with [SupportedOSPlatform]…

jonpryor
jonpryor reviewed Aug 19, 2021
View reviewed changes
tools/generator/SourceWriters/InterfaceInvokerProperty.cs
IsPublic = true;
IsUnsafe = true;

SourceWriterExtensions.AddSupportedOSPlatform (Attributes, property.Getter, opt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any instances where property getter & setter are in two separate API levels? Is this something we need to worry about or consider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of any, and at least for interfaces we probably wouldn't allow it to happen, as adding a required setter in a later API would be a breaking change. I think we can ignore this.

@jonpryor
Copy link
Contributor

Should you create a xamarin/xamarin-android PR which tests this change?

@jpobst
Copy link
Contributor Author

jpobst commented Aug 20, 2021

We only have a couple very basic unit tests to ensure that [SupportedOSPlatform] gets written:
https://github.com/xamarin/java.interop/blob/main/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs#L279-L290

Since this feature is only for Mono.Android.dll, if it ever were to break or change ApiCompat would detect it.

Spinning up a XA PR to test this is a good idea, will do.

@jpobst
Copy link
Contributor Author

jpobst commented Aug 26, 2021

Test XA merge:
dotnet/android#6213

Reduced the warnings emitted building Mono.Android.dll API-31 from 4827 to 158.

@jonpryor jonpryor merged commit f359e73 into main Aug 27, 2021
@jonpryor jonpryor deleted the more-osplatform branch August 27, 2021 16:43
@jonpryor jonpryor mentioned this pull request Aug 27, 2021
Closed
jpobst added a commit that referenced this pull request Sep 30, 2021
@jpobst
[generator] Generate more [SupportedOSPlatform] attributes (#868)
c963db5 
Fixes: #863

In commits da12df4 and 412e974 (and others) we added support to
emit the `[SupportedOSPlatformAttribute]` custom attribute when
targeting .NET 6+ builds, using the Android API-level that the member
was introduced:

	[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android23.0")]
	[global::Android.Runtime.Register ("android/telecom/InCallService", DoNotGenerateAcw=true, ApiSince = 23)]
	public abstract partial class InCallService : Android.App.Service {
	}

However, our "invoke" machinery uses this API without having the same
guards:

	[global::Android.Runtime.Register ("android/telecom/InCallService", DoNotGenerateAcw=true, ApiSince = 23)]
	internal partial class InCallServiceInvoker : InCallService {
	}

This results in build warnings when building e.g.
`xamarin-android/src/Mono.Android`:

	…/xamarin-android/src/Mono.Android/obj/Release/net6.0/android-30/mcw/Android.Telecom.InCallService.cs(1287,76):
	warning CA1416: This call site is reachable on all platforms. 'InCallService' is only supported on: 'android' 23.0 and later.

To fix these warnings, we need to emit the same
`[SupportedOSPlatformAttribute]` for these members as well:

	[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android23.0")]
	[global::Android.Runtime.Register ("android/telecom/InCallService", DoNotGenerateAcw=true, ApiSince = 23)]
	internal partial class InCallServiceInvoker : InCallService {
	}

This change removes 4,699 `CA1416` warnings from our
`Mono.Android.dll` build.

The remaining 61 `CA1416` warnings are of the form:

	…\xamarin-android\src\Mono.Android\obj\Debug\net6.0\android-31\mcw\Java.Lang.Reflect.Method.cs(35,71):
	warning CA1416: This call site is reachable on all platforms. 'Executable' is only supported on: 'android' 26.0 and later.

The problem here is that `Java.Lang.Reflect.Method` class is available
since API-1, but it inherits from the `Java.Lang.Reflect.Executable`
class which was added in API-26.

See:

  - https://developer.android.com/reference/java/lang/reflect/Method
  - https://developer.android.com/reference/java/lang/reflect/Executable

This seems like a `Mono.Android.dll` issue and not something we should
attempt to fix in `generator`.
@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.
Reviewers
1 more reviewer

@jonpryor jonpryor jonpryor left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invokers should emit [SupportedOSPlatformAttribute]
2 participants
@jpobst @jonpryor