[pigeon] Adds Dart implementation of ProxyApi #6043
Conversation
| }) { | ||
| const String codecName = '_${classNamePrefix}ProxyApiBaseCodec'; | ||
|
|
||
| // Each api has a private codec instance used by every host method, |
| .followedBy(flutterMethodsFromInterfaces) | ||
| .followedBy(api.flutterMethods), | ||
| )), | ||
| ); |
There was a problem hiding this comment.
This seems pretty nice. Definitely something to consider for the other Dart generation.
| ProxyApiSuperClass? nullableProxyApiParam, | ||
| ); | ||
|
|
||
| late bool aBool; |
There was a problem hiding this comment.
Why does everything need to be late in the interface definition?
There was a problem hiding this comment.
late is required since the field isn't added to the constructor. And it's not added to the constructor to differentiate it from constructor params: https://docs.google.com/document/d/1Sw6F8GbNiO_lvS2fFjXrRYk2Go5RNDHudDT6kIX6Sm0/edit?tab=t.0#heading=h.1amuvpa8b1ic
|
(Mostly small comments; There are some big walls of text so I was skimming in the generator, but it looks like the overall structure here is pretty well factored.) |
There was a problem hiding this comment.
Do you see these templates changing much in the future? I get a bit nervous thinking about adding logic into this later. Just based on doing that with previously written pigeon code.
There was a problem hiding this comment.
I wouldn't expect these to have many changes in the future. And if they do, it shouldn't be changes that require adding logic when generating them. This should at least be true for InstanceManager, InstanceManagerApi, and ProxyApiBaseClass. I expect all of these to always be basic templates.
However, ProxyApiBaseCodec could require logic later to solve some complex issues, but I don't have any plans for it yet.
| // 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can these be moved onto the AstProxyApi class itself and be pre-populated during the ast generation?
In general I'm hoping to move any data processing like this into pre generation. Especially if it's going to happen in the dart generator and or multiple generators.
| final bool hasHostMethod = root.apis | ||
| .whereType<AstHostApi>() | ||
| .any((AstHostApi api) => api.methods.isNotEmpty) || | ||
| root.apis.whereType<AstProxyApi>().any((AstProxyApi api) => | ||
| api.constructors.isNotEmpty || | ||
| api.attachedFields.isNotEmpty || | ||
| api.hostMethods.isNotEmpty); | ||
| final bool hasFlutterMethod = root.apis | ||
| .whereType<AstFlutterApi>() | ||
| .any((AstFlutterApi api) => api.methods.isNotEmpty) || | ||
| root.apis.any((Api api) => api is AstProxyApi); | ||
|
|
||
| if (hasHostMethod) { | ||
| _writeCreateConnectionError(indent); | ||
| } | ||
| if (hasFlutterApi || generatorOptions.testOutPath != null) { | ||
| if (hasFlutterMethod || generatorOptions.testOutPath != null) { | ||
| _writeWrapResponse(generatorOptions, root, indent); | ||
| } | ||
| } |
There was a problem hiding this comment.
You don't need to do this, but ideally these checks would be done pre-generation as well, so these bool checks would be a lot cleaner here.
There was a problem hiding this comment.
I created flutter/flutter#144642 to tracking moving the methods to Generator.
...ges/pigeon/platform_tests/shared_test_plugin_code/lib/src/generated/proxy_api_tests.gen.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This file makes me want to quit my job. Generally seems like everything is there and makes sense.
I'm torn about whether or not to keep it apart from the other core_tests file.
There was a problem hiding this comment.
Sorry, this file is all generated so that I can write the integration tests and verify the generated code doesn't cause any lint warnings. I assumed the review for this file would only require someone to skim it.
There was a problem hiding this comment.
In a sense this is the code that needs the most review, since it's the actual functioning code from what you've written. The rest of it is just preference and future work looking.
packages/pigeon/lib/ast.dart
Outdated
| /// 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Doesn't the code that associates the api's make copies? Maybe I'm looking in the wrong place.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you add a todo for this and tag me in it?
There was a problem hiding this comment.
I think we're good to land this now, if @stuartmorgan wants to get another pass in now's the time.
Please add an issue as mentioned above, and I think it's good to land.
|
I've reviewed manual versions of the output in |
bparrishMines
added
the
autosubmit
flutter/packages@a2f4ce0...23e56af 2024-03-19 [email protected] [camerax] Update README to encourage users to opt in (flutter/packages#6352) 2024-03-19 [email protected] [flutter_markdown] Allow for custom block element (flutter/packages#5815) 2024-03-19 [email protected] [pigeon] Adds Dart implementation of ProxyApi (flutter/packages#6043) 2024-03-19 [email protected] [camerax] Implements `setFocusMode` (flutter/packages#6176) 2024-03-19 [email protected] Roll Flutter from f217fc1 to d31a85b (23 revisions) (flutter/packages#6356) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Part of flutter/flutter#134777
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.