[ffigen] Protocol polish

[liamappelbe]

Clean up the class object when the implementation object and the builder have been destroyed, by wrapping it in a DOBJCDartProtocolClass object that calls objc_disposeClassPair in its dispose method, and having the builder and instances hold a reference to this object.

Also, since we're not allowing adding more methods after building the protocol, we don't have to copy the method table during instantiation anymore.

Finally, re-add a bunch of other GC tests that I accidentally deleted.

Fixes #2042

[github-actions]

PR Health

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?
objective_c	Breaking	6.0.0	7.0.0-wip	7.0.0	✔️

API leaks ⚠️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbols
objective_c	_Version

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

[coveralls]

coverage: 88.252% (+0.2%) from 88.062%
when pulling 2ee487c on proto_polish
into 6cec710 on main.

[mraleph]

Do you have a specific reason to separate this from the builder itself?

Consider a simpler setup where DOBJCDartProtocol just holds DOBJCDartProtocolBuilder and also where we create class in Objective-C and not on the Dart side. Below is a rough sketch.

I also tried to follow Google Objective-C Style Guide which we otherwise seemed to have ignored. It is good to have more comments and somewhat cleaner code.

@class DOBJCDartProtocol;

/**
 * Class responsible for creating Objective-C classes in runtime.
 * See also `...` in Dart.
 */
@interface DOBJCDartProtocolBuilder : NSObject

/**
 * Initialize the builder using the given @c debugName.
 * 
 * @param debugName prefix to use when creating an Objective-C class,
 *   "DOBJCDartProtocol" will be used if @c debugName is @c nil
 */
- (instancetype)initWithDebugName: (NSString*)debugName;

/**
 * Use @c block to implement selector @c sel.
 * 
 * This method can only be called before the first instance is created
 * via @c newInstanceWithDisposePort.
 * 
 * @param sel Selector which will be implemented.
 * @param block Block to redirect method to.
 */
- (void)implementMethod:(SEL) sel withBlock:(void*)block;

/**
 * Create an instance of the class built by this builder.
 * 
 * Finalizes and registers Objective-C class. No new methods can be implemented
 * after the first instance is created.
 * 
 * @param port Optional port to notify when newly created instance is 
 *   deallocated. Can be @c ILLEGAL_PORT.
 */ 
- (DOBJCDartProtocol*)newInstanceWithDisposePort:(Dart_Port)port;

- (void) dealloc;
@end

@interface DOBJCDartProtocol : NSObject
- (instancetype)initFromBuilder: (DOBJCDartProtocolBuilder*)builder withDisposePort:(Dart_Port)port;

- (id)getBlockImplementingSelector:(SEL)sel;

- (void)dealloc;
@end

@implementation DOBJCDartProtocolBuilder {
  BOOL _finalized;
  Class _cls;
  @public NSMutableDictionary *_methods;
}

static _Atomic int64_t gNextProtocolId = 0;

- (instancetype)initWithDebugName: (NSString *)debugName {
  // Note: no need for if (self) if you don't do `self = [super init]`.
  if (!debugName) {
    debugName = @"DOBJCDartProtocol";
  }
  int64_t id = atomic_fetch_add(&gNextProtocolId, 1);
  debugName = [NSString stringWithFormat:@"%@_%lld",  debugName, id];
  _cls = objc_allocateClassPair(objc_lookUpClass("DOBJCDartProtocol"), [debugName UTF8String], /*extra_bytes=*/0);
  _methods = [NSMutableDictionary new];
  _finalized = NO;
  return self;
}

- (DOBJCDartProtocol*)newInstanceWithDisposePort:(Dart_Port)port {
  if (!_finalized) {
    objc_registerClassPair(_cls);
    _finalized = YES;
  }

  DOBJCDartProtocol* obj = [_cls alloc];
  return [obj initFromBuilder:self withDisposePort:port];
}

- (void) dealloc {
  objc_disposeClassPair(_cls);
}
@end

@implementation DOBJCDartProtocol {
  DOBJCDartProtocolBuilder* _builder;
  Dart_Port _dispose_port;
}

- (instancetype)initFromBuilder: (DOBJCDartProtocolBuilder*)builder withDisposePort:(Dart_Port)port {
  _builder = builder;
  _dispose_port = port;
  return self;
}

- (void)dealloc {
  if (_dispose_port != ILLEGAL_PORT) {
    Dart_PostInteger_DL(_dispose_port, 0);
  }
}

- (id)getBlockImplementingSelector:(SEL)sel {
  return [_builder->_methods objectForKey:[NSValue valueWithPointer:sel]];
}
@end

[liamappelbe]

Done, more or less. In general, all else being equal, I prefer to orchestrate in Dart (because it's a better language ;) ). But yeah, creating the class and instantiating the implementation objects on the ObjC side simplifies things a little, and the DOBJCDartProtocolClass is unnecessary.

My formatting is always a little weird because I haven't managed to get clang format to work on these files (or rather, it makes nonsensical formatting decisions, like ignoring like length).

DOBJCDartProtocol's methods (other than dealloc) are given such verbose names, and contain DOBJC, to minimize the chances that they collide with a method that the user is trying to implement.

[mraleph]

Thanks! Choices look reasonable.

I do also prefer orchestrating in Dart - however I usually apply the rule of "keeping the logic concentrated in one place". So if I am already writing native code and native code will look more natural - then I will shift most of the logic into native code and make Dart code a thin wrapper. This makes the code easier to understand - otherwise the logic gets split and you need to jump back and forth to piece it together.

DOBJCDartProtocol's methods (other than dealloc) are given such verbose names, and contain DOBJC, to minimize the chances that they collide with a method that the user is trying to implement.

Makes sense.

[mraleph]

LGTM with comments

[mraleph]

Should [self implement] just be inlined? It is small and not used for anything else.

[liamappelbe]

Done

[mraleph]

nit: a more accurate name for bypassCache is probably forceClassListReload or something like this.

[liamappelbe]

Done

[mraleph]

Question: should we be also calling class_addProtocol to make code which checks protocol conformance work?

I think right now if somebody asks our protocols dynamically ([dartProtocol conformsToProtocol:@protocol(Whatever)]) they will get NO even though dartProtocol has all necessary methods.

[liamappelbe]

Done

[mraleph]

Thanks! Choices look reasonable.

I do also prefer orchestrating in Dart - however I usually apply the rule of "keeping the logic concentrated in one place". So if I am already writing native code and native code will look more natural - then I will shift most of the logic into native code and make Dart code a thin wrapper. This makes the code easier to understand - otherwise the logic gets split and you need to jump back and forth to piece it together.

DOBJCDartProtocol's methods (other than dealloc) are given such verbose names, and contain DOBJC, to minimize the chances that they collide with a method that the user is trying to implement.

Makes sense.