Skip to content

[generator] Import NRT data from managed assemblies. #912

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.

Sign up for GitHub

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

Merged
jonpryor merged 1 commit into from
Nov 15, 2021
Merged

[generator] Import NRT data from managed assemblies. #912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/NullableReferenceTypesRocks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
using System;
using System.Linq;
using Mono.Cecil;
using Mono.Collections.Generic;

namespace Java.Interop.Tools.Cecil
{
// Partial support for determining NRT status of method and field types.
// Reference: https://github.com/dotnet/roslyn/blob/main/docs/features/nullable-metadata.md
// The basics are supported, but advanced annotations like array elements,
// type parameters, and tuples are not supported. Our use case doesn't really need them.
Copy link
Contributor

@jonpryor jonpryor Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for commit message purposes, an elaboration on "our use case doesn't really need them" would be good. Is it not possible for Java code to express the difference between String?[]? vs. String[]? (possibly nullable array containing possibly null items vs. possibly null array which won't contain any null items)? That's my assumption.

At the same time, I now wonder if we're binding arrays & collections wrong; should all arrays permit null items?

As a "spot-check", we're binding DatabaseUtils.longForQuery(SQLiteDatabase, String, String[]) as:

public static unsafe long LongForQuery (Android.Database.Sqlite.SQLiteDatabase? db, string? query, string[]? selectionArgs);

And in this case, that looks correct; null items aren't allowed:

Compare to Arrays.hashCode(Object[]), which we similarly bind as:

public static unsafe int HashCode (Java.Lang.Object[]? a);

i.e. array can be null, but elements can't be. Compare to the source code, which does allow for null elements:

implying that we should have bound this as Object?[]?, not Object[]?.

I'm not sure there's a "good" answer here, particularly since the few Java annotations I've seen around nullability work at an "outermost type" level.

However, Kotlin appears to support it:

class E {
    fun m1(v : Array<String?>?)
    {
    }

    fun m2(v : Array<String>?)
    {
    }
}

…but that said, our //@not-null attribute (01d0684) only works on the "outermost type" level as well, meaning we can't differentiate Array<String?> from Array<String?>? anyway…

Copy link
Contributor Author

@jpobst jpobst Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically some of the annotations we look for do support string?[]?, like Lorg/jetbrains/annotations/NotNull;:

package org.jetbrains.annotations;

@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})
public @interface NotNull { ... }

(source)

