-
Notifications
You must be signed in to change notification settings - Fork 539
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
Comments
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? 🙏 |
Note, we were able to workaround this issue by replacing the named delegates that Sharpie generates with 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
|
I found a better workaround. We can allow the delegates to be generated and used as normal (including <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 |
Actually, while it looks like So there's two issues here. Neither 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. |
Ok. Nullable issue is in #17109. This one can stay focused on the internal visibility. |
@rolfbjarne any luck this is landing in .NET 8? |
I just proposed a fix, let's see what we can do. |
Steps to Reproduce
[Internal]
and is generated withinternal
access modifier.dotnet build
Repro below.
Expected Behavior
Builds
Actual Behavior
Generated code:
Environment
Version information
workloads:
SDK/runtime:
Build Logs
Example Project (If Possible)
Repro in this commit: getsentry/sentry-dotnet@
be0f2e2
(#1727)Part of this PR: getsentry/sentry-dotnet#1727
The text was updated successfully, but these errors were encountered: