Frillback

[tarrinneal]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature to Pigeon: support for generating native interop code using JNI on Android and FFI on iOS/macOS. This is a large and complex change that touches many parts of the code generation pipeline, from the AST to the individual language generators. The changes include new options to enable this functionality, new helper classes for type mapping and conversion, and updates to the generated code to use native interop where available, falling back to method channels. New test files and configurations have also been added to support this feature.

My review has found a few critical issues that should be addressed. The JNI implementation appears to be incomplete, with several key parts commented out. There are also some copy-paste errors from the JNI implementation into the FFI code that will cause it to fail. Additionally, the FFI configuration generator produces files with hardcoded absolute paths, which will break builds for other developers. I've also noted some areas for improvement in error handling and code correctness. Overall, this is a promising direction for Pigeon, but the implementation needs some fixes and completion before it's ready.

[gemini-code-assist]

The JNI implementation for custom classes appears to be a work-in-progress. This method (_writeToJni) and _writeFromJni are commented out, as are the class-handling sections within _PigeonJniCodec and the JNI path in _writeNativeInteropHostApi. If useJni is intended to be a user-facing feature in this PR, this functionality should be completed. If it's still a work-in-progress, it might be better to gate the feature more explicitly or add documentation about its current limitations.

[gemini-code-assist]

This block in _writeFfiFlutterApi seems to be an incorrect copy from the JNI implementation. It uses jni_bridge and JString which are for JNI, not FFI. The FFI implementation should be using FFI-specific constructs, likely involving NSString and ffi_bridge types for registration.

[gemini-code-assist]

The generated ffigen_config.dart file will contain absolute paths because Uri.file() is used with paths that are likely relative to the project root, which resolves them to absolute paths. This will break the build for other users or in different environments. The paths should be generated as relative paths, likely relative to the location of the generated ffigen_config.dart file itself. The JnigenYamlGenerator uses path.relative, which could be a good model to follow here.

[gemini-code-assist]

The res variable in the generated getInstance method is declared as late but is only initialized for Android, iOS, and macOS. If this code is run on an unsupported platform, return res; at the end of the function will throw a LateInitializationError. It would be safer to throw a PlatformException for unsupported platforms to provide a clearer error, or ensure all paths initialize res or return.

Suggested change

	static $dartApiName? getInstance({String channelName = defaultInstanceName}) {
	late $dartApiName res;
	if (Platform.isAndroid) {
	final $jniApiRegistrarName? link =
	$jniApiRegistrarName().getInstance(JString.fromString(channelName));
	if (link == null) {
	_throwNoInstanceError(channelName);
	}
	res = $dartApiName._withRegistrar(jniApi: link);
	} else if (Platform.isIOS || Platform.isMacOS) {
	final $ffiApiRegistrarName? link =
	$ffiApiRegistrarName.getInstanceWithName(NSString(channelName));
	if (link == null) {
	_throwNoInstanceError(channelName);
	}
	res = $dartApiName._withRegistrar(ffiApi: link);
	}
	return res;
	}
	static $dartApiName? getInstance({String channelName = defaultInstanceName}) {
	if (Platform.isAndroid) {
	final $jniApiRegistrarName? link =
	$jniApiRegistrarName().getInstance(JString.fromString(channelName));
	if (link == null) {
	_throwNoInstanceError(channelName);
	}
	return $dartApiName._withRegistrar(jniApi: link);
	} else if (Platform.isIOS || Platform.isMacOS) {
	final $ffiApiRegistrarName? link =
	$ffiApiRegistrarName.getInstanceWithName(NSString(channelName));
	if (link == null) {
	_throwNoInstanceError(channelName);
	}
	return $dartApiName._withRegistrar(ffiApi: link);
	}
	return null;
	}

[gemini-code-assist]

Using a Set for classesToCheck is a good improvement for efficiency over a List. However, the item is removed at the end of the loop. If a class refers to itself, this will result in an infinite loop. It would be safer to remove next from classesToCheck at the beginning of the loop.

  final Set<String> classesToCheck = Set<String>.from(referencedTypeNames);
  while (classesToCheck.isNotEmpty) {
    final String next = classesToCheck.first;
    classesToCheck.remove(next);
    final Class aClass = classes.firstWhere(
      (Class x) => x.name == next,
      orElse: () => Class(name: '', fields: <NamedType>[]),
    );
    for (final NamedType field in aClass.fields) {
      references.add(field.type, field.offset);
      if (_isUnseenCustomType(field.type, referencedTypeNames)) {
        classesToCheck.add(field.type.baseName);
      }
      for (final TypeDeclaration typeArg in field.type.typeArguments) {
        references.add(typeArg, field.offset);
        if (_isUnseenCustomType(typeArg, referencedTypeNames)) {
          classesToCheck.add(typeArg.baseName);
        }
      }
    }
  }

[gemini-code-assist]

Returning a string literal for an unhandled type, such as 'There is something wrong, a type is not classified', will lead to generated code that doesn't compile and provides a poor developer experience. It would be better to throw a PigeonError with more context (like the type name and location) to help the user identify the problem in their pigeon file. This applies to _FfiType.ffiName, _JniType.jniName, and other similar places.

[gemini-code-assist]

There's a typo in the generated error message: a stray double quote at the end of the interpolated string.

Suggested change

	'No ${api.name} instance with \${nativeInteropApiInstanceName.isEmpty ? 'no ' : ''} instance name \${nativeInteropApiInstanceName.isNotEmpty ? '"\$nativeInteropApiInstanceName"' : ''} "\$nativeInteropApiInstanceName "}found.');
	'No ${api.name} instance with ${nativeInteropApiInstanceName.isEmpty ? 'no ' : ''}instance name ${nativeInteropApiInstanceName.isNotEmpty ? '"$nativeInteropApiInstanceName"' : ''}found.');

[gemini-code-assist]

This file appears to be a duplicate of ni_tests.dart. It should probably be removed to avoid confusion and reduce maintenance.