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

Use of INSCopying in binding library causes CS8767 nullability warning #17130

Closed
mattjohnsonpint opened this issue Dec 29, 2022 · 8 comments · Fixed by #21099
Closed

Use of INSCopying in binding library causes CS8767 nullability warning #17130

mattjohnsonpint opened this issue Dec 29, 2022 · 8 comments · Fixed by #21099
Labels
bug If an issue is a bug or a pull request a bug fix generator Issues affecting the generator
Milestone

Comments

@mattjohnsonpint
Copy link

Steps to Reproduce

  1. Create an iOS binding library: dotnet new iosbinding -n MyBindingProject
  2. Update ApiDefinition.cs to:
    using Foundation;
    
    namespace MyBindingProject
    {
        [BaseType (typeof (NSObject))]
        interface Widget : INSCopying
        {
        }
    }
  3. Compile: dotnet build

Expected Behavior

Compiles without warnings, and generated Widget class contains:

public virtual NSObject Copy (NSZone? zone)

Actual Behavior

Gives CS8767 warning:

/Users/matt/temp/MyBindingProject/obj/Debug/net7.0-ios/iOS/MyBindingProject/Widget.g.cs(87,27): warning CS8767: Nullability of reference types in type of parameter 'zone' of 'NSObject Widget.Copy(NSZone zone)' doesn't match implicitly implemented member 'NSObject INSCopying.Copy(NSZone? zone)' (possibly because of nullability attributes). [/Users/matt/temp/MyBindingProject/MyBindingProject.csproj]

Generated Widget class contains:

public virtual NSObject Copy (NSZone zone)

... and its implementation (in Widget.g.cs) assumes zone is non-nullable, so will throw when passed a null.