(My understanding is it's the ElementType.TYPE_USE that allows specifying string?[].)

However the Android ones we primarily use only support string[]?:

package androidx.annotation;

@Target({METHOD, PARAMETER, FIELD, LOCAL_VARIABLE, ANNOTATION_TYPE, PACKAGE})
public @interface NonNull { ... }

(source)

As you pointed out, our tools do not support specifying the array element nullability.

While we probably wouldn't gain much by adding support to our tools since the primary Android annotations don't support it, we probably should update generator to always emit nullable array elements, since the default should be nullable unless specified @NotNull.

So we should always emit string?[] or string?[]?.

public static class NullableReferenceTypesRocks
{
public static Nullability GetTypeNullability (this FieldDefinition field)
{
if (field.FieldType.FullName == "System.Void")
Copy link
Contributor

@jonpryor jonpryor Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A "concern" here is that this doesn't check for assembly name, so if any yahoo adds System.Void to their assembly, it will be treated the same as System.Void, mscorlib or System.Void, System.Private.CoreLib or System.Void, netstandard.

I'm not sure of a good way to say "this is a core assembly", so this concern is likely ignorable, but it is something to be aware of.

Copy link
Contributor Author

@jpobst jpobst Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can live with this. I'm not comfortable trying to enumerate every "core" assembly that they might have put System.Void in, or will in the future. 😉

return Nullability.NotNull;

// Look for explicit annotation on field
var metadata = NullableMetadata.FromAttributeCollection (field.CustomAttributes);

if (metadata != null)
return (Nullability) metadata.Data [0];

// Default nullability status for type
return GetNullableContext (field.DeclaringType.CustomAttributes);
}

public static Nullability GetReturnTypeNullability (this MethodDefinition method)
{
if (method.MethodReturnType.ReturnType.FullName == "System.Void")
return Nullability.NotNull;

// Look for explicit annotation on return type
var metadata = NullableMetadata.FromAttributeCollection (method.MethodReturnType.CustomAttributes);

if (metadata != null)
return (Nullability) metadata.Data [0];

// Default nullability status for method
var nullable = GetNullableContext (method.CustomAttributes);

if (nullable != Nullability.Oblivous)
return nullable;

// Default nullability status for type
return GetNullableContext (method.DeclaringType.CustomAttributes);
}

public static Nullability GetTypeNullability (this ParameterDefinition parameter, MethodDefinition method)
{
if (parameter.ParameterType.FullName == "System.Void")
return Nullability.NotNull;

// Look for explicit annotation on parameter
var metadata = NullableMetadata.FromAttributeCollection (parameter.CustomAttributes);

if (metadata != null)
return (Nullability) metadata.Data [0];

// Default nullability status for method
var nullable = GetNullableContext (method.CustomAttributes);

if (nullable != Nullability.Oblivous)
return nullable;

// Default nullability status for type
return GetNullableContext (method.DeclaringType.CustomAttributes);
}

static Nullability GetNullableContext (Collection<CustomAttribute> attrs)
{
var attribute = attrs.FirstOrDefault (t => t.AttributeType.FullName == "System.Runtime.CompilerServices.NullableContextAttribute");

if (attribute != null)
return (Nullability) (byte) attribute.ConstructorArguments.First ().Value;

return Nullability.Oblivous;
}
}

public enum Nullability
{
Oblivous,
NotNull,
Nullable
}

class NullableMetadata
{
public byte [] Data { get; private set; }

NullableMetadata (byte [] data) => Data = data;

NullableMetadata (byte data) => Data = new [] { data };

public static NullableMetadata? FromAttributeCollection (Collection<CustomAttribute> attrs)
{
if (attrs is null)
return null;

var attribute = attrs.FirstOrDefault (t => t.AttributeType.FullName == "System.Runtime.CompilerServices.NullableAttribute");

if (attribute is null)
return null;

var ctor_arg = attribute.ConstructorArguments.First ();

if (ctor_arg.Value is CustomAttributeArgument [] caa)
ctor_arg = caa [0];

if (ctor_arg.Value is byte b)
return new NullableMetadata (b);

if (ctor_arg.Value is byte [] b2)
return new NullableMetadata (b2);

return null;
}
}
}
49 changes: 49 additions & 0 deletions tests/generator-Tests/Unit-Tests/ManagedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,28 @@ public Dictionary<T, List<string>> DoStuff (IEnumerable<KeyValuePair<T, List<Lis
}
}

