From a3f148ceb358a1f7ffdb9c0e699edd2d2c73c652 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 20 May 2020 16:30:01 -0400 Subject: [PATCH] [Xamarin.Android.Tools.Bytecode] Support @JvmOverloads (#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @moljac ran into an interesting warning from `class-parse` when binding [OfficeUIFrabric][0]: warning : class-parse: warning: method com/microsoft/officeuifabric/widget/BottomNavigationView.(Landroid/content/Context;)V: Local variables array has 0 entries ('LocalVariableTableAttribute(LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;'))'); descriptor has 1 entries! For starters, this shouldn't be a warning; it's not actionable. There is nothing a user can do to fix this warning; it's only meaningful to the Java.Interop team. Update `Xamarin.Android.Tools.Bytecode.MethodInfo.GetParameters()` so that these messages are *debug* messages, not warnings. `class-parse` knows how many parameters are present based on the JNI method descriptor, in this case `(Landroid/content/Context;)V`, which specifies one parameter type. The JNI descriptor *doesn't* contain parameter name information; where do parameter names come from? If the `.class` file was built via `javac -parameters`, then the `MethodParametersAttribute` blob can be used; see 4273e5ce. However, before checking for `MethodParametersAttribute`, `class-parse` will attempt to use the `LocalVariableTableAttribute` and `LocalVariableTableEntry` values to infer parameter names. As part of this inference, method parameters are assumed to be variables with `LocalVariableTableEntry.StartPC` is 0; from `class-parse --dump BottomNavigationView.class`: LocalVariableTableAttribute( LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;', StartPC=0, Index=1)) Next, `class-parse` will "skip" the first variable for instance methods. This results in "skipping" the `context` variable, resulting in the message: Local variables array has 0 entries …; descriptor has 1 entries! To fix this, `class-parse` needs a stricter "skip the `this` parameter" check: the first parameter should *only* be skipped when: 1. The method is an instance method, not a `static` method, *and* 2. The JNI descriptor of the first parameter is the same as the descriptor of the declaring type. Additionally, update the code style to use `enumValue.HasFlag(V)` instead of `(enumValue & V) == V` for readability. Finally, why was this failing in the first place? In some circumstances, when the Kotlin [`@JvmOverloads`][2] annotation is used on a `constructor`, Kotlin will emit all possible overloads for the constructor, and in many of those overloads the `this` parameter *won't* be present in the `LocalVariableTableAttribute` data, as seen above with `BottomNavigationView`. [0]: https://jcenter.bintray.com/com/microsoft/uifabric/OfficeUIFabric/ [1]: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.jvm/-jvm-overloads/ --- .../AttributeInfo.cs | 2 +- .../ClassFile.cs | 2 + src/Xamarin.Android.Tools.Bytecode/Methods.cs | 14 +- .../XmlClassDeclarationBuilder.cs | 2 +- .../JvmOverloadsConstructorTests.cs | 98 +++++++++++ .../Resources/JvmOverloadsConstructor.xml | 162 ++++++++++++++++++ .../kotlin/JvmOverloadsConstructor.class | Bin 0 -> 1781 bytes .../kotlin/JvmOverloadsConstructor.kt | 11 ++ 8 files changed, 284 insertions(+), 7 deletions(-) create mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/JvmOverloadsConstructorTests.cs create mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/Resources/JvmOverloadsConstructor.xml create mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/JvmOverloadsConstructor.class create mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/JvmOverloadsConstructor.kt diff --git a/src/Xamarin.Android.Tools.Bytecode/AttributeInfo.cs b/src/Xamarin.Android.Tools.Bytecode/AttributeInfo.cs index c58918a18..62ae790e9 100644 --- a/src/Xamarin.Android.Tools.Bytecode/AttributeInfo.cs +++ b/src/Xamarin.Android.Tools.Bytecode/AttributeInfo.cs @@ -429,7 +429,7 @@ public string Descriptor { public override string ToString () { - return string.Format ("LocalVariableTableEntry(Name='{0}', Descriptor='{1}')", Name, Descriptor); + return $"LocalVariableTableEntry(Name='{Name}', Descriptor='{Descriptor}', StartPC={StartPC}, Index={Index})"; } } diff --git a/src/Xamarin.Android.Tools.Bytecode/ClassFile.cs b/src/Xamarin.Android.Tools.Bytecode/ClassFile.cs index 2ab54dd28..9763c7743 100644 --- a/src/Xamarin.Android.Tools.Bytecode/ClassFile.cs +++ b/src/Xamarin.Android.Tools.Bytecode/ClassFile.cs @@ -98,6 +98,8 @@ public string PackageName { } } + public string FullJniName => "L" + ThisClass.Name.Value + ";"; + public string SourceFileName { get { var sourceFile = Attributes.Get (); diff --git a/src/Xamarin.Android.Tools.Bytecode/Methods.cs b/src/Xamarin.Android.Tools.Bytecode/Methods.cs index 3add9fcdd..c73335607 100644 --- a/src/Xamarin.Android.Tools.Bytecode/Methods.cs +++ b/src/Xamarin.Android.Tools.Bytecode/Methods.cs @@ -90,17 +90,21 @@ public ParameterInfo[] GetParameters () if (locals != null) { var names = locals.LocalVariables.Where (p => p.StartPC == 0).ToList (); int start = 0; - if ((AccessFlags & MethodAccessFlags.Static) == 0) + if (names.Count != parameters.Length && + !AccessFlags.HasFlag (MethodAccessFlags.Static) && + names.Count > start && + names [start].Descriptor == DeclaringType.FullJniName) { start++; // skip `this` parameter + } if (!DeclaringType.IsStatic && names.Count > start && (parameters.Length == 0 || parameters [0].Type.BinaryName != names [start].Descriptor)) { start++; // JDK 8? } - if (((AccessFlags & MethodAccessFlags.Synthetic) != MethodAccessFlags.Synthetic) && + if (!AccessFlags.HasFlag (MethodAccessFlags.Synthetic) && ((names.Count - start) != parameters.Length) && !enumCtor) { - Log.Warning (1,"class-parse: warning: method {0}.{1}{2}: " + + Log.Debug ("class-parse: method {0}.{1}{2}: " + "Local variables array has {3} entries ('{4}'); descriptor has {5} entries!", DeclaringType.ThisClass.Name.Value, Name, Descriptor, names.Count - start, @@ -111,7 +115,7 @@ public ParameterInfo[] GetParameters () for (int i = 0; i < max; ++i) { parameters [i].Name = names [start+i].Name; if (parameters [i].Type.BinaryName != names [start + i].Descriptor) { - Log.Warning (1, "class-parse: warning: method {0}.{1}{2}: " + + Log.Debug ("class-parse: method {0}.{1}{2}: " + "Local variable type descriptor mismatch! Got '{3}'; expected '{4}'.", DeclaringType.ThisClass.Name.Value, Name, Descriptor, parameters [i].Type.BinaryName, @@ -122,7 +126,7 @@ public ParameterInfo[] GetParameters () var sig = GetSignature (); if (sig != null) { if ((sig.Parameters.Count != parameters.Length) && !enumCtor) { - Log.Warning (1,"class-parse: warning: method {0}.{1}{2}: " + + Log.Debug ("class-parse: method {0}.{1}{2}: " + "Signature ('{3}') has {4} entries; Descriptor '{5}' has {6} entries!", DeclaringType.ThisClass.Name.Value, Name, Descriptor, Attributes.Get(), diff --git a/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs b/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs index a5d5b35e8..ccbacb489 100644 --- a/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs +++ b/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs @@ -39,7 +39,7 @@ public XElement ToXElement () GetExtendsGenericAware (), new XAttribute ("final", (classFile.AccessFlags & ClassAccessFlags.Final) != 0), new XAttribute ("name", GetThisClassName ()), - new XAttribute ("jni-signature", "L" + classFile.ThisClass.Name.Value + ";"), + new XAttribute ("jni-signature", classFile.FullJniName), GetSourceFile (), new XAttribute ("static", classFile.IsStatic), new XAttribute ("visibility", GetVisibility (classFile.Visibility)), diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/JvmOverloadsConstructorTests.cs b/tests/Xamarin.Android.Tools.Bytecode-Tests/JvmOverloadsConstructorTests.cs new file mode 100644 index 000000000..f9fd5cec7 --- /dev/null +++ b/tests/Xamarin.Android.Tools.Bytecode-Tests/JvmOverloadsConstructorTests.cs @@ -0,0 +1,98 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +using Xamarin.Android.Tools.Bytecode; + +using NUnit.Framework; + +using Assembly = System.Reflection.Assembly; + +namespace Xamarin.Android.Tools.BytecodeTests { + + [TestFixture] + public class JvmOverloadsConstructorTests : ClassFileFixture { + + const string ClassFile = "JvmOverloadsConstructor.class"; + const string XmlFile = "JvmOverloadsConstructor.xml"; + + [Test] + public void ClassFile_WithJavaType_class () + { + var c = LoadClassFile (ClassFile); + new ExpectedTypeDeclaration { + MajorVersion = 0x32, + MinorVersion = 0, + ConstantPoolCount = 59, + AccessFlags = ClassAccessFlags.Public | ClassAccessFlags.Final | ClassAccessFlags.Super, + FullName = "JvmOverloadsConstructor", + Superclass = new TypeInfo ("java/lang/Object", "Ljava/lang/Object;"), + Methods = { + new ExpectedMethodDeclaration { + Name = "", + AccessFlags = MethodAccessFlags.Public, + ReturnDescriptor = "V", + Parameters = { + new ParameterInfo ("something", "LJvmOverloadsConstructor;", "LJvmOverloadsConstructor;"), + new ParameterInfo ("id", "I", "I"), + new ParameterInfo ("imageId", "I", "I"), + new ParameterInfo ("title", "Ljava/lang/String;", "Ljava/lang/String;"), + new ParameterInfo ("useDivider", "Z", "Z"), + }, + }, + new ExpectedMethodDeclaration { + Name = "", + AccessFlags = MethodAccessFlags.Public | MethodAccessFlags.Synthetic, + ReturnDescriptor = "V", + Parameters = { + new ParameterInfo ("p0", "LJvmOverloadsConstructor;", "LJvmOverloadsConstructor;"), + new ParameterInfo ("p1", "I", "I"), + new ParameterInfo ("p2", "I", "I"), + new ParameterInfo ("p3", "Ljava/lang/String;", "Ljava/lang/String;"), + new ParameterInfo ("p4", "Z", "Z"), + new ParameterInfo ("p5", "I", "I"), + new ParameterInfo ("p6", "Lkotlin/jvm/internal/DefaultConstructorMarker;", "Lkotlin/jvm/internal/DefaultConstructorMarker;"), + }, + }, + new ExpectedMethodDeclaration { + Name = "", + AccessFlags = MethodAccessFlags.Public, + ReturnDescriptor = "V", + Parameters = { + new ParameterInfo ("something", "LJvmOverloadsConstructor;", "LJvmOverloadsConstructor;"), + new ParameterInfo ("id", "I", "I"), + new ParameterInfo ("imageId", "I", "I"), + new ParameterInfo ("title", "Ljava/lang/String;", "Ljava/lang/String;"), + }, + }, + new ExpectedMethodDeclaration { + Name = "", + AccessFlags = MethodAccessFlags.Public, + ReturnDescriptor = "V", + Parameters = { + new ParameterInfo ("something", "LJvmOverloadsConstructor;", "LJvmOverloadsConstructor;"), + new ParameterInfo ("id", "I", "I"), + new ParameterInfo ("title", "Ljava/lang/String;", "Ljava/lang/String;"), + }, + }, + new ExpectedMethodDeclaration { + Name = "", + AccessFlags = MethodAccessFlags.Public, + ReturnDescriptor = "V", + Parameters = { + new ParameterInfo ("something", "LJvmOverloadsConstructor;", "LJvmOverloadsConstructor;"), + new ParameterInfo ("title", "Ljava/lang/String;", "Ljava/lang/String;"), + }, + }, + }, + }.Assert (c); + } + + [Test] + public void XmlDeclaration_WithJavaType_class () + { + AssertXmlDeclaration (ClassFile, XmlFile); + } + } +} + diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/Resources/JvmOverloadsConstructor.xml b/tests/Xamarin.Android.Tools.Bytecode-Tests/Resources/JvmOverloadsConstructor.xml new file mode 100644 index 000000000..3cf7ae7e2 --- /dev/null +++ b/tests/Xamarin.Android.Tools.Bytecode-Tests/Resources/JvmOverloadsConstructor.xml @@ -0,0 +1,162 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/JvmOverloadsConstructor.class b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/JvmOverloadsConstructor.class new file mode 100644 index 0000000000000000000000000000000000000000..6a23a3a7d60a00f2139a390767d06c1f818fef72 GIT binary patch literal 1781 zcmb7FO>f&q5PeIE6h%ie#YC2+IIi2IjXx68cI)&*t%Ei}0S2;D)Ts~gr7LOU(4xqK zT zm9HJ$Kp7PqH!#m&$Uug|LX|;ZRa*vb+9==_LxJi_mB?vjQ(dL8$;3vW- z%bjIpu!pI58jOOTNa*}ah9BRt+x~Q?C!KjJUh!clr)t{gNna$bu@KXShW8k@-i>S= zysUyhVDSDYcvh;q6`R*b{C7b0Qry0fyvHS1ogIvHy4FlR95OqZ31!l)Wqoq>wi#Ys zoUPc}qGoH(#hI;H3RwACv6ai2qh;~pY)9K--|Sj-*L1X|wsmplT9)SM1Pd85GGsa? zt;Nh^5i0%ltEoA*V%uR<4w~0ohRwdD8}lUY35NN05Q&4~pd*qOyc3egZO2_69`Phl zcJ93W{Vw}GN<