Let's add a:

// Must not return nil unless "error" is set.

here, so that we have an explicit flag at the implementation point.

done

Pigeon note: I think there's a potential red flag here that we've created over two hundred lines of generated code to make an 80-line implementation file 10 lines longer.

Strong typing is definitely good, but the tradeoff here is getting to be somewhat questionable. I think it's worth thinking about ways to make pigeon feel lighter weight for simple plugins.

I don't think lines of code is the correct way to measure the change because of objc formatting and the need to distinguish between compile time verified lines and runtime verified lines. Here's instead how the changes should be measured:

1. Every method went from ~6 operations to ~2.
2. Every method removed 2 casts
3. Conformance to the API is enforced at compile time
4. Error handling has been made available to the objc handlers

Another thing to note is that xcode generated most of this code so it didn't feel heavy weight when writing it. Once you implement a protocol xcode offers to fill it in.

Why is Pigeon annotating this as nullable? It's non-nullable in the Dart specification; that seems like a significant Pigeon bug. (Or is this stale?)

It's not stale. It was probably done because of error handling. If there is an error you don't want people to artificially return some value.

Yea, reading through the code, that's what it's for. If you return nil and set an error, it will throw a PlatformException on the dart side. That's what we want. If you return nil and don't set an error, you'll get an exception when it is cast to a non-null type before returning. I think that's what we want as well.

That means that it's currently impossible to distinguish in the context of the native code between a nullable and a non-nullable return value, which is definitely not great. We can't do anything compiler-enforceable I guess, which is unfortunate, but it seems like we should at least have pigeon put a comment on methods like this that says:

/// This may only return nil if 'error' is set.

so that there's clear information within the context of the generated native code what the required behavior is.

If you return nil and don't set an error, you'll get an exception when it is cast to a non-null type before returning. I think that's what we want as well.

It looks like that's only on the Dart side. Shouldn't we be validating in ObjC code that either error or the return value is non-nil, in the case of non-nullable return values? Otherwise mistakes will look like they are on the Dart side rather than in the native implementation.

Yea, those are improvements we can do when we implement nullable return types. I'll add a note to that issue: flutter/flutter#98452 (comment)

Shouldn't we be validating in ObjC code that either error or the return value is non-nil, in the case of non-nullable return values? Otherwise mistakes will look like they are on the Dart side rather than in the native implementation.

It doesn't necessarily have to happen on the objc side. The end result will look the same to a user, an exception on the Dart side. I think generating a PlatformException about a null value instead of a null value exception will be more clear though.

it seems like we should at least have pigeon put a comment on methods like this that says:

Yea, it's a good idea. I don't know how helpful it will be in practice because it won't show up where people are actually implementing the logic.

The end result will look the same to a user, an exception on the Dart side.

Right, I wasn't thinking of the end user, but the plugin developer(s). Especially if multiple platforms share method channels (which is still overwhelmingly the common case in the ecosystem). If the exception is in the Dart code, it points someone investigating in the wrong direction; probably not a huge effect, but anything that makes it harder for people to root-cause bugs isn't ideal.

Pigeon note: It would be nice to optimize the generator to not create these at all when there are no custom types.

Filed issue: flutter/flutter#97845

Pigeon note: Couldn't we reduce the amount of generated boilerplate code by making this whole block a helper function, and only generate the block and name each time? (And the assertion I guess).

static void SomeHelper(
    id<FlutterBinaryMessenger> binaryMessenger,
    NSObject<SharedPreferencesApi *api,
    NSString *channelName,
    <insert correct block syntax here> methodHandler) {
  FlutterBasicMessageChannel *channel = [FlutterBasicMessageChannel
      messageChannelWithName:channelName
             binaryMessenger:binaryMessenger
                       codec:SharedPreferencesApiGetCodec()];
  if (api) {
    [channel setMessageHandler:messageHandler];
  } else {
    [channel setMessageHandler:nil];
  }
}

Then each code block here becomes just:

  NSCAssert(!api || [api respondsToSelector:@selector(removeKey:error:)],
            @"SharedPreferencesApi api (%@) doesn't respond to @selector(removeKey:error:)",
            api);
  SomeHelper(binaryMessenger, api, @"dev.flutter.pigeon.SharedPreferencesApi.remove",
      ^(id _Nullable message, FlutterReply callback) {
        NSArray *args = message;
        NSString *arg_key = args[0];
        FlutterError *error;
        NSNumber *output = [api removeKey:arg_key error:&error];
        callback(wrapResult(output, error));
      });

It wouldn't even need the scoping block.

Yea, that could be something we look into. It isn't boilerplate since it is generated and can be treated as a black box by the user. Boilerplate to me means code a user has to manage that has semantic meaning that is small compared to its verbosity.