namespace NullableTestTypes
{
#nullable enable
public class NullableClass
{
[Register ("<init>", "(Ljava/lang/String;Ljava/lang/String;)", "")]
public NullableClass (string notnull, string? nullable)
{
}

public string? null_field;
public string not_null_field = string.Empty;

[Register ("nullable_return_method", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", "")]
public string? NullableReturnMethod (string notnull, string? nullable) => null;

[Register ("not_null_return_method", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", "")]
public string NotNullReturnMethod (string notnull, string? nullable) => string.Empty;
}
}
#nullable disable

namespace generatortests
{
[TestFixture]
Expand Down Expand Up @@ -250,5 +272,32 @@ public void StripArity ()
Assert.AreEqual ("System.Collections.Generic.Dictionary<T,System.Collections.Generic.List<System.String>>", @class.Methods [0].ReturnType);
Assert.AreEqual ("System.Collections.Generic.Dictionary<T,System.Collections.Generic.List<System.String>>", @class.Methods [0].ManagedReturn);
}

[Test]
public void TypeNullability ()
{
var type = module.GetType ("NullableTestTypes.NullableClass");
var gen = CecilApiImporter.CreateClass (module.GetType ("NullableTestTypes.NullableClass"), options);

var not_null_field = CecilApiImporter.CreateField (type.Fields.First (f => f.Name == "not_null_field"));
Assert.AreEqual (true, not_null_field.NotNull);

var null_field = CecilApiImporter.CreateField (type.Fields.First (f => f.Name == "null_field"));
Assert.AreEqual (false, null_field.NotNull);

var null_method = CecilApiImporter.CreateMethod (gen, type.Methods.First (f => f.Name == "NullableReturnMethod"));
Assert.AreEqual (false, null_method.ReturnNotNull);
Assert.AreEqual (true, null_method.Parameters.First (f => f.Name == "notnull").NotNull);
Assert.AreEqual (false, null_method.Parameters.First (f => f.Name == "nullable").NotNull);

var not_null_method = CecilApiImporter.CreateMethod (gen, type.Methods.First (f => f.Name == "NotNullReturnMethod"));
Assert.AreEqual (true, not_null_method.ReturnNotNull);
Assert.AreEqual (true, not_null_method.Parameters.First (f => f.Name == "notnull").NotNull);
Assert.AreEqual (false, not_null_method.Parameters.First (f => f.Name == "nullable").NotNull);

var ctor = CecilApiImporter.CreateCtor (gen, type.Methods.First (f => f.Name == ".ctor"));
Assert.AreEqual (true, ctor.Parameters.First (f => f.Name == "notnull").NotNull);
Assert.AreEqual (false, ctor.Parameters.First (f => f.Name == "nullable").NotNull);
}
}
}
1 change: 1 addition & 0 deletions tests/generator-Tests/generator-Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<TargetFrameworks>net472;net6.0</TargetFrameworks>
<IsPackable>false</IsPackable>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<LangVersion>8.0</LangVersion>
</PropertyGroup>

<PropertyGroup>
Expand Down
7 changes: 5 additions & 2 deletions tools/generator/Extensions/ManagedExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Java.Interop.Tools.TypeNameMappings;
using Java.Interop.Tools.Cecil;
using Java.Interop.Tools.TypeNameMappings;
using Mono.Cecil;
using System.Collections.Generic;
using System.Linq;
Expand Down Expand Up @@ -42,7 +43,9 @@ public static IEnumerable<Parameter> GetParameters (this MethodDefinition m, Cus
// custom enum types and cannot simply use JNI signature here.
var rawtype = e?.Current.Type;
var type = p.ParameterType.FullName == "System.IO.Stream" && e != null ? e.Current.Type : null;
yield return CecilApiImporter.CreateParameter (p, type, rawtype);
var isNotNull = p.GetTypeNullability (m) == Nullability.NotNull;

yield return CecilApiImporter.CreateParameter (p, type, rawtype, isNotNull);
}
}

Expand Down
7 changes: 5 additions & 2 deletions tools/generator/Java.Interop.Tools.Generator.Importers/CecilApiImporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Linq;
using Java.Interop.Tools.Cecil;
using Java.Interop.Tools.TypeNameMappings;
using Mono.Cecil;
using Mono.Collections.Generic;
Expand Down Expand Up @@ -101,6 +102,7 @@ public static Field CreateField (FieldDefinition f)
IsStatic = f.IsStatic,
JavaName = reg_attr != null ? ((string) reg_attr.ConstructorArguments [0].Value).Replace ('/', '.') : f.Name,
Name = f.Name,
NotNull = f.GetTypeNullability () == Nullability.NotNull,
TypeName = f.FieldType.FullNameCorrected ().StripArity (),
Value = f.Constant == null ? null : f.FieldType.FullName == "System.String" ? '"' + f.Constant.ToString () + '"' : f.Constant.ToString (),
Visibility = f.IsPublic ? "public" : f.IsFamilyOrAssembly ? "protected internal" : f.IsFamily ? "protected" : f.IsAssembly ? "internal" : "private"
Expand Down Expand Up @@ -198,6 +200,7 @@ public static Method CreateMethod (GenBase declaringType, MethodDefinition m)
JavaName = reg_attr != null ? ((string) reg_attr.ConstructorArguments [0].Value) : m.Name,
ManagedReturn = m.ReturnType.FullNameCorrected ().StripArity ().FilterPrimitive (),
Return = m.ReturnType.FullNameCorrected ().StripArity ().FilterPrimitive (),
ReturnNotNull = m.GetReturnTypeNullability () == Nullability.NotNull,
Visibility = m.Visibility ()
};

Expand All @@ -221,12 +224,12 @@ public static Method CreateMethod (GenBase declaringType, MethodDefinition m)
return method;
}

public static Parameter CreateParameter (ParameterDefinition p, string jnitype, string rawtype)
public static Parameter CreateParameter (ParameterDefinition p, string jnitype, string rawtype, bool isNotNull)
{
// FIXME: safe to use CLR type name? assuming yes as we often use it in metadatamap.
// FIXME: IsSender?
var isEnumType = GetGeneratedEnumAttribute (p.CustomAttributes) != null;
return new Parameter (TypeNameUtilities.MangleName (p.Name), jnitype ?? p.ParameterType.FullNameCorrected ().StripArity (), null, isEnumType, rawtype);
return new Parameter (TypeNameUtilities.MangleName (p.Name), jnitype ?? p.ParameterType.FullNameCorrected ().StripArity (), null, isEnumType, rawtype, isNotNull);
}

public static Parameter CreateParameter (string managedType, string javaType)
Expand Down