Skip to content

Commit

Permalink
Fix Java.Interop.Export-Tests
Browse files Browse the repository at this point in the history 
A funny thing happened on the way through CI:
`Java.Interop.Export-Tests` *with* `jnimarshalmethod-gen` failed!

	  Failed AddExportMethods [169 ms]
	  Error Message:
	   Java.Interop.JavaException : Could not initialize class com.xamarin.interop.export.ExportType
	  Stack Trace:
	     at Java.Interop.JniEnvironment.StaticMethods.GetStaticMethodID(JniObjectReference type, String name, String signature) in /Users/runner/work/1/s/src/Java.Interop/obj/Release/net7.0/JniEnvironment.g.cs:line 21407
	   at Java.Interop.JniType.GetStaticMethod(String name, String signature) in /Users/runner/work/1/s/src/Java.Interop/Java.Interop/JniType.cs:line 315
	   at Java.InteropTests.MarshalMemberBuilderTest.AddExportMethods()
	   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
	   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
	  --- End of managed Java.Interop.JavaException stack trace ---
	java.lang.NoClassDefFoundError: Could not initialize class com.xamarin.interop.export.ExportType

The cause?  e1822f0 updated `jnimarshalmethod-gen`:

	- registrations.Add (new ExpressionMethodRegistration (name, signature, mmDef));
	+ // Assume that `JavaCallableAttribute.Name` is "public" JCW name, and JCW's declare `n_`-prefixed `native` methods…
	+ registrations.Add (new ExpressionMethodRegistration ("n_" + method.Name, signature, mmDef));

That is, instead of attempting to register e.g.
`ExportType.action()V` (via `[JavaCallable("action")]`), it was
instead attempting to register `ExportType.n_action()V`, because of
the introduced `"n_" + method.Name` change.

The "problem" is that `jnimarshalmethod-gen` was written in the
context of Java.Interop, *not* .NET Android, and the existing
`Java.Interop.Export-Tests` unit tests assume that there is no
`n_`-prefix on native method declarations.

There are two plausible solutions: update the unit tests to conform
to existing .NET Android convention, and use an `n_` prefix on
`native` method declarations.

Or update `jcw-gen` so that when using
`jcw-gen --codegen-target=JavaInterop1`, the `JavaCallableAttribute.Name`
value is used for the `native` method declaration.

Because @jonpryor is a glutton for punishment and "cleanliness",
let's try the latter.  `JavaCallableWrapperGenerator` already has
a `.CodeGenerationTarget` property, How Hard Could It Be™?

Turns out, harder than expected: `JavaCallableWrapperGenerator`
was doing *lots* of work in its constructor, including the all
important bit of populating `Signature` values, and the constructor
runs *before* the `.CodeGenerationTarget` property is set.
This is a somewhat straightforward fix: turn most of the
`JavaCallableWrapperGenerator` constructor into a `Initialize()`
method, and call `Initialize()` from the public methods.
This creates a semantic change: some exceptions which were thrown
by the constructor are now thrown from the public methods.
We deem this as acceptable because all known uses of
`JavaCallableWrapperGenerator` ~immediately call `.Generate()` after
creating the instance, so the exception will still be thrown from a
site near where it previously would have been thrown.

The more annoying aspect is `Signature` initialization: we need to
pass in `.CodeGenerationTarget`, which is Yet Another Constructor
Parameter, and we already have A Lot™.  Attempt to bring sanity to
this mess by introducing a new `SignatureOptions` type to hold the
`Signature` parameters.

Update `jnimarshalmethod-gen` to undo the change which broke
`Java.Interop.Export-Tests`.

Update `tests/Java.Interop.Tools.JavaCallableWrappers-Tests` to
add a test for `.CodeGenerationTarget==JavaInterop1`.

Add `$(NoWarn)` to `Java.Interop.Tools.JavaCallableWrappers-Tests.csproj`
in order to "work around" the warnings-as-errors:

	…/src/Java.Interop.NamingCustomAttributes/Java.Interop/ExportFieldAttribute.cs(19,63): error CA1019: Remove the property setter from Name or reduce its accessibility because it corresponds to positional argument name
	…/src/Java.Interop.NamingCustomAttributes/Android.Runtime/RegisterAttribute.cs(53,4): error CA1019: Remove the property setter from Name or reduce its accessibility because it corresponds to positional argument name
	…/src/Java.Interop.NamingCustomAttributes/Java.Interop/ExportFieldAttribute.cs(12,16): error CA1813: Avoid unsealed attributes
	…

This is "weird"; the warnings/errors appear to come in because
`Java.Interop.Tools.JavaCallableWrappers-Tests.csproj` now has:

	<Compile Include="..\..\src\Java.Interop\Java.Interop\JniTypeSignatureAttribute.cs" />

which appears to pull in `src/Java.Interop/.editorconfig`, which makes
CA1019 and CA1813 errors.

(insert massively confused face here.  Like, wat?)
  • Loading branch information
@jonpryor
jonpryor committed Nov 7, 2023
1 parent e1822f0 commit 9027591
Show file tree
Hide file tree
Showing 8 changed files with 218 additions and 46 deletions.
3 changes: 2 additions & 1 deletion src/Java.Interop.Export/Java.Interop/JavaCallableAttribute.cs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#nullable enable
using System;

namespace Java.Interop {

[AttributeUsage (AttributeTargets.Method, AllowMultiple=false)]
[AttributeUsage (AttributeTargets.Constructor | AttributeTargets.Method, AllowMultiple=false)]
public sealed class JavaCallableAttribute : Attribute {

public JavaCallableAttribute ()
Expand Down
2 changes: 1 addition & 1 deletion src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<LangVersion>8.0</LangVersion>
<LangVersion>11.0</LangVersion>
<Nullable>enable</Nullable>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<SignAssembly>true</SignAssembly>
Expand Down
177 changes: 138 additions & 39 deletions ...aCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/JavaCallableWrapperGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,18 @@ void AddNestedTypes (TypeDefinition type)
type.IsSubclassOf ("Android.Content.ContentProvider", cache)))
Diagnostic.Error (4203, LookupSource (type), Localization.Resources.JavaCallableWrappers_XA4203, jniName);

this.outerType = outerType;
}

bool initialized;
string? outerType;

