Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Xamarin.Android.Tools.Bytecode] Only hide getters/setters for internal properties. #1151

Merged
merged 1 commit into from
Oct 26, 2023
Merged

[Xamarin.Android.Tools.Bytecode] Only hide getters/setters for internal properties. #1151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinClassMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to verify/validate this is redonkulous:

Fortunately, instead of trying to make sense of their code, I can just have the Java compiler tell me:

import org.jetbrains.kotlin.metadata.deserialization.Flags;

class DumpKotlinFlags {
    public static void main(String[] args) {
        System.out.println("IS_EXTERNAL_PROPERTY=" + Flags.IS_EXTERNAL_PROPERTY.invert(0));
    }
}
% javac -cp ~/Downloads/kotlinc/lib/kotlin-compiler.jar dump-kotlin-flags.java
% java -cp ~/Downloads/kotlinc/lib/kotlin-compiler.jar:. DumpKotlinFlags
IS_EXTERNAL_PROPERTY=16384

16384 is 0x4000 which is 0b_0100_0000_0000_0000, which is 0b_001000000_00_00_000_0. Things match!

IsDelegated = 0b_010000000_00_00_000_0,
IsExpectProperty = 0b_100000000_00_00_000_0
}

[Flags]
Expand Down
36 changes: 28 additions & 8 deletions src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
Expand Down Expand Up @@ -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));
Expand All @@ -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 ();
}
Expand Down
13 changes: 13 additions & 0 deletions src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels fragile and prone to bite us, e.g. Capitalize("ioRequest")=="IoRequest".

I half wonder if looking for case- insensitive checks elsewhere would be "better", as with this approach we're "guessing" at the intended capitalization, at the cost of not allowing "case-differing properties".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this feels fragile, but it does seem to be what Kotlin does:

// Kotlin
internal val Capital1 = 0
internal val capital2 = 0
internal val cAPitAL3 = 0

// Generated Java
private final int Capital1;  
private final int capital2;  
private final int cAPitAL3;

public final int getCapital1$main() {
  return this.Capital1;
}
  
public final int getCapital2$main() {
  return this.capital2;
}
  
public final int getCAPitAL3$main() {
  return this.cAPitAL3;
}

"case-differing properties" do seem to be allowed:

// Kotlin
internal val Foo = 0
internal val FOO = 0

// Generated Java
private final int Foo;
private final int FOO;

public final int getFoo$main() {
  return this.Foo;
}
  
public final int getFOO$main() {
  return this.FOO;
}

The "good" news is that it does guard against case-differing property names that would create a clash:

// Kotlin
internal val foo = 0
internal val Foo = 0

error G6DDBDC0F: platform declaration clash: The following declarations have the same JVM signature (getFoo$main()I):
    fun `<get-Foo>`(): Int defined in NameShadowing
    fun `<get-foo>`(): Int defined in NameShadowing
  internal val foo = 0

}

public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null, bool convertUnsignedToPrimitive = true)
{
if (type is null)
Expand Down
22 changes: 22 additions & 0 deletions tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Binary file modified tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.class
View file
Open in desktop
Binary file not shown.
20 changes: 18 additions & 2 deletions tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) { }
}