Skip to content

Commit

Permalink
[generator] Add nullable reference types (NRT) support. (#563)
Browse files Browse the repository at this point in the history
Fixes: #468
Context: xamarin/xamarin-android#4227

Add [C#8 nullable reference type][0] (NRT) support to `generator` when
given `generator -lang-features=nullable-reference-types`.  This uses a
variety of Java annotations to infer nullable information (18c29b7)
via the `//method/@return-not-null` and `//parameter/@not-null`
attribute values within `api.xml` to "forward" nullability information
to the generated C# binding.

~~ Goals ~~

`generator` should be able to interpret the nullable annotations
provided by an input `.jar` file (via `class-parse`).  It should use 
this information to generate bindings that expose similar nullable
annotations on the produced public C# API.

For example, this Java:

	// Java
	public class Foo {
	    public void bar (@NotNull Object baz, String value) { … }
	}

Should generate this C# API:

	// C# Binding
	public class Foo : Java.Lang.Object {
	    public void Bar (Java.Lang.Object baz, string? value) { … }
	}

Additionally, the generated binding code should not produce any
additional warnings *on its own*.  That is, the internal plumbing code
itself should not create warnings.


~~ Non-Goals ~~

There exists cases in our generated plumbing code that do not play
nicely with the provability of C#8 nullable reference types.
For example, we may generate code like this:

	int Java.Lang.IComparable.CompareTo (Java.Lang.Object o)
	{
	    return CompareTo (global::Java.Interop.JavaObjectExtensions.JavaCast<Android.Util.Half>(o));
	}

Technically `.JavaCast<>()` can return `null`, which cannot be passed
to `.CompareTo (object o)` because it does not accept `null`.  In these
cases we liberally use the [null forgiving operator (`!`)][1] to
suppress warnings.  It may be desirable to change how this code is
structured to be better provably `null`-safe, however this PR does not
attempt to make those modification.  It is assumed that the code is
currently working, so `null` is prevented here via other mechanisms.

No functional changes are made to generated code.

Additionally, there are cases where Java nullable annotations can
create scenarios that will produce warnings in C#, particularly around
inheritance.  For example:

	// Java
	public class Base {
	    public void m (@NotNull Object baz) { … }
	}

	public class Derived extends Base {
	    @OverRide public void m (Object baz) { … }
	}

This would produce a C# warning such as:

	CS8610: Nullability of reference types in type of parameter 'M' doesn't match overridden member.  

`generator` will not attempt to resolve this error, it is an exercise
for the user.  This can be accomplished by fixing the Java code or
using `metadata` to override the `//@not-null` attribute such as:

	<attr path="/api/package[@name='blah']/class[@name='Foo2']/method[@name='Bar' and count(parameter)=1 and parameter[1][@type='object']]/parameter" name="not-null">true</attr>


~~ Unit Test Changes ~~

Several of the unit test "expected output" files changed their property
type from `java.lang.String` to `string`.  This occurred due to a
related refactoring of parameter & return type generation code.  This
change shouldn't be "user visible" because the unit tests don't go
through a "complete" pipeline which would involve ensuring that get-
and set-method pairs have consistent parameter & return types.

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references
[1]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-forgiving
  • Loading branch information
jpobst committed Apr 22, 2020
1 parent c19794e commit 01d0684
Show file tree
Hide file tree
Showing 99 changed files with 3,071 additions and 158 deletions.
Expand Up @@ -2,6 +2,8 @@

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<LangVersion>8.0</LangVersion>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>

Expand All @@ -23,6 +25,9 @@
<Compile Include="..\Java.Interop.Tools.TypeNameMappings\Java.Interop.Tools.TypeNameMappings\JavaNativeTypeManager.cs">
<Link>JavaNativeTypeManager.cs</Link>
</Compile>
<Compile Include="..\Java.Interop\NullableAttributes.cs">
<Link>NullableAttributes.cs</Link>
</Compile>
</ItemGroup>

<ItemGroup>
Expand Down

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/Java.Interop/Java.Interop.csproj
Expand Up @@ -19,6 +19,7 @@
<DefineConstants>DEBUG;$(DefineConstants)</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Compile Condition=" '$(TargetFramework)' != 'netstandard2.0' " Remove="NullableAttributes.cs" />
<Compile Remove="Java.Interop\JniLocationException.cs" />
</ItemGroup>
<PropertyGroup>
Expand Down
4 changes: 2 additions & 2 deletions src/Java.Interop/Java.Interop/JavaObjectArray.cs
Expand Up @@ -158,7 +158,7 @@ public override IList<T> CreateGenericValue (ref JniObjectReference reference, J
});
}

