xamarin-macios icon indicating copy to clipboard operation
xamarin-macios copied to clipboard

Binding library internal delegate generated as public

Open bruno-garcia opened this issue 2 years ago • 3 comments

Steps to Reproduce

  1. Create an iOS binding library project on .NET 6
  2. Add a delegate in the ApiDefinition.cs:
    [Internal]
    internal delegate SentryBreadcrumb SentryBeforeBreadcrumbCallback(SentryBreadcrumb arg0);
  1. Type in argument/return of the delegate also has [Internal] and is generated with internal access modifier.
  2. dotnet build

Repro below.

Expected Behavior

Builds

Actual Behavior

/Users/bruno/git/sentry-dotnet/src/Sentry/obj/Debug/net6.0-ios/iOS/SupportDelegates.g.cs(52,46): error CS0058: Inconsistent accessibility: return type 'SentryBreadcrumb' is less accessible than delegate 'SentryBeforeBreadcrumbCallback' [/Users/bruno/git/sentry-dotnet/src/Sentry/Sentry.csproj]
/Users/bruno/git/sentry-dotnet/src/Sentry/obj/Debug/net6.0-ios/iOS/SupportDelegates.g.cs(52,46): error CS0058: Inconsistent accessibility: return type 'SentryBreadcrumb' is less accessible than delegate 'SentryBeforeBreadcrumbCallback' [/Users/bruno/git/sentry-dotnet/src/Sentry/Sentry.csproj]
/Users/bruno/git/sentry-dotnet/src/Sentry/obj/Debug/net6.0-ios/iOS/SupportDelegates.g.cs(52,46): error CS0059: Inconsistent accessibility: parameter type 'SentryBreadcrumb' is less accessible than delegate 'SentryBeforeBreadcrumbCallback' [/Users/bruno/git/sentry-dotnet/src/Sentry/Sentry.csproj]

Generated code:

namespace Sentry.iOS {
	#nullable enable
	public delegate Sentry.iOS.SentryBreadcrumb SentryBeforeBreadcrumbCallback (Sentry.iOS.SentryBreadcrumb arg0);
	public delegate Sentry.iOS.SentryEvent SentryBeforeSendEventCallback (Sentry.iOS.SentryEvent arg0);
}

Environment

Version information

workloads:

Installed Workload Ids      Installation Source
-----------------------------------------------
maui-ios                    SDK 6.0.300
maui-android                SDK 6.0.300
ios                         SDK 6.0.300
maui                        SDK 6.0.300
android                     SDK 6.0.300

SDK/runtime:

.NET SDK (reflecting any global.json):
 Version:   6.0.301
 Commit:    43f9b18481

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  12.4
 OS Platform: Darwin
 RID:         osx.12-x64
 Base Path:   /usr/local/share/dotnet/sdk/6.0.301/

Host (useful for support):
  Version: 6.0.6
  Commit:  7cca709db2

.NET SDKs installed:
  2.1.818 [/usr/local/share/dotnet/sdk]
  3.1.414 [/usr/local/share/dotnet/sdk]
  3.1.415 [/usr/local/share/dotnet/sdk]
  3.1.416 [/usr/local/share/dotnet/sdk]
  3.1.419 [/usr/local/share/dotnet/sdk]
  3.1.420 [/usr/local/share/dotnet/sdk]
  5.0.402 [/usr/local/share/dotnet/sdk]
  5.0.403 [/usr/local/share/dotnet/sdk]
  5.0.404 [/usr/local/share/dotnet/sdk]
  5.0.408 [/usr/local/share/dotnet/sdk]
  6.0.100-rc.2.21505.57 [/usr/local/share/dotnet/sdk]
  6.0.100 [/usr/local/share/dotnet/sdk]
  6.0.101 [/usr/local/share/dotnet/sdk]
  6.0.106 [/usr/local/share/dotnet/sdk]
  6.0.202 [/usr/local/share/dotnet/sdk]
  6.0.300 [/usr/local/share/dotnet/sdk]
  6.0.301 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.30 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.30 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.20 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.21 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.22 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.25 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.26 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.11 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.12 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.13 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.17 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0-rc.2.21480.10 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.1 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.4 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.5 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.6 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.30 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.20 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.21 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.22 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.25 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.26 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.11 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.12 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.13 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.17 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0-rc.2.21480.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.1 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.4 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.6 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Build Logs

Example Project (If Possible)

Repro in this commit: getsentry/sentry-dotnet@be0f2e2 (#1727) Part of this PR: https://github.com/getsentry/sentry-dotnet/pull/1727

bruno-garcia avatar Jun 18 '22 14:06 bruno-garcia

This seems to be a bug in the generator, thanks for the report!

dalexsoto avatar Jun 30 '22 15:06 dalexsoto

This seems to be a bug in the generator, thanks for the report!

Is this something we can help with a PR (would need some pointers)? Or by any chance there's a plan to address this you could share? 🙏 This blocks our native mobile .NET support, which ultimately also supports our MAUI support.

bruno-garcia avatar Jul 07 '22 13:07 bruno-garcia

Note, we were able to workaround this issue by replacing the named delegates that Sharpie generates with Func<T> or Action<T>. They seem to work fine.

The downside is that there doesn't seem to be correct handling of nullable parameters. In other words, given these two approaches:

[Internal]
delegate void MyNamedDelegate ([NullAllowed] string arg0);

[Internal]
[Export("somemethod)"]
void SomeMethod(MyNamedDelegate callback);

and

[Internal]
[Export("somemethod)"]
void SomeMethod(Action<string?> callback);

The first one doesn't compile because the delegate doesn't honor the [Internal] flag, but the second one doesn't see string?, it just sees string so the resulting generated code doesn't get marked nullable.

[NullAllowed] wouldn't work, because that generates Action<string>?, not Action<string?>

mattjohnsonpint avatar Aug 02 '22 00:08 mattjohnsonpint

I found a better workaround. We can allow the delegates to be generated and used as normal (including [NullAllowed] attributes), then have the build replace public with internal in the source file before its compiled by adding this to the csproj of the binding library:

<Target Name="_SetGeneratedSupportDelegatesInternal" BeforeTargets="CoreCompile">
  <PropertyGroup>
    <GeneratedSupportDelegatesFile>$(MSBuildThisFileDirectory)$(GeneratedSourcesDir)SupportDelegates.g.cs</GeneratedSupportDelegatesFile>
  </PropertyGroup>
  <WriteLinesToFile
    File="$(GeneratedSupportDelegatesFile)"
    Lines="$([System.IO.File]::ReadAllText($(GeneratedSupportDelegatesFile)).Replace('public delegate','internal delegate'))"
    Overwrite="true" />
</Target>

Of course, this sets all delegates to internal, but that's what I need in my case. One could adjust the string replacement if necessary.

This is still a workaround. It shouldn't be needed. The real fix should be to honor the [Internal] attribute in the following code instead of hardcoding public:

https://github.com/xamarin/xamarin-macios/blob/83b07279671c253fcf5fc79ba36657ebcc5140d3/src/generator.cs#L6074

mattjohnsonpint avatar Dec 20 '22 23:12 mattjohnsonpint

Actually, while it looks like [NullAllowed] is working on the input parameters, [return: NullAllowed] does not work. And if I try to replace to workaround, I get error CS8600: Converting null literal or possible null value to non-nullable type. in ObjCRuntime/Trampolines.g.cs.

So there's two issues here. Neither [Internal] nor [return: NullAllowed] work on delegate types.

Should I open a new issue for the latter? Or ok to track them both here?

mattjohnsonpint avatar Dec 21 '22 00:12 mattjohnsonpint

Should I open a new issue for the latter? Or ok to track them both here?

Please open a new issue, that way it's easier to make sure they're both fixed.

rolfbjarne avatar Dec 21 '22 07:12 rolfbjarne

Ok. Nullable issue is in #17109. This one can stay focused on the internal visibility.

mattjohnsonpint avatar Dec 21 '22 16:12 mattjohnsonpint

@rolfbjarne any luck this is landing in .NET 8?

bruno-garcia avatar Sep 16 '23 02:09 bruno-garcia

@rolfbjarne any luck this is landing in .NET 8?

I just proposed a fix, let's see what we can do.

rolfbjarne avatar Sep 18 '23 16:09 rolfbjarne