[Export ("copyWithZone:")]
[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
[Preserve (Conditional = true)]
public virtual NSObject Copy (NSZone zone)
{
    var zone__handle__ = zone!.GetNonNullHandle (nameof (zone));
    if (IsDirectBinding) {
        return Runtime.GetNSObject (global::ApiDefinition.Messaging.NativeHandle_objc_msgSend_NativeHandle (this.Handle, Selector.GetHandle ("copyWithZone:"), zone.Handle))!;
    } else {
        return Runtime.GetNSObject (global::ApiDefinition.Messaging.NativeHandle_objc_msgSendSuper_NativeHandle (this.SuperHandle, Selector.GetHandle ("copyWithZone:"), zone.Handle))!;
    }
}

Environment

Version information
Visual Studio Enterprise 2022 for Mac Preview
Version 17.5 Preview (17.5 build 437)
Installation UUID: 6031c5dd-6198-4923-98fb-3d9a34ae3f55

Runtime
.NET 7.0.0 (64-bit)
Architecture: Arm64

Roslyn (Language Service)
4.4.0-3.22461.4+8ab250290a4010c11a21521f78dbc87dbb7aac81

NuGet
Version: 6.3.1.1

.NET SDK (Arm64)
SDK: /usr/local/share/dotnet/sdk/7.0.101/Sdks
SDK Versions:
	7.0.101
	7.0.100
	6.0.404
	6.0.403
	6.0.402
MSBuild SDKs: /Applications/Visual Studio (Preview).app/Contents/MonoBundle/MSBuild/Current/bin/Sdks

.NET SDK (x64)
SDK Versions:
	6.0.404
	6.0.403
	3.1.426
	3.1.425

.NET Runtime (Arm64)
Runtime: /usr/local/share/dotnet/dotnet
Runtime Versions:
	7.0.1
	7.0.0
	6.0.12
	6.0.11
	6.0.10

.NET Runtime (x64)
Runtime: /usr/local/share/dotnet/x64/dotnet
Runtime Versions:
	6.0.12
	6.0.11
	3.1.32
	3.1.31
	3.1.30

Xamarin.Profiler
Version: 1.8.0.19
Location: /Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

Updater
Version: 11

Apple Developer Tools
Xcode 14.2 (21534)
Build 14C18

Xamarin.Mac
Version: 8.12.0.2 (Visual Studio Enterprise)
Hash: 87f98a75e
Branch: d17-3
Build date: 2022-07-25 20:18:54-0400

Xamarin.iOS
Version: 16.0.0.72 (Visual Studio Enterprise)
Hash: 6756a1146
Branch: release/6.0.4xx-xcode14
Build date: 2022-09-21 08:51:06-0400

Xamarin Designer
Version: 17.5.1.13
Hash: 14bdde937f
Branch: remotes/origin/d17-5
Build date: 2022-11-02 21:36:49 UTC

Xamarin.Android
Version: 13.1.0.1 (Visual Studio Enterprise)
Commit: xamarin-android/d17-4/13ba222
Android SDK: /Users/matt/Library/Android/sdk
	Supported Android versions:
		12.1 (API level 32)
		12.0 (API level 31)
		8.1  (API level 27)
		11.0 (API level 30)
		10.0 (API level 29)
		9.0  (API level 28)
		13.0 (API level 33)

SDK Command-line Tools Version: 7.0
SDK Platform Tools Version: 33.0.3
SDK Build Tools Version: 33.0.0 rc4

Build Information: 
Mono: a96bde9
Java.Interop: xamarin/java.interop/d17-4@fcc33ce2
SQLite: xamarin/sqlite/3.39.3@23e1ae7
Xamarin.Android Tools: xamarin/xamarin-android-tools/main@0be567a

Microsoft Build of OpenJDK
Java SDK: /Library/Java/JavaVirtualMachines/microsoft-11.jdk
11.0.16.1
Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

Eclipse Temurin JDK
Java SDK: /Library/Java/JavaVirtualMachines/temurin-8.jdk
1.8.0.302
Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

Android SDK Manager
Version: 17.5.0.8
Hash: 6f27afd
Branch: remotes/origin/HEAD
Build date: 2022-11-02 21:36:54 UTC

Android Device Manager
Version: 0.0.0.1217
Hash: 6175224
Branch: main
Build date: 2022-11-02 21:36:54 UTC

Build Information
Release ID: 1705000437
Git revision: a5d0fb913c1ff781c80fe725a3e0511c34e26a08
Build date: 2022-11-02 21:34:22+00
Build branch: release-17.5
Build lane: release-17.5

Operating System
Mac OS X 12.6.1
Darwin 21.6.0 Darwin Kernel Version 21.6.0
    Thu Sep 29 20:13:56 PDT 2022
    root:xnu-8020.240.7~1/RELEASE_ARM64_T6000 arm64

Build Logs

msbuild.binlog.zip

Example Project

MyBindingProject.zip

@mattjohnsonpint
Copy link
Author

mattjohnsonpint commented Dec 29, 2022

Because the zone parameter is ignored (per Apple docs), it seems a viable workaround is to ignore the warning (optionally with a NoWarn in the csproj) and to always pass NSZone.Default.

Even better, an extension method such as follows, which also keeps the result strongly typed.

public static T Copy<T>(this T obj) where T : NSObject, INSCopying => (T)obj.Copy(NSZone.Default);

@mattjohnsonpint
Copy link
Author

Oh wait, nevermind about my last suggestion. It seems that NSObject.Copy already exists.
https://developer.apple.com/documentation/objectivec/nsobject/1418807-copy

So I guess there's not much point in even applying INSCopying in the bindings at all? Copy should still work, right?

@rolfbjarne
Copy link
Member

This is the implementation of NSObject.Copy:

[Export ("copy")]
[return: ReleaseAttribute ()]
[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
public virtual NSObject Copy ()
{
	if (!(this is INSCopying)) throw new InvalidOperationException ("Type does not conform to NSCopying");
	NSObject? ret;
	if (IsDirectBinding) {
		ret = Runtime.GetNSObject (global::ObjCRuntime.Messaging.NativeHandle_objc_msgSend (this.Handle, Selector.GetHandle ("copy")))!;
	} else {
		ret = Runtime.GetNSObject (global::ObjCRuntime.Messaging.NativeHandle_objc_msgSendSuper (this.SuperHandle, Selector.GetHandle ("copy")))!;
	}
	if (ret != null)
		global::ObjCRuntime.Messaging.void_objc_msgSend (ret.Handle, Selector.GetHandle ("release"));
	return ret!;
}

so since it checks if the current type implements INSCopying, it seems you need to implement the INSCopying interface in your binding.

@rolfbjarne rolfbjarne added bug If an issue is a bug or a pull request a bug fix generator Issues affecting the generator labels Jan 9, 2023
@rolfbjarne rolfbjarne added this to the Future milestone Jan 9, 2023
@bruno-garcia
Copy link
Contributor

Any updates on this one? (going through our bindings script and checking links for updates). Updating bindings is extremely difficult

@rolfbjarne
Copy link
Member

Any updates on this one? (going through our bindings script and checking links for updates). Updating bindings is extremely difficult

I've just implemented a fix for this. It might take a little while until it's released though (at the very latest it'll be with .NET 9 later in the fall).

@rolfbjarne
Copy link
Member

Any updates on this one? (going through our bindings script and checking links for updates). Updating bindings is extremely difficult

I've just implemented a fix for this. It might take a little while until it's released though (at the very latest it'll be with .NET 9 later in the fall).

Scratch that, I got confused with #17109 and posted here instead of in #17109.

@bruno-garcia
Copy link
Contributor

Thanks @rolfbjarne. Will this land on .NET 9?

@rolfbjarne
Copy link
Member

Thanks @rolfbjarne. Will this land on .NET 9?

Yes, this will be included in .NET 9.

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
3 participants