From 49312510026a37e9947aaca5e37ecc3077f13c4c Mon Sep 17 00:00:00 2001 From: Jonathan Pobst Date: Tue, 17 Oct 2023 07:25:29 -1000 Subject: [PATCH] [Xamarin.Android.Tools.Bytecode] Only hide getters/setters for internal properties. --- .../Kotlin/KotlinClassMetadata.cs | 24 +++++++----- .../Kotlin/KotlinFixups.cs | 36 ++++++++++++++---- .../Kotlin/KotlinUtilities.cs | 13 +++++++ .../KotlinFixupsTests.cs | 22 +++++++++++ .../kotlin/NameShadowing.class | Bin 1238 -> 1936 bytes .../kotlin/NameShadowing.kt | 20 +++++++++- 6 files changed, 96 insertions(+), 19 deletions(-) diff --git a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinClassMetadata.cs b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinClassMetadata.cs index d983df5c4..de5a5513e 100644 --- a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinClassMetadata.cs +++ b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinClassMetadata.cs @@ -318,6 +318,10 @@ public class KotlinProperty : KotlinMethodBase } public override string? ToString () => Name; + + public bool IsInternalVisibility => (Flags & KotlinPropertyFlags.VisibilityMask) == KotlinPropertyFlags.Internal; + + public bool IsPrivateVisibility => (Flags & KotlinPropertyFlags.VisibilityMask) == KotlinPropertyFlags.Private; } public class KotlinType @@ -688,6 +692,8 @@ public enum KotlinPropertyFlags PrivateToThis = 0b00_00_100_0, Local = 0b00_00_101_0, + VisibilityMask = 0b00_00_111_0, + Final = 0b00_00_000_0, Open = 0b00_01_000_0, Abstract = 0b00_10_000_0, @@ -698,15 +704,15 @@ public enum KotlinPropertyFlags Delegation = 0b10_00_000_0, Synthesized = 0b11_00_000_0, - IsVar = 0b_000000001_000_00_000_0, - HasGetter = 0b_000000010_000_00_000_0, - HasSetter = 0b_000000100_000_00_000_0, - IsConst = 0b_000001000_000_00_000_0, - IsLateInit = 0b_000010000_000_00_000_0, - HasConstant = 0b_000100000_000_00_000_0, - IsExternalProperty = 0b_001000000_000_00_000_0, - IsDelegated = 0b_010000000_000_00_000_0, - IsExpectProperty = 0b_100000000_000_00_000_0 + IsVar = 0b_000000001_00_00_000_0, + HasGetter = 0b_000000010_00_00_000_0, + HasSetter = 0b_000000100_00_00_000_0, + IsConst = 0b_000001000_00_00_000_0, + IsLateInit = 0b_000010000_00_00_000_0, + HasConstant = 0b_000100000_00_00_000_0, + IsExternalProperty = 0b_001000000_00_00_000_0, + IsDelegated = 0b_010000000_00_00_000_0, + IsExpectProperty = 0b_100000000_00_00_000_0 } [Flags] diff --git a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs index 0ad357d84..94f1435ad 100644 --- a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs +++ b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs @@ -272,8 +272,8 @@ static void FixupProperty (MethodInfo? getter, MethodInfo? setter, KotlinPropert if (getter is null && setter is null) return; - // Hide property if it isn't Public/Protected - if (!metadata.Flags.IsPubliclyVisible ()) { + // Hide getters/setters if property is Internal + if (metadata.IsInternalVisibility) { if (getter?.IsPubliclyVisible == true) { Log.Debug ($"Kotlin: Hiding internal getter method {getter.DeclaringType?.ThisClass.Name.Value} - {getter.Name}"); @@ -384,7 +384,17 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata) static MethodInfo? FindJavaPropertyGetter (KotlinFile kotlinClass, KotlinProperty property, ClassFile klass) { - var possible_methods = klass.Methods.Where (method => string.Compare (method.GetMethodNameWithoutSuffix (), $"get{property.Name}", StringComparison.OrdinalIgnoreCase) == 0 && + // Private properties do not have getters + if (property.IsPrivateVisibility) + return null; + + // Public/protected getters look like "getFoo" + // Internal getters look like "getFoo$main" + var possible_methods = property.IsInternalVisibility ? + klass.Methods.Where (method => method.Name.StartsWith ($"get{property.Name.Capitalize ()}$", StringComparison.Ordinal)) : + klass.Methods.Where (method => method.Name.Equals ($"get{property.Name.Capitalize ()}", StringComparison.Ordinal)); + + possible_methods = possible_methods.Where (method => method.GetParameters ().Length == 0 && property.ReturnType != null && TypesMatch (method.ReturnType, property.ReturnType, kotlinClass)); @@ -394,11 +404,21 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata) static MethodInfo? FindJavaPropertySetter (KotlinFile kotlinClass, KotlinProperty property, ClassFile klass) { - var possible_methods = klass.Methods.Where (method => string.Compare (method.GetMethodNameWithoutSuffix (), $"set{property.Name}", StringComparison.OrdinalIgnoreCase) == 0 && - property.ReturnType != null && - method.GetParameters ().Length == 1 && - method.ReturnType.BinaryName == "V" && - TypesMatch (method.GetParameters () [0].Type, property.ReturnType, kotlinClass)); + // Private properties do not have setters + if (property.IsPrivateVisibility) + return null; + + // Public/protected setters look like "setFoo" + // Internal setters look like "setFoo$main" + var possible_methods = property.IsInternalVisibility ? + klass.Methods.Where (method => method.Name.StartsWith ($"set{property.Name.Capitalize ()}$", StringComparison.Ordinal)) : + klass.Methods.Where (method => method.Name.Equals ($"set{property.Name.Capitalize ()}", StringComparison.Ordinal)); + + possible_methods = possible_methods.Where (method => + property.ReturnType != null && + method.GetParameters ().Length == 1 && + method.ReturnType.BinaryName == "V" && + TypesMatch (method.GetParameters () [0].Type, property.ReturnType, kotlinClass)); return possible_methods.FirstOrDefault (); } diff --git a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs index f177db989..b9312c40e 100644 --- a/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs +++ b/src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Text; using System.Threading.Tasks; @@ -8,6 +9,18 @@ namespace Xamarin.Android.Tools.Bytecode { public static class KotlinUtilities { + [return: NotNullIfNotNull (nameof (value))] + public static string? Capitalize (this string? value) + { + if (string.IsNullOrWhiteSpace (value)) + return value; + + if (value.Length < 1) + return value; + + return char.ToUpperInvariant (value [0]) + value.Substring (1); + } + public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null, bool convertUnsignedToPrimitive = true) { if (type is null) diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs b/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs index a393c6782..1108e081a 100644 --- a/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs +++ b/tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs @@ -324,7 +324,29 @@ public void HandleKotlinNameShadowing () Assert.True (klass.Methods.Single (m => m.Name == "count").AccessFlags.HasFlag (MethodAccessFlags.Public)); Assert.True (klass.Methods.Single (m => m.Name == "hitCount").AccessFlags.HasFlag (MethodAccessFlags.Public)); + + // Private property and explicit getter/setter + // There is no generated getter/setter, and explicit ones should be public + Assert.True (klass.Methods.Single (m => m.Name == "getType").AccessFlags.HasFlag (MethodAccessFlags.Public)); Assert.True (klass.Methods.Single (m => m.Name == "setType").AccessFlags.HasFlag (MethodAccessFlags.Public)); + + // Private immutable property and explicit getter/setter + // There is no generated getter/setter, and explicit ones should be public + Assert.True (klass.Methods.Single (m => m.Name == "getType2").AccessFlags.HasFlag (MethodAccessFlags.Public)); + Assert.True (klass.Methods.Single (m => m.Name == "setType2").AccessFlags.HasFlag (MethodAccessFlags.Public)); + + // Internal property and explicit getter/setter + // Generated getter/setter should not be public, and explicit ones should be public + Assert.False (klass.Methods.Single (m => m.Name == "getItype$main").AccessFlags.HasFlag (MethodAccessFlags.Public)); + Assert.False (klass.Methods.Single (m => m.Name == "setItype$main").AccessFlags.HasFlag (MethodAccessFlags.Public)); + Assert.True (klass.Methods.Single (m => m.Name == "getItype").AccessFlags.HasFlag (MethodAccessFlags.Public)); + Assert.True (klass.Methods.Single (m => m.Name == "setItype").AccessFlags.HasFlag (MethodAccessFlags.Public)); + + // Internal immutable property and explicit getter/setter + // Generated getter should not be public, and explicit ones should be public + Assert.False (klass.Methods.Single (m => m.Name == "getItype2$main").AccessFlags.HasFlag (MethodAccessFlags.Public)); + Assert.True (klass.Methods.Single (m => m.Name == "getItype2").AccessFlags.HasFlag (MethodAccessFlags.Public)); + Assert.True (klass.Methods.Single (m => m.Name == "setItype2").AccessFlags.HasFlag (MethodAccessFlags.Public)); } [Test] diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.class b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.class index 9b84408640d969ce6adc5eb3a23473e095eeb50b..989e48d095c56e792129b72c04ff12df99c6fdd6 100644 GIT binary patch literal 1936 zcma)6OHUI~6#nkColeK;bSMa=fGC1OeH7|jQR5>fHK+*~V!~>G35?LG;SN4_jX%Y; z8xxI*3qx4?qm19387elTxS8|%zH`n!_uTpM^V@d-mvKj+_|$EAOPlU``;8xLAZ^=GRuy|plpjDSAx2YyIKW_o7XLJqo#Eb;=mwf1fh3W%nK0?Yz5923Zf zo4yn%E&P%HRzzj^e zfg2X8F?xi-1kGTPxTXS3wp>3T*pE|>$KaDh5PTSspQq!(U7l3ch$3;uM70mRjU=TW zrDOwV~&1dP>MlWqNHwQDtn!VHY@Ju_CM%!J=UE}Rh!LHmJ;bCvM-V4 zMCKxCJN=Xc4c#dw_VFHju*V)cY%khzy%+oMAF2+_y$)SbYP#$LSJlx~_80pd<+6#Y zW{tdKOk|CMV=fLj$C9v0V#Hy~F_ToLCv14qvpc)mY%mF%otlh;e?q~Kvd!4IQ zTJQt!>27P)+j;J;Zqrjt1{g$`vOP`Q?LC_A} z&~FD4b6ock};vHzQ=UAgq&R4Gp*Gvtl%g%meh%UrI97DOILi?K7;8RN^G65S@p}xh~mBC6{~2 zg;a8XDtWM%Tudbop^~(HCF+!S_xs#&%XUN@2zqx2I!-4;nEbychEkiD>NfG$ zOQ;+%gfpr2XL^S)l3Ks^f8^0r^0}j%ANyzX8sDCJB-e49QgWxx^O;;&L8gg|P1Kv1 z#T->Mnz)3^D-aS_aCHS030vZtgf97$SBfJsE^$JlBrzcs;H<pHvIhc?FWDY?lB}PqHXT%i-!Bga+(n^ z2E8SE!mx$YG@jL3X5EiKVF=aTuH!SXl0sZcD9bCQI6?@=z!76m{e8=0NS04kTxW>x zTYgcJ<46qA6B7OQwaF0iOn*1<7MDva`@r zRh~PRzrIPN#j@LJ8ZFbWb%aF)h2yxs7#6R%ex+;Mgqk{~dub$WX)S^a4B-uns%?^P z5>Sm}3Rw*qOfxL}TXO`n!Kebxyap8u45_iir=~9&!WZ;<-tNi4GI=B^tOX%x;0F{n z))?Lo`g0o3a*gYQzQ&`{iIP=DNuC|_3p~#rg^oUixz3-2b!CRFE)(`WFvtw8yI4ZKUN|Q)9O% z;)u|f7B}$qz^X_QB1(u=${O;L$cA!XpnVKr2A#41C6T8yKJw3z2O=lVBq#qQ$Ic|D zNS_kC`2krj%YR4a0xJ!1M@|S2h xm-nEQa3%OGVrd**Ez#7kQ5M(tz&tEtWe=K%xQCpFc@Lq$NqEpbq&(c9@HY*(>}&u4 diff --git a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.kt b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.kt index 6c05ca9b4..5c00497fc 100644 --- a/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.kt +++ b/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.kt @@ -8,7 +8,23 @@ public class NameShadowing { private var hitCount = 0 fun hitCount(): Int = hitCount - // Property and setter + // Private property and explicit getter/setter private var type = 0 - fun setType(type: Int) = { println (type); } + fun getType(): Int = type + fun setType(type: Int) { } + + // Private immutable property and explicit getter/setter + private val type2 = 0 + fun getType2(): Int = type2 + fun setType2(type: Int) { } + + // Internal property and explicit getter/setter + internal var itype = 0 + fun getItype(): Int = itype + fun setItype(type: Int) { } + + // Internal immutable property and explicit getter/setter + internal val itype2 = 0 + fun getItype2(): Int = itype2 + fun setItype2(type: Int) { } } \ No newline at end of file