Skip to content

[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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[swiftgen] Initial version of swiftgen #2372

wants to merge 13 commits into from

Conversation

liamappelbe
Copy link
Contributor

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 the swiftc command to generate the ObjC wrapper header for the library, then runs ffigen on that header.

#1393

@github-actions github-actions bot added type-infra A repository infrastructure change or enhancement package:ffigen package:swiftgen labels Jun 24, 2025
Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

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.

Package Leaked API symbols
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/native_doc_dartifier/example/native_doc_dartifier_example.dart
pkgs/native_doc_dartifier/lib/native_doc_dartifier.dart
pkgs/native_doc_dartifier/lib/src/native_doc_dartifier_base.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@coveralls
Copy link

coveralls commented Jun 24, 2025

Coverage Status

coverage: 91.736%. remained the same
when pulling 90865c9 on swiftgen4
into 7944c5a on main.

@liamappelbe liamappelbe marked this pull request as ready for review June 24, 2025 04:43
@liamappelbe liamappelbe requested a review from dcharkes June 24, 2025 04:43
Copy link
Collaborator

@dcharkes dcharkes left a 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(
Copy link
Collaborator

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.)

Copy link
Contributor Author

@liamappelbe liamappelbe Jun 25, 2025

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.

Copy link
Collaborator

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 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.

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 called Config).

Yeah, that makes sense 👍

Copy link
Contributor Author

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',
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

}

/// Selected options from the ffigen Config object.
class FfiGenConfig {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@liamappelbe liamappelbe Jun 25, 2025

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@liamappelbe
Copy link
Contributor Author

The ffigen refactor is a bigger job so I'll leave it until later: #2376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ffigen package:swiftgen type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants