Skip to content

Binding library internal delegate generated as public #15299

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

Closed
bruno-garcia opened this issue Jun 18, 2022 · 9 comments · Fixed by #19038
Closed

Binding library internal delegate generated as public #15299

bruno-garcia opened this issue Jun 18, 2022 · 9 comments · Fixed by #19038
Assignees
Labels
bug If an issue is a bug or a pull request a bug fix generator Issues affecting the generator
Milestone

Comments

@bruno-garcia
Copy link
Member

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: getsentry/sentry-dotnet#1727

@dalexsoto dalexsoto added bug If an issue is a bug or a pull request a bug fix iOS Issues affecting iOS labels Jun 30, 2022
@dalexsoto dalexsoto added this to the Future milestone Jun 30, 2022
@dalexsoto dalexsoto added generator Issues affecting the generator and removed iOS Issues affecting iOS labels Jun 30, 2022
@dalexsoto
Copy link
Member

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

@bruno-garcia
Copy link
Member Author

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.

@mattjohnsonpint
Copy link

mattjohnsonpint commented Aug 2, 2022

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
Copy link

mattjohnsonpint commented Dec 20, 2022

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
Copy link

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?

@rolfbjarne
Copy link
Member

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.

@mattjohnsonpint
Copy link

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

@bruno-garcia
Copy link
Member Author

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

@rolfbjarne
Copy link
Member

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix generator Issues affecting the generator
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants