-
Notifications
You must be signed in to change notification settings - Fork 69
[swiftgen] Initial version of swiftgen #2372
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
base: main
Are you sure you want to change the base?
Conversation
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. 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.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This brings up some interesting questions for the generator APIs:
- How do we facilitate composition?
- How do we make them as declarative as possible?
I haven't fully flashed out my thinking about this yet and left a bunch of comments w.r.t. this.
If you have any bright ideas, shoot! cc @HosseinYousefi
stderr.writeln('${record.level.name}: ${record.message}'); | ||
}); | ||
|
||
await generate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can we make it
final generator = Swiftgen(
target: ...,
...,
);
await generator.run(logger: Logger());
edit: I see we have it the other way around for ffigen. The constructor takes the logger and the run
the config. I guess that makes running two FFIgen's slightly more convenient, you don't pass the logger twice. However, it leads to having both an FfiGen
and a Config
object. Having a big object literal in the run
argument makes it feel less declarative. I feel like the user-configuration should be the declarative constructor/factory, and then context about how that config should be executed should be parameters to run
(Logger
, FileSystem
, ProcessManager
, workingDirectory
). This would follow the Flutter-style focusing on object literals that the users write and then having the "framework" pass everything to the run/build methods.
(And we might be able to add methods such as validate
next to run
.)
In our case we'd not be composing widgets, but we would be composing generators. (If possible, you construct the FFIgen object with its config already in the factory/constructor of SwiftGen, so SwiftGen.run
basically only invokes run of other generators and some process calls. But maybe we only have all the required information for the FFIgen config once we've already run some earlier step, in that case we'd need to construct the generator in the run function.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapping the args around so the config stuff is in the constructor and the logger etc is in the run
method sounds reasonable to me.
The idea of merging the FfiGen
object with the Config
object doesn't feel right to me though.
The Config
object is referenced all through the ffigen implementation. Passing around a monolithic config feels fine to me (probably because there's an implicit contract that this object is just dumb immutable data, due to the class being called Config
).
If we're instead passing around an FfiGen
object, that's now a big cyclic dependency: every part of the ffigen implementation now has a reference to the top level object, with a run
method that kicks off the whole pipeline.
We could make the API look like they're merged, but internally still have the distinction, by having the FfiGen
constructor take all the same params as the Config
constructor, and just construct it's own Config
. But that's a whole lot of code duplication, especially since Config
is an interface with multiple implementations with their own constructors.
Maybe if run
was an extension method on the Config
object, it'd feel less weird? At least that way I could avoid importing that extension into the main ffigen implementation, so there wouldn't be a cyclic dep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make the API look like they're merged, but internally still have the distinction, by having the
FfiGen
constructor take all the same params as theConfig
constructor, and just construct it's ownConfig
. But that's a whole lot of code duplication, especially sinceConfig
is an interface with multiple implementations with their own constructors.
Yeah I considered this as well. And then FfiGen.yaml(...)
can be the constructor for the yaml config.
With CBuilder
we actually have that class as API and then a RunCBuilder
for running it that lives behind the API.
So the public API is a facade so we can internally refactor and rename without making breaking changes to the public API. (At the cost of having to have constructors that forward all params to a config object.)
The
Config
object is referenced all through the ffigen implementation. Passing around a monolithic config feels fine to me (probably because there's an implicit contract that this object is just dumb immutable data, due to the class being calledConfig
).
Yeah, that makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, at least for the swiftgen part. Ffigen part will come later. Renamed swiftgen's Config
object to SwiftGen
, and made generate
an extension method.
); | ||
|
||
final result = Process.runSync('swiftc', [ | ||
'-emit-library', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these params should then be part of the declarative SwiftGen
(factory) constructor.
The process invocation should then be part of the run
function.
And the stdout and stderr can be streamed to the Logger
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general we should keep code generation and compilation separate. I don't know how the user wants to build their library, but I assume they'd typically compile it along with a bunch of other swift files directly into their app. Also typically the generated code is checked in, but the compiled artifact isn't. In the future I'll probably codegen a build hook, here and in ffigen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more polished version of this example would probably have this command in a separate build hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, you're compiling the outputted code, not compiling code as part of the code gen. Yes, this should live in the build hook.
Actually, it would be nice if the same Swiftgen
declarative object has a Future generate({logger})
and a Future build({logger})
method. And then you put that object declaration in your lib/src/
and refer to it from both hook/build.dart
and tool/generate.dart
(or hook/generate.dart
if we introduce that). That way we can completely abstract away where the generated code is and what it looks like. The only question then is how does the user access the dylib at runtime. I guess the Swiftgen
constructor should take an asset ID then.
Would an approach like this work do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. It's a little odd, but could work. Odd because the compile step only needs a few params from this huge config object. If users don't think to share the definition of the config object, they'll end up with a lot of redundant setting of constructor fields. I guess that's just a documentation issue though.
The asset ID could be a constructor param, but it can be optional. The user doesn't really need to know how the dylib is being loaded at runtime, because the generator is also generating the bindings that load it. So we control both ends of the loading. It could be entirely hidden from the user.
pkgs/swiftgen/lib/src/config.dart
Outdated
} | ||
|
||
/// Selected options from the ffigen Config object. | ||
class FfiGenConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is interesting, and brings us to the generator composition.
If this is a strict subset, we might want to avoid redeclaring all the fields.
Can we take FFIgens Config
instead? (We should probably rename that class to FfiGenConfig
if we expect it to be reused in other generators due to composition of generators.) And then add a copyWith
method to FfiGenConfig
to add/modify some of the config fields from SwiftGen. (If we merge FfiGen
and FfiGenConfig
and make it have a run function, then the copyWith
method should be on FfiGen
.)
Or is it the case that the FFIgen config has some mandatory fields that are required that are provided by swiftgen? In that case:
- Maybe make all the doc-comments have a link to the ffigen fields instead of repeating the doc-comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mainly just the entryPoints
, the language
, and the compilerOpts
that would need to be controlled by swiftgen. So I guess something like copyWith
would work in theory.
One issue with taking the entire ffigen.Config
object was that users might not know that we're overwriting some fields. But that's mainly a documentation issue.
There's also the more concrete problem that there are multiple implementations of ffigen.Config
. It's not clear how the copyWith
pattern can be applied in that situation. Each implementation would need a different copyWith
, but they have different fields so the copyWith
methods would have different signatures. It happens to be the case that the 3 fields we care about in swiftgen have more or less the same underlying data type (not exactly the same, but close enough) among the existing implementations of ffigen.Config
. So some sort of bespoke copyWith
that only supports modifying those fields might be doable. But we'd still need an implementation of this method per ffigen.Config
implementation, so it's a bit of a maintenance burden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to copyWith
, which would solve that issue I mentioned, would be to write an implementation of ffigen.Config
in swiftgen that wraps the user's ffigen.Config
, and delegates most of its methods to the user's config. Then it would just override the behavior of the getters for those 3 fields to use swiftgen's value instead. Still a bit of a maintenance burden, but in this case it's about the same burden as this custom FfiGenConfig
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we have a bunch of options here. Having a separate config object yes no, having copyWith or explicitly marking the config as base class so it can be extended in other packages (with overwriting some fields). Let's discuss in the sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Went with the delegating re-implementation approach, as discussed.
The ffigen refactor is a bigger job so I'll leave it until later: #2376 |
Initial version of swiftgen. This version doesn't integrate with swift2objc. Instead it assumes that the input library is already annotated with
@objc
. It invokes theswiftc
command to generate the ObjC wrapper header for the library, then runs ffigen on that header.#1393