public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState (IList<T> value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState ([MaybeNull]IList<T> value, ParameterAttributes synchronize)
{
return JavaArray<T>.CreateArgumentState (value, synchronize, (list, copy) => {
var a = copy
Expand All @@ -169,7 +169,7 @@ public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState
});
}

public override void DestroyGenericArgumentState (IList<T> value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
public override void DestroyGenericArgumentState ([AllowNull]IList<T> value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
{
JavaArray<T>.DestroyArgumentState<JavaObjectArray<T>> (value, ref state, synchronize);
}
Expand Down
Expand Up @@ -2,6 +2,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
Expand Down Expand Up @@ -203,7 +204,7 @@ public override JniValueMarshalerState CreateArgumentState (object? value, Param
throw new NotSupportedException ();
}

public override JniValueMarshalerState CreateGenericArgumentState (IntPtr value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericArgumentState ([MaybeNull]IntPtr value, ParameterAttributes synchronize)
{
throw new NotSupportedException ();
}
Expand All @@ -213,7 +214,7 @@ public override JniValueMarshalerState CreateObjectReferenceArgumentState (objec
throw new NotSupportedException ();
}

public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState (IntPtr value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState ([MaybeNull]IntPtr value, ParameterAttributes synchronize)
{
throw new NotSupportedException ();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Java.Interop/Java.Interop/JniRuntime.JniTypeManager.cs
Expand Up @@ -155,7 +155,7 @@ static Type GetUnderlyingType (Type type, out int rank)
}

// `type` will NOT be an array type.
protected virtual string GetSimpleReference (Type type)
protected virtual string? GetSimpleReference (Type type)
{
return GetSimpleReferences (type).FirstOrDefault ();
}
Expand Down
28 changes: 16 additions & 12 deletions src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs
Expand Up @@ -199,7 +199,7 @@ public virtual void DisposePeerUnlessReferenced (IJavaPeerable value)
DisposePeer (h, value);
}

public abstract IJavaPeerable PeekPeer (JniObjectReference reference);
public abstract IJavaPeerable? PeekPeer (JniObjectReference reference);

public object? PeekValue (JniObjectReference reference)
{
Expand Down Expand Up @@ -261,7 +261,7 @@ static Type GetPeerType (Type type)
return type;
}

public virtual IJavaPeerable CreatePeer (ref JniObjectReference reference, JniObjectReferenceOptions transfer, Type? targetType)
public virtual IJavaPeerable? CreatePeer (ref JniObjectReference reference, JniObjectReferenceOptions transfer, Type? targetType)
{
if (disposed)
throw new ObjectDisposedException (GetType ().Name);
Expand Down Expand Up @@ -396,7 +396,9 @@ public T CreateValue<T> (ref JniObjectReference reference, JniObjectReferenceOpt
targetType = targetType ?? typeof (T);

if (typeof (IJavaPeerable).IsAssignableFrom (targetType)) {
#pragma warning disable CS8601 // Possible null reference assignment.
return (T) JavaPeerableValueMarshaler.Instance.CreateGenericValue (ref reference, options, targetType);
#pragma warning restore CS8601 // Possible null reference assignment.
}

var marshaler = GetValueMarshaler<T> ();
Expand Down Expand Up @@ -473,7 +475,9 @@ public T GetValue<T> (ref JniObjectReference reference, JniObjectReferenceOption
}

if (typeof (IJavaPeerable).IsAssignableFrom (targetType)) {
#pragma warning disable CS8601 // Possible null reference assignment.
return (T) JavaPeerableValueMarshaler.Instance.CreateGenericValue (ref reference, options, targetType);
#pragma warning restore CS8601 // Possible null reference assignment.
}

var marshaler = GetValueMarshaler<T> ();
Expand Down Expand Up @@ -607,12 +611,12 @@ public override void DestroyArgumentState (object? value, ref JniValueMarshalerS
}
}

sealed class JavaPeerableValueMarshaler : JniValueMarshaler<IJavaPeerable> {
sealed class JavaPeerableValueMarshaler : JniValueMarshaler<IJavaPeerable?> {

internal static JavaPeerableValueMarshaler Instance = new JavaPeerableValueMarshaler ();

[return: MaybeNull]
public override IJavaPeerable CreateGenericValue (ref JniObjectReference reference, JniObjectReferenceOptions options, Type? targetType)
public override IJavaPeerable? CreateGenericValue (ref JniObjectReference reference, JniObjectReferenceOptions options, Type? targetType)
{
var jvm = JniEnvironment.Runtime;
var marshaler = jvm.ValueManager.GetValueMarshaler (targetType ?? typeof(IJavaPeerable));
Expand All @@ -621,15 +625,15 @@ public override IJavaPeerable CreateGenericValue (ref JniObjectReference referen
return jvm.ValueManager.CreatePeer (ref reference, options, targetType);
}

public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState (IJavaPeerable value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState ([MaybeNull]IJavaPeerable? value, ParameterAttributes synchronize)
{
if (value == null || !value.PeerReference.IsValid)
return new JniValueMarshalerState ();
var r = value.PeerReference.NewLocalRef ();
return new JniValueMarshalerState (r);
}

public override void DestroyGenericArgumentState (IJavaPeerable value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
public override void DestroyGenericArgumentState ([MaybeNull]IJavaPeerable? value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
{
var r = state.ReferenceValue;
JniObjectReference.Dispose (ref r);
Expand Down Expand Up @@ -694,12 +698,12 @@ public override T CreateGenericValue (ref JniObjectReference reference, JniObjec
return (T) ValueMarshaler.CreateValue (ref reference, options, targetType ?? typeof (T))!;
}

public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState (T value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState ([MaybeNull]T value, ParameterAttributes synchronize)
{
return ValueMarshaler.CreateObjectReferenceArgumentState (value, synchronize);
}

public override void DestroyGenericArgumentState (T value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
public override void DestroyGenericArgumentState ([AllowNull]T value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
{
ValueMarshaler.DestroyArgumentState (value, ref state, synchronize);
}
Expand All @@ -720,12 +724,12 @@ public override Expression CreateReturnValueFromManagedExpression (JniValueMarsh
}
}

sealed class ProxyValueMarshaler : JniValueMarshaler<object> {
sealed class ProxyValueMarshaler : JniValueMarshaler<object?> {

internal static ProxyValueMarshaler Instance = new ProxyValueMarshaler ();

[return: MaybeNull]
public override object CreateGenericValue (ref JniObjectReference reference, JniObjectReferenceOptions options, Type? targetType)
public override object? CreateGenericValue (ref JniObjectReference reference, JniObjectReferenceOptions options, Type? targetType)
{
var jvm = JniEnvironment.Runtime;

Expand All @@ -748,7 +752,7 @@ public override object CreateGenericValue (ref JniObjectReference reference, Jni
return jvm.ValueManager.CreatePeer (ref reference, options, targetType);
}

public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState (object value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState ([MaybeNull]object? value, ParameterAttributes synchronize)
{
if (value == null)
return new JniValueMarshalerState ();
Expand All @@ -765,7 +769,7 @@ public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState
return new JniValueMarshalerState (p!.PeerReference.NewLocalRef ());
}

public override void DestroyGenericArgumentState (object value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
public override void DestroyGenericArgumentState (object? value, ref JniValueMarshalerState state, ParameterAttributes synchronize)
{
var vm = state.Extra as JniValueMarshaler;
if (vm != null) {
Expand Down
3 changes: 2 additions & 1 deletion src/Java.Interop/Java.Interop/JniStringValueMarshaler.cs
@@ -1,6 +1,7 @@
#nullable enable

using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.CompilerServices;
Expand All @@ -18,7 +19,7 @@ sealed class JniStringValueMarshaler : JniValueMarshaler<string?> {
return JniEnvironment.Strings.ToString (ref reference, options, targetType ?? typeof (string));
}

public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState (string? value, ParameterAttributes synchronize)
public override JniValueMarshalerState CreateGenericObjectReferenceArgumentState ([MaybeNull]string? value, ParameterAttributes synchronize)
{
var r = JniEnvironment.Strings.NewString (value);
return new JniValueMarshalerState (r);
Expand Down
Expand Up @@ -5,7 +5,7 @@ int Count {
[Register ("set_Count", "(I)V", "Getset_Count_IHandler:java.code.IMyInterfaceInvoker, ")] set;
}

java.lang.String Key {
string Key {
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='get_Key' and count(parameter)=0]"
[Register ("get_Key", "()Ljava/lang/String;", "Getget_KeyHandler:java.code.IMyInterfaceInvoker, ")] get;
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='set_Key' and count(parameter)=1 and parameter[1][@type='java.lang.String']]"
Expand Down
Expand Up @@ -39,7 +39,7 @@ public partial interface IMyInterface : IJavaObject, IJavaPeerable {
[Register ("set_Count", "(I)V", "Getset_Count_IHandler:java.code.IMyInterfaceInvoker, ")] set;
}

java.lang.String Key {
string Key {
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='get_Key' and count(parameter)=0]"
[Register ("get_Key", "()Ljava/lang/String;", "Getget_KeyHandler:java.code.IMyInterfaceInvoker, ")] get;
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='set_Key' and count(parameter)=1 and parameter[1][@type='java.lang.String']]"
Expand Down
Expand Up @@ -9,7 +9,7 @@ public partial interface IMyInterface : IJavaObject, IJavaPeerable {
[Register ("set_Count", "(I)V", "Getset_Count_IHandler:java.code.IMyInterfaceInvoker, ")] set;
}

java.lang.String Key {
string Key {
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='get_Key' and count(parameter)=0]"
[Register ("get_Key", "()Ljava/lang/String;", "Getget_KeyHandler:java.code.IMyInterfaceInvoker, ")] get;
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='set_Key' and count(parameter)=1 and parameter[1][@type='java.lang.String']]"
Expand Down
@@ -0,0 +1,11 @@
System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator ()
{
return GetEnumerator ();
}

public System.Collections.Generic.IEnumerator<char> GetEnumerator ()
{
for (int i = 0; i < Length(); i++)
yield return CharAt (i);
}

0 comments on commit 01d0684

Please sign in to comment.