From 4a02bc3291ccf81e82379a426a731716a31741f8 Mon Sep 17 00:00:00 2001 From: Jonathan Pobst Date: Fri, 2 Jul 2021 11:53:14 -0500 Subject: [PATCH] Revert "[Xamarin.Android.Tools.Bytecode] hide nested types (#827)" (#855) Revert commit 4ef50813b3625cc4e7a94308135af7a0dc761ccc. Fixes: https://github.com/xamarin/java.interop/issues/854 Context: 4ef50813b3625cc4e7a94308135af7a0dc761ccc Context: https://github.com/xamarin/java.interop/issues/826 [As noted][0] in [PR #827][1], there is some "weirdness" with what appears in the [`InnerClasses` collection][2]. It turns out this is due to a misunderstanding of what the `InnerClasses` collection contains. As per the [docs][2]]: > If a class has members that are classes or interfaces, its > `constant_pool` table (and hence its `InnerClasses` attribute) must > refer to each such member, even if that member is not otherwise > mentioned by the class. > **These rules imply that a nested class or interface member will > have `InnerClasses` information for each enclosing class and for > each immediate member.** (Emphasis added.) That is, the `PagedList$Config$Builder$Companion` class lists *both* its immediate containing type `PagedList$Config$Builder` and the "parent-parent" containing type `PagedList$Config` within `InnerClasses`. The change made in commit 4ef50813 loops through `InnerClasses`, marking them as `internal`, assuming they are all nested types. This is causing us to hide *declaring* types when we are trying to hide *nested* types. This will require more investigation and the deadline for 16.11 is ~now, so we're just going to revert the original commit. [0]: https://github.com/xamarin/java.interop/pull/827#discussion_r620729731 [1]: https://github.com/xamarin/java.interop/pull/827 [2]: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.6 --- .../Kotlin/KotlinFixups.cs | 30 ++++-------------- .../KotlinFixupsTests.cs | 19 ----------- ...estedInterface$DoubleNestedInterface.class | Bin 612 -> 0 bytes ...sWithNestedInterface$NestedInterface.class | Bin 599 -> 0 bytes .../InternalClassWithNestedInterface.class | Bin 627 -> 0 bytes .../InternalClassWithNestedInterface.kt | 5 --- 6 files changed, 6 insertions(+), 48 deletions(-) delete mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface$NestedInterface$DoubleNestedInterface.class delete mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface$NestedInterface.class delete mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface.class delete mode 100644 tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface.kt diff --git a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs index 60c4b8fa4..3ae27ebf2 100644 --- a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs +++ b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs @@ -30,7 +30,7 @@ public static void Fixup (IList classes) var class_metadata = (metadata as KotlinClass); if (class_metadata != null) { - FixupClassVisibility (c, class_metadata, classes); + FixupClassVisibility (c, class_metadata); if (!c.AccessFlags.IsPubliclyVisible ()) continue; @@ -62,7 +62,7 @@ public static void Fixup (IList classes) } } - static void FixupClassVisibility (ClassFile klass, KotlinClass metadata, IList classes) + static void FixupClassVisibility (ClassFile klass, KotlinClass metadata) { // Hide class if it isn't Public/Protected if (klass.AccessFlags.IsPubliclyVisible () && !metadata.Visibility.IsPubliclyVisible ()) { @@ -83,33 +83,15 @@ static void FixupClassVisibility (ClassFile klass, KotlinClass metadata, IList classes) - { - var existing = klass.InnerClassAccessFlags; - - klass.InnerClassAccessFlags = SetVisibility (existing, ClassAccessFlags.Private); - Log.Debug ($"Kotlin: Hiding nested internal type {klass.InnerClass.Name.Value}"); - - // Setting the inner class access flags above doesn't technically do anything, because we output - // from the inner class's ClassFile, so we need to find that and set it there. - var kf = classes.FirstOrDefault (c => c.ThisClass.Name.Value == klass.InnerClass.Name.Value); - - if (kf != null) { - kf.AccessFlags = SetVisibility (kf.AccessFlags, ClassAccessFlags.Private); - kf.InnerClass.InnerClassAccessFlags = SetVisibility (kf.InnerClass.InnerClassAccessFlags, ClassAccessFlags.Private); - - foreach (var inner_class in kf.InnerClasses.Where (c => c.OuterClass.Name.Value == kf.ThisClass.Name.Value)) - HideInternalInnerClass (inner_class, classes); - } - } - // Passing null for 'newVisibility' parameter means 'package-private' static ClassAccessFlags SetVisibility (ClassAccessFlags existing, ClassAccessFlags? newVisibility) { diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs b/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs index d8d7396dd..39841abe2 100644 --- a/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs +++ b/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs @@ -310,24 +310,5 @@ public void HandleKotlinNameShadowing () Assert.True (klass.Methods.Single (m => m.Name == "hitCount").AccessFlags.HasFlag (MethodAccessFlags.Public)); Assert.True (klass.Methods.Single (m => m.Name == "setType").AccessFlags.HasFlag (MethodAccessFlags.Public)); } - - [Test] - public void HidePublicInterfaceInInternalClass () - { - var klass = LoadClassFile ("InternalClassWithNestedInterface.class"); - var inner_interface = LoadClassFile ("InternalClassWithNestedInterface$NestedInterface.class"); - var inner_inner_interface = LoadClassFile ("InternalClassWithNestedInterface$NestedInterface$DoubleNestedInterface.class"); - - KotlinFixups.Fixup (new [] { klass, inner_interface, inner_inner_interface }); - - var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString (); - Assert.True (output.Contains ("visibility=\"public\"")); - - var output2 = new XmlClassDeclarationBuilder (inner_interface).ToXElement ().ToString (); - Assert.True (output2.Contains ("visibility=\"private\"")); - - var output3 = new XmlClassDeclarationBuilder (inner_inner_interface).ToXElement ().ToString (); - Assert.True (output3.Contains ("visibility=\"private\"")); - } } } diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface$NestedInterface$DoubleNestedInterface.class b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface$NestedInterface$DoubleNestedInterface.class deleted file mode 100644 index e27a40add46a30f2db2aa7a4511c39b462b1b2da..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 612 zcmb_ZO-lnY5Pg%?Zo7W;L#wTN5cKTU9`z(jaiJH<}ysDdzDxtV!BH?6J z-PzC)HX~!yI9r&qgy1QijC-o36Gf;$PK?td^{lPtHC-8#I_Y$3Y>s`y8auo=9_kz` z%n0mYH&@`ehkecm%nQ_T$oYbeS?8PUz>DB#FOR|x3L_AwQEXv;s C>Za8I diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface$NestedInterface.class b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface$NestedInterface.class deleted file mode 100644 index ad2e8205be74411cfa025c2d6fca7fafcf39dacd..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 599 zcmb7B%T59@6g_uF85kct6a{f%B5S}|YFro%$t0+WCc0o%plD&}B($*RyIdI7ew6Xf zByP}z(8s+!r@ha9eSdrc2yjYhh1$xg7SU}ajCob|{XrTl2Pr)ieOX_@9*9sFiMeP- zLccX1dn4Jm9$Z4PGmdSfbn{+XF%VW<5uC}~KCglI* zaK6s2F}8%=P-{6&KS&ya{~XVzec4u#BvhYf+Nz0sQATlV_uen{@JTqOmA-r^Dk^4n{h#gJ_9`?E*gMH-r%AjCS HG${Q5M&qDU diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface.class b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalClassWithNestedInterface.class deleted file mode 100644 index acfd9bfce1fcf6d68727927bb70e04fd6759a524..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 627 zcmaix&rTaL5XQgpCY!L`k^pT9Q2w`4%b|fB3qqwxNRa|afWjdsCoy2M@k(olbDpJd z&{L%zs;I|46zX^d7cQ}^Z~gmrW^9lDJ^ua!pn*-oM$(%Nh7?#`5Ts-N38 zvZqQQLiJug$a*5pZT-A^uX?V8fDj&Nqun7P`1H9Qqlhp<0VP7g-DykMYE3J?Pnc^B zQkQ5`KUGflrIYN5;X}X&Brg@lZjSKC+tPro{vP4uWE@ALCL&Rtj3ZGN<;l1<88^f( zeWl=g@$ZjNRD}<25LZLx#nlMA(+>FIMM?=Z!uGVRF9XL-I8OVDH*IO7&PKzo%C2NL zVQ0RT_GHqQnfCl-#!X{XmOp~x)