[pigeon] Add errors for ProxyAPI callback methods and null instances when reading in a ProxyApiBaseCodec

[bparrishMines]

• Adds an error message when a ProxyAPI callback method that returns a non-null value is nullable. Returning a null value causes error on the platform side. This ensures the Dart side prevents this from happening.

• Adds an error message in the ProxyApiBaseCodec when an instance could not be retrieved when reading a value.

Pre-launch 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. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• 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 this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[stuartmorgan]

I just noticed this being different from the other paths; is this a latent bug? Shouldn't it be callback(Result(success(Unit))?

[stuartmorgan]

Looking at the whole method: does this really need to take a callback at all? It seems like it could just directly return Result<Unit>, which would have prevented both of these bugs.

[bparrishMines]

It mainly takes a callback because the code that generates the MethodCall is reused from the FlutterApi generation. I think removing the callback would require me to write separate code for the MethodCall.

[stuartmorgan]

It's using _writeFlutterMethod, which is a thin wrapper around two functions, the declaration part of which is already parameterized as to whether or not it's async, so unless there are details I'm missing it should be as easy as just re-exposing isAsynchronous (as a default-true argument) in _writeFlutterMethod, and setting it to false for this call.

[bparrishMines]

_writeProxyApiNewInstanceMethod only uses _writeFlutterMethodMessageCall and not _writeFlutterMethod. And _writeFlutterMethodMessageCall always assumes that the Flutter method uses a completion. All methods in FlutterApis are async from the platform side.

[stuartmorgan]

I was looking at the Kotlin generator (I assumed the Swift generator had the same structure), where _writeProxyApiNewInstanceMethod is almost entirely a call to _writeFlutterMethod, so maybe this is easy for Kotlin but not Swift.

We can leave this for now though since the PR is still an improvement, even if it leaves the potential for a new version of this bug in the future.

[bparrishMines]

Whoops I didn't realize this comment was made on a different file than the one below. But it looks like the _writeFlutterMethod in the Kotlin generator contains onWriteBody, which gives the method more flexibility than the swift one. I think the main problem is that the method parameters for the method signature is different than the parameters passed in the MethodCall. Basically _writeProxyApiNewInstanceMethod does

void method(MyClass newInstance) {
  // Check if a new dart instance needs be created and return if not.

  // Retrieve all the field values needed by the Dart instance. This part is not included in a typical Flutter method.

  // use _writeFlutterMethodMessageCall with all the field values from above
}

There's probably a better way to do this and make it clearer that this is happening.

[stuartmorgan]

Same structural question for this whole method (I can't comment on the method declaration since it's outside of the change context): could this have a simple return instead of a completion, reducing the complexity?

[bparrishMines]

I changed the code to use a if...else if...else, so it no longer needs a return.

[stuartmorgan]

The issue isn't the return, it's that (as the Kotlin bug demonstrated) having a function use an async callback is inherently more error-prone, because the compiler will enforce that you return exactly once, but callbacks don't. Since we don't need any async behavior here, the method would be safer if it used a return value instead of a callback.

[bparrishMines]

Yes, but technically the method is making a async call when it makes the MethodCall to create the Dart instance. However, the case that the MethodCall is replaced by the error, the method probably could just return a Result. Were you suggesting that the method should return a Result when its not actually making a method call?

[stuartmorgan]

I'm not following where there's an async step in this method, looking at the code. The structure of pigeonNewInstance is:

func pigeonNewInstance(
    pigeonInstance: ProxyApiTestClass,
    completion: @escaping (Result<Void, ProxyApiTestsError>) -> Void
  ) {
    if pigeonRegistrar.ignoreCallsToDart {
      completion(...)
    } else if pigeonRegistrar.instanceManager.containsInstance(pigeonInstance as AnyObject) {
      completion(...)
    } else {
      completion(...)
    }
  }

In every branch, it's just synchronously calling completion. I don't see any reason (other than the logistics of the generator logic helper function discussed in the other thread) it couldn't be

pigeonNewInstance(pigeonInstance: ProxyApiTestClass) -> Result<Void, ProxyApiTestsError>

with every completion(...) replaced with a return ....

[bparrishMines]

For most classes, the final completion is done inside of a MethodCall:

packages/packages/pigeon/platform_tests/test_plugin/ios/Classes/ProxyApiTests.gen.swift

Line 3885 in 2c3c947

} else {

  } else {
      let pigeonIdentifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(
        pigeonInstance as AnyObject)
      let binaryMessenger = pigeonRegistrar.binaryMessenger
      let codec = pigeonRegistrar.codec
      let channelName: String =
        "dev.flutter.pigeon.pigeon_integration_tests.ProxyApiSuperClass.pigeon_newInstance"
      let channel = FlutterBasicMessageChannel(
        name: channelName, binaryMessenger: binaryMessenger, codec: codec)
      channel.sendMessage([pigeonIdentifierArg] as [Any?]) { response in
        guard let listResponse = response as? [Any?] else {
          completion(.failure(createConnectionError(withChannelName: channelName)))
          return
        }
        if listResponse.count > 1 {
          let code: String = listResponse[0] as! String
          let message: String? = nilOrValue(listResponse[1])
          let details: String? = nilOrValue(listResponse[2])
          completion(.failure(ProxyApiTestsError(code: code, message: message, details: details)))
        } else {
          completion(.success(()))
        }
      }
    }

Its based on whether the Dart class has required callback methods to be implemented. If yes, this final completion synchronously returns an error.

[stuartmorgan]

Oh, I see now. Sorry about that; I missed that there were two different forms of it.

[stuartmorgan]

LGTM with a nit about a stray return.

[stuartmorgan]

This return shouldn't be generated any more; with the change to the if/else if/else structure it's not needed, and it's inconsistent with the other branches.

[stuartmorgan]

I think this is corresponding generator line?

[tarrinneal]

tiny nit, optional really

[tarrinneal]

Pretty minor nit, but I think it's better to put the spaces before the else rather than after the } since the scoped statement doesn't need it, but the else statement does.

[tarrinneal]

here too

[tarrinneal]

here