void Initialize ()
{
if (initialized)
return;
initialized = true;

foreach (MethodDefinition minfo in type.Methods.Where (m => !m.IsConstructor)) {
var baseRegisteredMethod = GetBaseRegisteredMethod (minfo);
if (baseRegisteredMethod != null)
Expand Down Expand Up @@ -298,7 +310,7 @@ void AddConstructor (MethodDefinition ctor, TypeDefinition type, string? outerTy
if (!string.IsNullOrEmpty (eattr.Name)) {
// Diagnostic.Warning (log, "Use of ExportAttribute.Name property is invalid on constructors");
}
ctors.Add (new Signature (ctor, eattr, cache));
ctors.Add (new Signature (new (cache, CodeGenerationTarget, ctor, eattr)));
curCtors.Add (ctor);
return;
}
Expand All @@ -307,7 +319,10 @@ void AddConstructor (MethodDefinition ctor, TypeDefinition type, string? outerTy
if (rattr != null) {
if (ctors.Any (c => c.JniSignature == rattr.Signature))
return;
ctors.Add (new Signature (ctor, rattr, managedParameters, outerType, cache));
ctors.Add (new Signature (new (cache, CodeGenerationTarget, ctor, rattr) {
ManagedParameters = managedParameters,
OuterType = outerType,
}));
curCtors.Add (ctor);
return;
}
Expand All @@ -328,12 +343,26 @@ void AddConstructor (MethodDefinition ctor, TypeDefinition type, string? outerTy
}

if (baseCtors.Any (m => m.Parameters.AreParametersCompatibleWith (ctor.Parameters, cache))) {
ctors.Add (new Signature (".ctor", jniSignature, "", managedParameters, outerType, null));
ctors.Add (new Signature (new (cache, CodeGenerationTarget) {
RegisterName = ".ctor",
RegisterSignature = jniSignature,
RegisterConnector = "",
ManagedParameters = managedParameters,
OuterType = outerType,
IsDynamicallyRegistered = true,
}));
curCtors.Add (ctor);
return;
}
if (baseCtors.Any (m => !m.HasParameters)) {
ctors.Add (new Signature (".ctor", jniSignature, "", managedParameters, outerType, ""));
ctors.Add (new Signature (new (cache, CodeGenerationTarget) {
RegisterName = ".ctor",
RegisterSignature = jniSignature,
RegisterConnector = "",
ManagedParameters = managedParameters,
OuterType = outerType,
IsDynamicallyRegistered = true,
}));
curCtors.Add (ctor);
return;
}
Expand Down Expand Up @@ -489,15 +518,17 @@ void AddMethod (MethodDefinition? registeredMethod, MethodDefinition implemented
Diagnostic.Error (4217, LookupSource (implementedMethod), Localization.Resources.JavaCallableWrappers_XA4217, attr.Name);

bool shouldBeDynamicallyRegistered = methodClassifier?.ShouldBeDynamicallyRegistered (type, registeredMethod, implementedMethod, attr.OriginAttribute) ?? true;
var msig = new Signature (implementedMethod, attr, cache, shouldBeDynamicallyRegistered);
var msig = new Signature (new (cache, CodeGenerationTarget, implementedMethod, attr) {
IsDynamicallyRegistered = shouldBeDynamicallyRegistered,
});
if (!registeredMethod.IsConstructor && !methods.Any (m => m.Name == msig.Name && m.Params == msig.Params))
methods.Add (msig);
}
foreach (ExportAttribute attr in GetExportAttributes (implementedMethod)) {
if (type.HasGenericParameters)
Diagnostic.Error (4206, LookupSource (implementedMethod), Localization.Resources.JavaCallableWrappers_XA4206);

var msig = new Signature (implementedMethod, attr, cache);
var msig = new Signature (new (cache, CodeGenerationTarget, implementedMethod, attr));
if (!string.IsNullOrEmpty (attr.SuperArgumentsString)) {
// Diagnostic.Warning (log, "Use of ExportAttribute.SuperArgumentsString property is invalid on methods");
}
Expand All @@ -508,7 +539,7 @@ void AddMethod (MethodDefinition? registeredMethod, MethodDefinition implemented
if (type.HasGenericParameters)
Diagnostic.Error (4207, LookupSource (implementedMethod), Localization.Resources.JavaCallableWrappers_XA4207);

var msig = new Signature (implementedMethod, attr, cache);
var msig = new Signature (new (cache, CodeGenerationTarget, implementedMethod, attr));
if (!implementedMethod.IsConstructor && !methods.Any (m => m.Name == msig.Name && m.Params == msig.Params)) {
methods.Add (msig);
exported_fields.Add (new JavaFieldInfo (implementedMethod, attr.Name, cache));
Expand Down Expand Up @@ -557,6 +588,8 @@ string GetManagedParameters (MethodDefinition ctor, string? outerType)

public void Generate (TextWriter writer)
{
Initialize ();

if (!string.IsNullOrEmpty (package)) {
writer.WriteLine ("package " + package + ";");
writer.WriteLine ();
Expand Down Expand Up @@ -773,7 +806,7 @@ void GenerateRegisterType (TextWriter sw, JavaCallableWrapperGenerator self, str

foreach (Signature method in self.methods) {
if (method.IsDynamicallyRegistered) {
sw.Write ("\t\t\t\"", method.Method);
sw.Write ("\t\t\t\"");
sw.Write (method.Method);
sw.WriteLine ("\\n\" +");
}
Expand Down Expand Up @@ -839,65 +872,121 @@ bool CannotRegisterInStaticConstructor (TypeDefinition type)
return JavaNativeTypeManager.IsApplication (type, cache) || JavaNativeTypeManager.IsInstrumentation (type, cache);
}

class Signature {
record struct SignatureOptions (IMetadataResolver Cache, JavaPeerStyle Style) {

public Signature (MethodDefinition method, RegisterAttribute register, IMetadataResolver cache, bool shouldBeDynamicallyRegistered = true) : this (method, register, null, null, cache, shouldBeDynamicallyRegistered) {}
public SignatureOptions (IMetadataResolver cache, JavaPeerStyle style, MethodDefinition method)
: this (cache, style)
{
Method = method;
IsStatic = method.IsStatic;
JavaAccess = JavaCallableWrapperGenerator.GetJavaAccess (method.Attributes & MethodAttributes.MemberAccessMask);
Annotations = JavaCallableWrapperGenerator.GetAnnotationsString ("\t", method.CustomAttributes, cache);
IsDynamicallyRegistered = true;
}

public Signature (MethodDefinition method, RegisterAttribute register, string? managedParameters, string? outerType, IMetadataResolver cache, bool shouldBeDynamicallyRegistered = true)
: this (register.Name, register.Signature, register.Connector, managedParameters, outerType, null)
public SignatureOptions (IMetadataResolver cache, JavaPeerStyle style, MethodDefinition method, RegisterAttribute register)
: this (cache, style, method)
{
Annotations = JavaCallableWrapperGenerator.GetAnnotationsString ("\t", method.CustomAttributes, cache);
IsDynamicallyRegistered = shouldBeDynamicallyRegistered;
Register = register;
}

public Signature (MethodDefinition method, ExportAttribute export, IMetadataResolver cache)
: this (method.Name, GetJniSignature (method, cache), "__export__", null, null, export.SuperArgumentsString)
public SignatureOptions (IMetadataResolver cache, JavaPeerStyle style, MethodDefinition method, ExportAttribute export)
: this (cache, style, method)
{
IsExport = true;
IsStatic = method.IsStatic;
JavaAccess = JavaCallableWrapperGenerator.GetJavaAccess (method.Attributes & MethodAttributes.MemberAccessMask);
ThrownTypeNames = export.ThrownNames;
JavaNameOverride = export.Name;
Annotations = JavaCallableWrapperGenerator.GetAnnotationsString ("\t", method.CustomAttributes, cache);
IsExport = true;
SuperCall = export.SuperArgumentsString;
ThrownTypeNames = export.ThrownNames;
JavaNameOverride = export.Name;
if (style == JavaPeerStyle.JavaInterop1) {
RegisterName = export.Name;
}
}

public Signature (MethodDefinition method, ExportFieldAttribute exportField, IMetadataResolver cache)
: this (method.Name, GetJniSignature (method, cache), "__export__", null, null, null)
public SignatureOptions (IMetadataResolver cache, JavaPeerStyle style, MethodDefinition method, ExportFieldAttribute exportField)
: this (cache, style, method)
{
IsExport = true;
Annotations = null;

if (method.HasParameters)
Diagnostic.Error (4205, JavaCallableWrapperGenerator.LookupSource (method), Localization.Resources.JavaCallableWrappers_XA4205);
if (method.ReturnType.MetadataType == MetadataType.Void)
Diagnostic.Error (4208, JavaCallableWrapperGenerator.LookupSource (method), Localization.Resources.JavaCallableWrappers_XA4208);
IsExport = true;
IsStatic = method.IsStatic;
JavaAccess = JavaCallableWrapperGenerator.GetJavaAccess (method.Attributes & MethodAttributes.MemberAccessMask);
}


// annotations are processed within JavaFieldInfo, not the initializer method. So we don't generate them here.
public string? RegisterName;
public string? RegisterSignature;
public string? RegisterConnector;
public RegisterAttribute Register {
set {
RegisterName = value.Name;
RegisterSignature = value.Signature;
RegisterConnector = value.Connector;
}
}
public MethodDefinition Method {
set {
RegisterName = value.Name;
RegisterSignature = GetJniSignature (value, Cache);
RegisterConnector = "__export__";
Annotations = JavaCallableWrapperGenerator.GetAnnotationsString ("\t", value.CustomAttributes, Cache);
}
}
public bool IsDynamicallyRegistered;
public string? ManagedParameters;
public string? OuterType;
public string? SuperCall;
public string? Annotations;
public string? JavaAccess;
public string? JavaNameOverride;
public bool IsExport;
public bool IsStatic;
public string[]? ThrownTypeNames;
}

public Signature (string name, string? signature, string? connector, string? managedParameters, string? outerType, string? superCall)
class Signature {

public Signature (SignatureOptions options)
{
ManagedParameters = managedParameters;
JniSignature = signature ?? throw new ArgumentNullException ("`connector` cannot be null.", nameof (connector));
Method = "n_" + name + ":" + JniSignature + ":" + connector;
Name = name;
if (options.RegisterName == null) {
throw new ArgumentNullException ("`RegisterName` cannot be null.", nameof (options.RegisterName));
}
if (options.RegisterSignature == null) {
throw new ArgumentNullException ("`RegisterSignature` cannot be null.", nameof (options.RegisterSignature));
}
Annotations = options.Annotations;
IsDynamicallyRegistered = options.IsDynamicallyRegistered;
IsExport = options.IsExport;
IsStatic = options.IsStatic;
JavaAccess = options.JavaAccess;
JavaNameOverride = options.JavaNameOverride;
JniSignature = options.RegisterSignature;
ManagedParameters = options.ManagedParameters;
Method = options.RegisterName + ":" + JniSignature + ":" + options.RegisterConnector;
Name = options.RegisterName;
ThrownTypeNames = options.ThrownTypeNames;

if (options.Style == JavaPeerStyle.XAJavaInterop1) {
Method = "n_" + Method;
}

var jnisig = JniSignature;
int closer = jnisig.IndexOf (')');
string ret = jnisig.Substring (closer + 1);
retval = JavaNativeTypeManager.Parse (ret)?.Type;
string jniparms = jnisig.Substring (1, closer - 1);
if (string.IsNullOrEmpty (jniparms) && string.IsNullOrEmpty (superCall))
if (string.IsNullOrEmpty (jniparms) && string.IsNullOrEmpty (options.SuperCall))
return;
var parms = new StringBuilder ();
var scall = new StringBuilder ();
var acall = new StringBuilder ();
bool first = true;
int i = 0;
foreach (JniTypeName jti in JavaNativeTypeManager.FromSignature (jniparms)) {
if (outerType != null) {
acall.Append (outerType).Append (".this");
outerType = null;
if (options.OuterType != null) {
acall.Append (options.OuterType).Append (".this");
options.OuterType = null;
continue;
}
string? parmType = jti.Type;
Expand All @@ -913,7 +1002,7 @@ public Signature (string name, string? signature, string? connector, string? man
++i;
}
this.parms = parms.ToString ();
this.call = superCall != null ? superCall : scall.ToString ();
this.call = options.SuperCall != null ? options.SuperCall : scall.ToString ();
this.ActivateCall = acall.ToString ();
}

Expand Down Expand Up @@ -1050,13 +1139,21 @@ void GenerateMethod (Signature method, TextWriter sw)
sw.Write (' ');
if (method.IsStatic)
sw.Write ("static ");
if (CodeGenerationTarget == JavaPeerStyle.JavaInterop1) {
sw.Write ("native ");
}
sw.Write (method.Retval);
sw.Write (' ');
sw.Write (method.JavaName);
sw.Write (" (");
sw.Write (method.Params);
sw.Write (')');
sw.WriteLine (method.ThrowsDeclaration);
sw.Write (method.ThrowsDeclaration);
if (CodeGenerationTarget == JavaPeerStyle.JavaInterop1) {
sw.WriteLine (";");
return;
}
sw.WriteLine ();
sw.WriteLine ("\t{");
#if MONODROID_TIMING
sw.WriteLine ("\t\tandroid.util.Log.i(\"MonoDroid-Timing\", \"{0}.{1}: time: \"+java.lang.System.currentTimeMillis());", name, method.Name);
Expand Down Expand Up @@ -1127,6 +1224,8 @@ StreamWriter OpenStream (string outputPath)
/// </summary>
public string GetDestinationPath (string outputPath)
{
Initialize ();

var dir = package.Replace ('.', Path.DirectorySeparatorChar);
return Path.Combine (outputPath, dir, name + ".java");
}
Expand Down
2 changes: 2 additions & 0 deletions src/Java.Interop/Java.Interop/JniTypeSignatureAttribute.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ public sealed class JniTypeSignatureAttribute : Attribute {

public JniTypeSignatureAttribute (string simpleReference)
{
#if !JCW_ONLY_TYPE_NAMES
JniRuntime.JniTypeManager.AssertSimpleReference (simpleReference, nameof (simpleReference));
#endif // !JCW_ONLY_TYPE_NAMES

SimpleReference = simpleReference;
}
Expand Down
6 changes: 6 additions & 0 deletions ...rop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers-Tests.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<TargetFramework>$(DotNetTargetFramework)</TargetFramework>
<IsPackable>false</IsPackable>
<DefineConstants>$(DefineConstants);HAVE_CECIL;JCW_ONLY_TYPE_NAMES</DefineConstants>
<NoWarn>$(NoWarn);CA1019;CA1813</NoWarn>
</PropertyGroup>

<Import Project="..\..\TargetFrameworkDependentValues.props" />
Expand All @@ -29,4 +30,9 @@

<Import Project="..\..\src\Java.Interop.NamingCustomAttributes\Java.Interop.NamingCustomAttributes.projitems" Label="Shared" Condition="Exists('..\..\src\Java.Interop.NamingCustomAttributes\Java.Interop.NamingCustomAttributes.projitems')" />

<ItemGroup>
<Compile Include="..\..\src\Java.Interop\Java.Interop\JniTypeSignatureAttribute.cs" />
<Compile Include="..\..\src\Java.Interop.Export\Java.Interop\JavaCallableAttribute.cs" />
</ItemGroup>

</Project>
Loading

0 comments on commit 9027591

Please sign in to comment.