Skip to content

[pigeon] Adds Dart implementation of ProxyApi #6043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 45 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
6e385f1
update ast
bparrishMines Jan 10, 2024
abc026a
constructor extend method and make required default to true
bparrishMines Jan 10, 2024
870cec3
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Jan 10, 2024
0c69e1d
use helper method
bparrishMines Jan 10, 2024
eb14522
fix some validation and add tests
bparrishMines Jan 12, 2024
155469f
change required to isRequired
bparrishMines Jan 12, 2024
3a45694
improve docs
bparrishMines Jan 12, 2024
6350353
fix lint errors and hide ProxyApi
bparrishMines Jan 12, 2024
5cfcb3a
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Jan 12, 2024
4ce1e21
add hide comment
bparrishMines Jan 12, 2024
22c3065
test for recursive looks
bparrishMines Jan 12, 2024
a14485e
fix tools version
bparrishMines Jan 12, 2024
360dd7a
review comments
bparrishMines Jan 23, 2024
ea9f7c3
switch to a method that looks for matching prefix
bparrishMines Jan 24, 2024
33b2e07
avoid loops
bparrishMines Jan 24, 2024
6c79879
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Jan 24, 2024
c8db9c8
fix test
bparrishMines Jan 24, 2024
b414381
change superClass and interfaces to TypeDeclarations
bparrishMines Jan 25, 2024
00db939
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Jan 25, 2024
54c7ac2
add deps
bparrishMines Jan 25, 2024
d05bebf
add proxyapi ast helper gen methods
bparrishMines Jan 27, 2024
dfb7e45
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Jan 30, 2024
8015a9a
implementation of proxyapis stuff for dart
bparrishMines Jan 31, 2024
2dac15d
add proxy api base class
bparrishMines Jan 31, 2024
36bbca1
add documentation
bparrishMines Feb 2, 2024
afa62e8
add tests and some minor validatation improvements
bparrishMines Feb 2, 2024
1cd4d95
add tests actually
bparrishMines Feb 2, 2024
b08d017
generate test api file
bparrishMines Feb 2, 2024
fc3df6f
method improvements
bparrishMines Feb 2, 2024
0caa963
add protected for more methods
bparrishMines Feb 2, 2024
1c61c51
improvement of docs
bparrishMines Feb 2, 2024
cb2e654
change enum name
bparrishMines Feb 4, 2024
12d9c7d
dont gen method without apis
bparrishMines Feb 4, 2024
8abdb3f
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Feb 4, 2024
1d2c549
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Feb 27, 2024
c0aa558
fix class name generation without underscore
bparrishMines Feb 27, 2024
1f0c6ce
change to indexMap and fix extra space
bparrishMines Feb 27, 2024
7f1f70e
review comments and regen
bparrishMines Feb 27, 2024
2b04aa5
`Merge branch 'main' of github.com:flutter/packages into pigeon_wrapp…
bparrishMines Feb 27, 2024
94106ae
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Feb 28, 2024
796d336
move methods
bparrishMines Feb 28, 2024
dcbd085
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Feb 28, 2024
358b6d7
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Mar 5, 2024
0138563
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Mar 14, 2024
ea338c8
Merge branch 'main' of github.com:flutter/packages into pigeon_wrappe…
bparrishMines Mar 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions packages/pigeon/lib/ast.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:collection/collection.dart' show ListEquality;
import 'package:meta/meta.dart';
import 'generator_tools.dart';
import 'pigeon_lib.dart';

typedef _ListEquals = bool Function(List<Object?>, List<Object?>);
Expand Down Expand Up @@ -177,6 +178,46 @@ class AstProxyApi extends Api {
(ApiField field) => !field.isAttached,
);

/// A list of AstProxyApis where each `extends` the API that follows it.
Iterable<AstProxyApi> get allSuperClasses => recursiveGetSuperClassApisChain(
this,
);

/// All ProxyApis this API `implements` and all the interfaces those APIs
/// `implements`.
Iterable<AstProxyApi> get apisOfInterfaces =>
recursiveFindAllInterfaceApis(this);

/// All methods inherited from interfaces and the interfaces of interfaces.
Iterable<Method> flutterMethodsFromInterfaces() sync* {
for (final AstProxyApi proxyApi in apisOfInterfaces) {
yield* proxyApi.methods;
}
}

/// A list of Flutter methods inherited from the ProxyApi that this ProxyApi
/// `extends`.
///
/// This also recursively checks the ProxyApi that the super class `extends`
/// and so on.
///
/// This also includes methods that super classes inherited from interfaces
/// with `implements`.
Iterable<Method> flutterMethodsFromSuperClasses() sync* {
for (final AstProxyApi proxyApi in allSuperClasses.toList().reversed) {
yield* proxyApi.flutterMethods;
}
if (superClass != null) {
final Set<AstProxyApi> interfaceApisFromSuperClasses =
recursiveFindAllInterfaceApis(
superClass!.associatedProxyApi!,
);
for (final AstProxyApi proxyApi in interfaceApisFromSuperClasses) {
yield* proxyApi.methods;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if performance really matters here, but ideally these methods would only ever be ran once, and the data stored on the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since some of the fields of AstProxyApi that this method uses are mutable, I don't think we can run it only once. And they are mutable because of the step in pigeon_lib.dart where the associatedProxyApi is added to all the type declarations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the code that associates the api's make copies? Maybe I'm looking in the wrong place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, a new api is not created: https://github.com/flutter/packages/blob/main/packages/pigeon/lib/pigeon_lib.dart#L1297 and https://github.com/flutter/packages/blob/main/packages/pigeon/lib/pigeon_lib.dart#L1337.

Only NamedTypes (e.g. fields, methods parameters, method returnType, constructor parameters, etc...) are copied. The original apis, methods, and constructors are retained.

But even so, as long as the fields are mutable, it could cause a bug in the future if a contributor uses the method. I do think we should probably restructure the AST with immutable classes in the future. Mutable data classes can end up being a foot-gun.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a todo for this and tag me in it?


@override
String toString() {
return '(ProxyApi name:$name methods:$methods field:$fields '
Expand Down
97 changes: 30 additions & 67 deletions packages/pigeon/lib/dart_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -455,9 +455,9 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
Indent indent, {
required String dartPackageName,
}) {
indent.writeln(proxyApiBaseClass);
indent.format(proxyApiBaseClass);

indent.writeln(
indent.format(
instanceManagerTemplate(
allProxyApiNames: root.apis
.whereType<AstProxyApi>()
Expand All @@ -473,7 +473,7 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
Indent indent, {
required String dartPackageName,
}) {
indent.writeln(
indent.format(
instanceManagerApiTemplate(
dartPackageName: dartPackageName,
pigeonChannelCodecVarName: _pigeonChannelCodec,
Expand All @@ -487,7 +487,7 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
Root root,
Indent indent,
) {
indent.writeln(proxyApiBaseCodec);
indent.format(proxyApiBaseCodec);
}

@override
Expand All @@ -500,48 +500,11 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
}) {
const String codecName = '_${classNamePrefix}ProxyApiBaseCodec';

// Each api has a private codec instance used by every host method,
// Each API has a private codec instance used by every host method,
// constructor, or non-static field.
final String codecInstanceName = '${_varNamePrefix}codec${api.name}';

// A list of ProxyApis where each `extends` the API that follows it.
final List<AstProxyApi> superClassApisChain =
recursiveGetSuperClassApisChain(
api,
);

// All ProxyApis this API `implements` and all the interfaces those APIs
// `implements`.
final Set<AstProxyApi> apisOfInterfaces =
recursiveFindAllInterfaceApis(api);

// All methods inherited from interfaces and the interfaces of interfaces.
final List<Method> flutterMethodsFromInterfaces = <Method>[];
for (final AstProxyApi proxyApi in apisOfInterfaces) {
flutterMethodsFromInterfaces.addAll(proxyApi.methods);
}

// A list of Flutter methods inherited from the ProxyApi that this ProxyApi
// `extends`. This also recursively checks the ProxyApi that the super class
// `extends` and so on.
//
// This also includes methods that super classes inherited from interfaces
// with `implements`.
final List<Method> flutterMethodsFromSuperClasses = <Method>[];
for (final AstProxyApi proxyApi in superClassApisChain.reversed) {
flutterMethodsFromSuperClasses.addAll(proxyApi.flutterMethods);
}
if (api.superClass != null) {
final Set<AstProxyApi> interfaceApisFromSuperClasses =
recursiveFindAllInterfaceApis(
api.superClass!.associatedProxyApi!,
);
for (final AstProxyApi proxyApi in interfaceApisFromSuperClasses) {
flutterMethodsFromSuperClasses.addAll(proxyApi.methods);
}
}

// Ast class used by code_builder to generate the code.
// AST class used by code_builder to generate the code.
final cb.Class proxyApi = cb.Class(
(cb.ClassBuilder builder) => builder
..name = api.name
Expand All @@ -564,17 +527,18 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
codecInstanceName: codecInstanceName,
superClassApi: api.superClass?.associatedProxyApi,
unattachedFields: api.unattachedFields,
flutterMethodsFromSuperClasses: flutterMethodsFromSuperClasses,
flutterMethodsFromInterfaces: flutterMethodsFromInterfaces,
flutterMethodsFromSuperClasses: api.flutterMethodsFromSuperClasses(),
flutterMethodsFromInterfaces: api.flutterMethodsFromInterfaces(),
declaredFlutterMethods: api.flutterMethods,
))
..constructors.add(
_proxyApiDetachedConstructor(
apiName: api.name,
superClassApi: api.superClass?.associatedProxyApi,
unattachedFields: api.unattachedFields,
flutterMethodsFromSuperClasses: flutterMethodsFromSuperClasses,
flutterMethodsFromInterfaces: flutterMethodsFromInterfaces,
flutterMethodsFromSuperClasses:
api.flutterMethodsFromSuperClasses(),
flutterMethodsFromInterfaces: api.flutterMethodsFromInterfaces(),
declaredFlutterMethods: api.flutterMethods,
),
)
Expand All @@ -592,7 +556,7 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
api.flutterMethods,
apiName: api.name,
))
..fields.addAll(_proxyApiInterfaceApiFields(apisOfInterfaces))
..fields.addAll(_proxyApiInterfaceApiFields(api.apisOfInterfaces))
..fields.addAll(_proxyApiAttachedFields(api.attachedFields))
..methods.add(
_proxyApiSetUpMessageHandlerMethod(
Expand All @@ -602,8 +566,8 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
codecName: codecName,
unattachedFields: api.unattachedFields,
hasCallbackConstructor: api.flutterMethods
.followedBy(flutterMethodsFromSuperClasses)
.followedBy(flutterMethodsFromInterfaces)
.followedBy(api.flutterMethodsFromSuperClasses())
.followedBy(api.flutterMethodsFromInterfaces())
.every((Method method) => !method.isRequired),
),
)
Expand All @@ -623,17 +587,20 @@ final BinaryMessenger? ${_varNamePrefix}binaryMessenger;
codecInstanceName: codecInstanceName,
codecName: codecName,
))
..methods.add(_proxyApiCopyMethod(
apiName: api.name,
unattachedFields: api.unattachedFields,
declaredAndInheritedFlutterMethods: flutterMethodsFromSuperClasses
.followedBy(flutterMethodsFromInterfaces)
.followedBy(api.flutterMethods),
)),
..methods.add(
_proxyApiCopyMethod(
apiName: api.name,
unattachedFields: api.unattachedFields,
declaredAndInheritedFlutterMethods: api
.flutterMethodsFromSuperClasses()
.followedBy(api.flutterMethodsFromInterfaces())
.followedBy(api.flutterMethods),
),
),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty nice. Definitely something to consider for the other Dart generation.


final cb.DartEmitter emitter = cb.DartEmitter(useNullSafetySyntax: true);
indent.write(DartFormatter().format('${proxyApi.accept(emitter)}'));
indent.format(DartFormatter().format('${proxyApi.accept(emitter)}'));
}

/// Generates Dart source code for test support libraries based on the given AST
Expand Down Expand Up @@ -1140,8 +1107,8 @@ if (${_varNamePrefix}replyList == null) {
/// The detached constructor present for every ProxyApi.
///
/// This constructor doesn't include a host method call to create a new native
/// class instance. It is mainly used when the native side once to create a
/// Dart instance and when the `InstanceManager` wants to create a copy for
/// class instance. It is mainly used when the native side wants to create a
/// Dart instance or when the `InstanceManager` wants to create a copy for
/// automatic garbage collection.
cb.Constructor _proxyApiDetachedConstructor({
required String apiName,
Expand Down Expand Up @@ -1260,17 +1227,13 @@ if (${_varNamePrefix}replyList == null) {
<String>[
...method.documentationComments,
...<String>[
if (method.documentationComments.isNotEmpty)
''
else ...<String>[
'Callback method.',
'',
],
if (method.documentationComments.isEmpty) 'Callback method.',
'',
'For the associated Native object to be automatically garbage collected,',
"it is required that the implementation of this `Function` doesn't have a",
'strong reference to the encapsulating class instance. When this `Function`',
'references a non-local variable, it is strongly recommended to access it',
'from a `WeakReference`:',
'with a `WeakReference`:',
'',
'```dart',
'final WeakReference weakMyVariable = WeakReference(myVariable);',
Expand Down
8 changes: 4 additions & 4 deletions packages/pigeon/pigeons/proxy_api_tests.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ enum ProxyApiTestEnum {
three,
}

/// The core interface that each host language plugin must implement in
/// platform_test integration tests.
/// The core ProxyApi test class that each supported host language must
/// implement in platform_tests integration tests.
@ProxyApi()
abstract class ProxyApiTestClass extends ProxyApiSuperClass
implements ProxyApiInterface {
Expand Down Expand Up @@ -454,15 +454,15 @@ abstract class ProxyApiTestClass extends ProxyApiSuperClass
String callFlutterEchoAsyncString(String aString);
}

/// ProxyApi to serve as a super class to the core ProxyApi interface.
/// ProxyApi to serve as a super class to the core ProxyApi class.
@ProxyApi()
abstract class ProxyApiSuperClass {
ProxyApiSuperClass();

void aSuperMethod();
}

/// ProxyApi to serve as an interface to the core ProxyApi interface.
/// ProxyApi to serve as an interface to the core ProxyApi class.
@ProxyApi()
abstract class ProxyApiInterface {
late void Function()? anInterfaceMethod;
Expand Down
Loading