[pigeon] Fixes support for Kotlin/Java classes that override equals and hashCode for ProxyApis #10039
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,11 +75,39 @@ class ProxyApiTestsPigeonInstanceManager( | |
| fun onFinalize(identifier: Long) | ||
| } | ||
|
|
||
| // Extends WeakReference and overrides the `equals` and `hashCode` methods using identity rather | ||
| // than equality. | ||
| // | ||
| // Two `IdentityWeakReference`s are equal if either | ||
| // 1: `get()` returns the identical nonnull value for both references. | ||
| // 2: `get()` returns null for both references and the references are identical. | ||
| class IdentityWeakReference<T : Any> : java.lang.ref.WeakReference<T> { | ||
| private val savedHashCode: Int | ||
|
|
||
| constructor(instance: T) : this(instance, null) | ||
|
|
||
| constructor(instance: T, queue: java.lang.ref.ReferenceQueue<T>?) : super(instance, queue) { | ||
| savedHashCode = System.identityHashCode(instance) | ||
| } | ||
|
|
||
| override fun equals(other: Any?): Boolean { | ||
| val instance = get() | ||
| if (instance != null) { | ||
| return other is IdentityWeakReference<*> && other.get() === instance | ||
| } | ||
| return other === this | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| return savedHashCode | ||
| } | ||
| } | ||
|
|
||
| private val identifiers = java.util.WeakHashMap<IdentityWeakReference<Any>, Long>() | ||
| private val weakInstances = HashMap<Long, IdentityWeakReference<Any>>() | ||
| private val strongInstances = HashMap<Long, Any>() | ||
| private val referenceQueue = java.lang.ref.ReferenceQueue<Any>() | ||
| private val weakReferencesToIdentifiers = HashMap<IdentityWeakReference<Any>, Long>() | ||
| private val handler = android.os.Handler(android.os.Looper.getMainLooper()) | ||
| private val releaseAllFinalizedInstancesRunnable = Runnable { | ||
| this.releaseAllFinalizedInstances() | ||
|
|
@@ -144,9 +172,12 @@ class ProxyApiTestsPigeonInstanceManager( | |
| */ | ||
| fun getIdentifierForStrongReference(instance: Any?): Long? { | ||
| logWarningIfFinalizationListenerHasStopped() | ||
| if (instance == null) { | ||
| return null | ||
| } | ||
| val identifier = identifiers[IdentityWeakReference(instance)] | ||
| if (identifier != null) { | ||
| strongInstances[identifier] = instance | ||
| } | ||
| return identifier | ||
| } | ||
|
|
@@ -185,14 +216,14 @@ class ProxyApiTestsPigeonInstanceManager( | |
| /** Retrieves the instance associated with identifier, if present, otherwise `null`. */ | ||
| fun <T> getInstance(identifier: Long): T? { | ||
| logWarningIfFinalizationListenerHasStopped() | ||
| val instance = weakInstances[identifier] as IdentityWeakReference<T>? | ||
| return instance?.get() | ||
| } | ||
|
|
||
| /** Returns whether this manager contains the given `instance`. */ | ||
| fun containsInstance(instance: Any?): Boolean { | ||
| logWarningIfFinalizationListenerHasStopped() | ||
| return instance != null && identifiers.containsKey(IdentityWeakReference(instance)) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -233,9 +264,8 @@ class ProxyApiTestsPigeonInstanceManager( | |
| if (hasFinalizationListenerStopped()) { | ||
| return | ||
| } | ||
| var reference: IdentityWeakReference<Any>? | ||
| while ((referenceQueue.poll() as IdentityWeakReference<Any>?).also { reference = it } != null) { | ||
| val identifier = weakReferencesToIdentifiers.remove(reference) | ||
| if (identifier != null) { | ||
| weakInstances.remove(identifier) | ||
|
|
@@ -251,8 +281,8 @@ class ProxyApiTestsPigeonInstanceManager( | |
| require(!weakInstances.containsKey(identifier)) { | ||
| "Identifier has already been added: $identifier" | ||
| } | ||
| val weakReference = IdentityWeakReference(instance, referenceQueue) | ||
| identifiers[weakReference] = identifier | ||
| weakInstances[identifier] = weakReference | ||
| weakReferencesToIdentifiers[weakReference] = identifier | ||
| strongInstances[identifier] = instance | ||
|
|
@@ -460,14 +490,36 @@ private class ProxyApiTestsPigeonProxyApiBaseCodec( | |
| return | ||
| } | ||
|
|
||
| fun logNewInstanceFailure(apiName: String, value: Any, exception: Throwable?) { | ||
| Log.w( | ||
| "PigeonProxyApiBaseCodec", | ||
| "Failed to create new Dart proxy instance of $apiName: $value. $exception") | ||
| } | ||
|
|
||
| if (value is ProxyApiTestClass) { | ||
| registrar.getPigeonApiProxyApiTestClass().pigeon_newInstance(value) { | ||
| if (it.isFailure) { | ||
| logNewInstanceFailure("ProxyApiTestClass", value, it.exceptionOrNull()) | ||
| } | ||
| } | ||
| } else if (value is com.example.test_plugin.ProxyApiSuperClass) { | ||
| registrar.getPigeonApiProxyApiSuperClass().pigeon_newInstance(value) { | ||
| if (it.isFailure) { | ||
| logNewInstanceFailure("ProxyApiSuperClass", value, it.exceptionOrNull()) | ||
| } | ||
| } | ||
| } else if (value is ProxyApiInterface) { | ||
| registrar.getPigeonApiProxyApiInterface().pigeon_newInstance(value) { | ||
| if (it.isFailure) { | ||
| logNewInstanceFailure("ProxyApiInterface", value, it.exceptionOrNull()) | ||
| } | ||
| } | ||
| } else if (android.os.Build.VERSION.SDK_INT >= 25 && value is ClassWithApiRequirement) { | ||
| registrar.getPigeonApiClassWithApiRequirement().pigeon_newInstance(value) { | ||
| if (it.isFailure) { | ||
| logNewInstanceFailure("ClassWithApiRequirement", value, it.exceptionOrNull()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,10 +149,97 @@ class InstanceManagerTest { | |
| assertFalse(finalizerRan) | ||
| } | ||
|
|
||
| @Test | ||
| fun containsInstanceAndGetIdentifierForStrongReferenceUseIdentityComparison() { | ||
| val instanceManager: ProxyApiTestsPigeonInstanceManager = createInstanceManager() | ||
| instanceManager.stopFinalizationListener() | ||
|
|
||
| // Create two objects that are equal. | ||
| val testString = "aString" | ||
| val testObject1 = TestDataClass(testString) | ||
| val testObject2 = TestDataClass(testString) | ||
| assertEquals(testObject1, testObject2) | ||
|
|
||
| val identifier1 = instanceManager.addHostCreatedInstance(testObject1) | ||
| assertFalse(instanceManager.containsInstance(testObject2)) | ||
| assertNull(instanceManager.getIdentifierForStrongReference(testObject2)) | ||
|
|
||
| val identifier2 = instanceManager.addHostCreatedInstance(testObject2) | ||
| assertTrue(instanceManager.containsInstance(testObject1)) | ||
| assertTrue(instanceManager.containsInstance(testObject2)) | ||
| assertEquals(identifier1, instanceManager.getIdentifierForStrongReference(testObject1)) | ||
| assertEquals(identifier2, instanceManager.getIdentifierForStrongReference(testObject2)) | ||
| } | ||
|
|
||
| @Test | ||
| fun addingTwoDartCreatedInstancesThatAreEqual() { | ||
| val instanceManager: ProxyApiTestsPigeonInstanceManager = createInstanceManager() | ||
| instanceManager.stopFinalizationListener() | ||
|
|
||
| // Create two objects that are equal. | ||
| val testString = "aString" | ||
| val testObject1 = TestDataClass(testString) | ||
| val testObject2 = TestDataClass(testString) | ||
| assertEquals(testObject1, testObject2) | ||
|
|
||
| instanceManager.addDartCreatedInstance(testObject1, 0) | ||
| instanceManager.addDartCreatedInstance(testObject2, 1) | ||
|
|
||
| assertEquals(testObject1, instanceManager.getInstance(0)) | ||
| assertEquals(testObject2, instanceManager.getInstance(1)) | ||
| assertEquals(0L, instanceManager.getIdentifierForStrongReference(testObject1)) | ||
| assertEquals(1L, instanceManager.getIdentifierForStrongReference(testObject2)) | ||
| } | ||
|
|
||
| @Test | ||
| fun identityWeakReferencesAreEqualWithSameInstance() { | ||
| val testObject = Any() | ||
|
|
||
| assertEquals( | ||
| ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject), | ||
| ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject)) | ||
| } | ||
|
|
||
| @Test | ||
| fun identityWeakReferenceRemainsEqualAfterGetReturnsNull() { | ||
| var testObject: Any? = Any() | ||
|
|
||
| val reference = ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject!!) | ||
|
|
||
| // To allow for object to be garbage collected. | ||
| @Suppress("UNUSED_VALUE") | ||
| testObject = null | ||
| Runtime.getRuntime().gc() | ||
|
|
||
| assertNull(reference.get()) | ||
| assertEquals(reference, reference) | ||
| } | ||
|
|
||
| @Test | ||
| fun identityWeakReferencesAreNotEqualAfterGetReturnsNull() { | ||
| var testObject1: Any? = Any() | ||
| var testObject2: Any? = Any() | ||
|
|
||
| val reference1 = ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject1!!) | ||
| val reference2 = ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject2!!) | ||
|
|
||
| // To allow for object to be garbage collected. | ||
| @Suppress("UNUSED_VALUE") | ||
| testObject1 = null | ||
| testObject2 = null | ||
| Runtime.getRuntime().gc() | ||
|
|
||
| assertNull(reference1.get()) | ||
| assertNull(reference2.get()) | ||
| assertFalse(reference1 == reference2) | ||
| } | ||
|
|
||
| private fun createInstanceManager(): ProxyApiTestsPigeonInstanceManager { | ||
| return ProxyApiTestsPigeonInstanceManager.create( | ||
| object : ProxyApiTestsPigeonInstanceManager.PigeonFinalizationListener { | ||
| override fun onFinalize(identifier: Long) {} | ||
| }) | ||
| } | ||
|
|
||
| data class TestDataClass(val value: String) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ name: pigeon | |
| description: Code generator tool to make communication between Flutter and the host platform type-safe and easier. | ||
| repository: https://github.com/flutter/packages/tree/main/packages/pigeon | ||
| issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+pigeon%22 | ||
| version: 26.0.2 # This must match the version in lib/src/generator_tools.dart | ||
|
|
||
| environment: | ||
| sdk: ^3.7.0 | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this logging is a great addition for debugging, the current implementation in the generator leads to duplicated code blocks in the generated
writeValuemethod for eachProxyApitype. To improve the maintainability and readability of the generated code, consider refactoring this. You could generate a private helper method within theProxyApiBaseCodecclass to encapsulate the logging logic, and then call that helper from eachifblock.For example, you could generate a method like this:
Then, the call site in the
ifblock would be a single, clean line.