[pigeon] Fixes support for Kotlin/Java classes that override equals and hashCode for ProxyApis

[bparrishMines]

ProxyApis for classes that override equals()/hashcode in Kotlin/Java were breaking the InstanceManager.

The Kotlin InstanceManager would:

1. Return true for containsInstance.
2. Then remove the strong reference to the original object in getIdentifierForStrongReference.
3. Erroneously trigger the call to Dart to remove the strong reference of an instance.

This changes the the use of WeakReference to a new IdentityWeakReference that uses identity to check for equality. Java/Kotlin has an IdentityHashMap and a WeakHashMap, but not an IdentityWeakHashMap. So this is a quick workaround to create an equivalent.

Alternatively, getIdentifierForStrongReference could be updated to check containsInstance first, but this would leave small timeframes where objects would only have a weak reference. And it seems possible that Kotlin GC could cleanup at any moment.

Potential fix for flutter/flutter#174134 since the class they were creating a ProxyApi for was a data class: https://github.com/JavesonYehudi/proxy_bug_report/blob/main/android/src/main/kotlin/com/example/proxy_bug_report/auth/Credentials.kt

Pre-Review Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[bparrishMines]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively addresses a bug in the Kotlin InstanceManager where classes overriding equals() and hashCode() could cause incorrect instance tracking. The introduction of the IdentityKey wrapper to enforce identity-based comparisons in the underlying WeakHashMap is a clever solution to the lack of a built-in IdentityWeakHashMap. The addition of new tests specifically for this scenario is also a great way to ensure the fix is robust. Furthermore, adding logging for failures during Dart proxy instance creation improves debuggability. I've found one critical issue in the implementation of the new IdentityKey class that should be addressed.

[gemini-code-assist]

The hashCode implementation for IdentityKey has a critical bug and a logical inconsistency.

1. NPE Bug: If the WeakReference has been cleared by the garbage collector, instance.get() will return null. Calling .hashCode() on null will result in a NullPointerException. The equals method correctly handles this case, but hashCode does not.
2. Logical Inconsistency: The purpose of IdentityKey is to enforce identity-based equality, similar to IdentityHashMap. Therefore, its hash code should also be based on identity. Using the instance's own hashCode() can lead to unnecessary hash collisions if different instances have the same value-based hash code, and it deviates from the principle of identity-based hashing.

The correct approach is to use System.identityHashCode(), which is safe for null inputs (it returns 0) and provides an identity-based hash code.

Suggested change

	override fun hashCode(): Int {
	return instance.get().hashCode()
	}
	override fun hashCode(): Int {
	return System.identityHashCode(instance.get())
	}

[bparrishMines]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a critical bug in the Kotlin/Java InstanceManager for ProxyApis where classes overriding equals() and hashCode() could cause incorrect instance tracking. The fix introduces an IdentityWeakReference wrapper to ensure instance management is based on object identity rather than equality, which is a robust solution to the problem. The changes are well-tested with new unit tests covering the identity comparison logic and edge cases related to garbage collection. Additionally, the PR improves diagnostics by adding logging for failed Dart proxy instance creations. The overall implementation is solid. I have one suggestion to reduce code duplication in the generated code for the new logging feature.

[gemini-code-assist]

While this logging is a great addition for debugging, the current implementation in the generator leads to duplicated code blocks in the generated writeValue method for each ProxyApi type. To improve the maintainability and readability of the generated code, consider refactoring this. You could generate a private helper method within the ProxyApiBaseCodec class to encapsulate the logging logic, and then call that helper from each if block.

For example, you could generate a method like this:

private fun logNewInstanceFailure(value: Any, apiName: String, result: Result<*>) {
  if (result.isFailure) {
    Log.w(
      "PigeonProxyApiBaseCodec",
      "Failed to create new Dart proxy instance of $apiName: $value. ${result.exceptionOrNull()}"
    )
  }
}

Then, the call site in the if block would be a single, clean line.