Skip to content

Commit

Permalink
[generator] Don't generate unexpected NRT types like void? (#856)
Browse files Browse the repository at this point in the history
Context: #850 (comment)

When a user binds a Java library that contains a Java interface which
implements another Java interface:

	// Java; android.jar
	public interface /* android.view. */ Menu {…}

	// Java; androidx.core.core.jar
	public interface /* androidx.core.internal.view. */ SupportMenu implements android.view.Menu {…}

then when we create the binding for the "leaf" interface:

	// C# binding for androidx.core.core
	namespace AndroidX.Core.Internal.View {
	    public interface ISupportMenu : Android.Views.IMenu {…}

	    internal partial class ISupportMenuInvoker : Java.Lang.Object, ISupportMenu {
	        // …
	    }
	}

The generated `*Invoker` type implements all methods for all
implemented interfaces, e.g. methods from both `IMenu` and
`ISupportMenu`.

When:

 1. The base interface (e.g. `IMenu`) comes from a referenced
    assembly, *and*

 2. `$(Nullable)`=Enable when binding the derived interface

then we interpret the return types on imported interface methods as
*always* being of a nullable type, even for types such as `void`:

	// C# binding for androidx.core.core with $(Nullable)=Enable
	partial class ISupportMenuInvoker : Java.Lang.Object, ISupportMenu {

	    public unsafe void? Clear () {…}
	}

This results in errors from the C# compiler:

	Error CS1519: Invalid token '?' in class, record, struct, or interface member declaration
	Error CS1520: Method must have a return type
	Error CS0535: 'ISupportMenuInvoker' does not implement interface member 'IMenu.Clear()'

The culprit is twofold:

  - In `CecilApiImporter`, we set `Method.ManagedReturn` to
    `System.Void` instead of `void`.

  - In `CodeGenerationOptions`, we only check for `void` to omit the
    null operator, not `System.Void`.

Note this also happens for all primitive types like
`System.Int32`/`int`.

This commit fixes both aspects:

  - Change `Method.ManagedReturn` to contain primitive types instead
    of fully qualified types.

  - Update `CodeGenerationOptions.GetNullable()` to correctly handle
    fully qualified types if one slips through.

With this change, all of AndroidX/GooglePlayServices/FaceBook/MLKit
can be successfully compiled with `<Nullable>enable</Nullable>`.
  • Loading branch information
jpobst committed Jul 12, 2021
1 parent 4a02bc3 commit 855ecfa
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 12 deletions.
16 changes: 16 additions & 0 deletions tests/generator-Tests/Unit-Tests/CodeGenerationOptionsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,21 @@ public void GetOutputNameUseGlobal ()
Assert.AreEqual ("global::System.Collections.Generic.List<global::System.Collections.Generic.List<string>.Enumerator[]>",
opt.GetOutputName ("System.Collections.Generic.List<System.Collections.Generic.List<string>.Enumerator[]>"));
}

[Test]
public void GetTypeReferenceName_Nullable ()
{
var opt = new CodeGenerationOptions { SupportNullableReferenceTypes = true };

var system_void = new ReturnValue (null, "void", "System.Void", false, false);
system_void.Validate (opt, null, null);

Assert.AreEqual ("void", opt.GetTypeReferenceName (system_void));

var primitive_void = new ReturnValue (null, "void", "void", false, false);
primitive_void.Validate (opt, null, null);

Assert.AreEqual ("void", opt.GetTypeReferenceName (primitive_void));
}
}
}
4 changes: 2 additions & 2 deletions tests/generator-Tests/Unit-Tests/ManagedTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public void Method ()

Assert.AreEqual ("public", method.Visibility);
Assert.AreEqual ("void", method.Return);
Assert.AreEqual ("System.Void", method.ReturnType);
Assert.AreEqual ("void", method.ReturnType);
Assert.AreEqual ("Bar", method.Name);
Assert.AreEqual ("bar", method.JavaName);
Assert.AreEqual ("()V", method.JniSignature);
Expand Down Expand Up @@ -165,7 +165,7 @@ public void MethodWithParameters ()
Assert.IsTrue (method.Validate (new CodeGenerationOptions (), new GenericParameterDefinitionList (), new CodeGeneratorContext ()), "method.Validate failed!");
Assert.AreEqual ("(ZID)Ljava/lang/String;", method.JniSignature);
Assert.AreEqual ("java.lang.String", method.Return);
Assert.AreEqual ("System.String", method.ManagedReturn);
Assert.AreEqual ("string", method.ManagedReturn);

var parameter = method.Parameters [0];
Assert.AreEqual ("a", parameter.Name);
Expand Down
18 changes: 10 additions & 8 deletions tools/generator/CodeGenerationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,23 @@ string GetNullable (string s)
switch (s) {
case "void":
case "int":
//case "int[]":
case "bool":
//case "bool[]":
case "float":
//case "float[]":
case "sbyte":
//case "sbyte[]":
case "long":
//case "long[]":
case "char":
//case "char[]":
case "double":
//case "double[]":
case "short":
//case "short[]":
case "System.Boolean":
case "System.Byte":
case "System.Char":
case "System.Double":
case "System.Int16":
case "System.Int32":
case "System.Int64":
case "System.Single":
case "System.SByte":
case "System.Void":
case "Android.Graphics.Color":
return string.Empty;
}
Expand Down
19 changes: 19 additions & 0 deletions tools/generator/Extensions/ManagedExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,25 @@ public static string StripArity (this string type)

return type;
}

// Convert a fully qualified type like "System.Void" to the primitive type "void" if applicable
public static string FilterPrimitive (this string type)
{
return type switch {
"System.Boolean" => "bool",
"System.Char" => "char",
"System.Byte" => "byte",
"System.SByte" => "byte",
"System.Int16" => "short",
"System.Int32" => "int",
"System.Int64" => "long",
"System.Single" => "float",
"System.Double" => "double",
"System.Void" => "void",
"System.String" => "string",
_ => type
};
}
}
#endif
}
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,8 @@ public static Method CreateMethod (GenBase declaringType, MethodDefinition m)
IsStatic = m.IsStatic,
IsVirtual = m.IsVirtual,
JavaName = reg_attr != null ? ((string) reg_attr.ConstructorArguments [0].Value) : m.Name,
ManagedReturn = m.ReturnType.FullNameCorrected ().StripArity (),
Return = m.ReturnType.FullNameCorrected ().StripArity (),
ManagedReturn = m.ReturnType.FullNameCorrected ().StripArity ().FilterPrimitive (),
Return = m.ReturnType.FullNameCorrected ().StripArity ().FilterPrimitive (),
Visibility = m.Visibility ()
};

Expand Down

0 comments on commit 855ecfa

Please sign in to comment.