Skip to content

Introduce protocol-based structure for generating code #249

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 2 commits into
base: main
Choose a base branch
from

Conversation

madsodgaard
Copy link
Contributor

This goal of this PR is to introduce a protocol Swift2JavaGenerator, which does all the code generation. This will allow us to provide multiple generation modes, such as FFM or JNI.

Most of the stuff from Swift2JavaTranslator+... has been moved into FFMSwift2JavaGenerator.

@madsodgaard
Copy link
Contributor Author

@rintaro I could use a bit of your thoughts here. Currently, it seems like the visitor depends on translated types, specifically this function


used in places like
translatedSignature = try translator.translate(swiftSignature: swiftSignature, as: .initializer)

This stuff seems FFM specific, so should really live in the FFM implementation of the protocol and the visitor should not depend on it. It seems like the property translatedSignature is only actually used in the translation phase, not the analysis phase. So my initial thought was to postpone this translation to the actual FFM phase and not during the visiting phase. I just wanted to confirm with you whether you think this is a good idea, or if you have any better ideas?

@@ -0,0 +1,3 @@
protocol Swift2JavaGenerator {
func generate() throws
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 seems kind of weird that this protocol just has an "empty" function with no arguments. But, I did that to prevent having to pass AnalysisResult to a bunch of sub-functions in print... So instead the implementations can receive them as initializer arguments.

Copy link
Member

@rintaro rintaro Jun 5, 2025

Choose a reason for hiding this comment

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

It's not clear for me how this protocol will be used. The main flow can be just like:

  try translator.analyze()
  
  if shouldGenerateFFM {
    FFMSwift2JavaGenerator(analysis, ...).generate()
  }
  
  if shouldGenrateJNI {
    JNISwift2JavaGenerator(analysis, ...).generate()
  }

Could you explain why we'd want this protocol?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that’s pretty equivalent. The protocol being just the generate method seemed nicer to me, and we’d swap out what we generate with but it’s equivalent to that if statement. Since there’s not much to share either approach is fine i think, modeling wise i like the protocol but we could do the if

@@ -88,6 +88,13 @@ extension Swift2JavaTranslator {
/// a checked truncation operation at the Java/Swift board.
var javaPrimitiveForSwiftInt: JavaType { .long }

var result: AnalysisResult {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this should just be the return value of analyze()?

@rintaro
Copy link
Member

rintaro commented Jun 5, 2025

I'm totally fine with delaying FFM translate after the "analysis" phase, which will mean the AnalysisResult (i.e. ImportedFunc) would only hold SwiftFunctionSignature at most, not TranslatedFunctionSignature, right?

There are a couple of reasons I didn't do that:

  • Wasn't sure sharing the visitor would be the way to go. It's just 250 lines of code. JNI generator might want to create its own visitor with its own needs. We could still share SwiftSymbolTable and SwiftFunctionSignature.
  • Wanted to keep the TranslatedFunctionSignature somewhere around. Although I don't think it's used multiple times at this point, it's possible that we want to use it multiple places in the generator/printer.

That being said, I now feel sharing analysis result is a good move. To resolve the 2nd bullet, probably FFMSwift2JavaGenerator can hold the cached TranslatedFunctionSignature result for each ImportedFunc, (just like the thunkNameRegistry) I.e.

var translatedSignatures: [ImportedFunc: TranslatedFunctionSignature]

@ktoso
Copy link
Collaborator

ktoso commented Jun 5, 2025

Cool, that’s the outcome I was hoping for here — when we delay the translated signature the analysis can remain detached of the output, and FFM and JNI can do their own translation/lowering based on that result 👍

@rintaro
Copy link
Member

rintaro commented Jun 5, 2025

Okay, I am moving the FFM translation part to after analyze(). #252

@madsodgaard Sorry for the conflicts, but I believe this makes JNI additions easier.

@ktoso
Copy link
Collaborator

ktoso commented Jun 6, 2025

Thank you Rintaro! :)

@madsodgaard madsodgaard force-pushed the protocol-generation branch from c668a45 to 08aa68d Compare June 7, 2025 13:25
@madsodgaard
Copy link
Contributor Author

@rintaro @ktoso I think this is ready for a first pass. For now, I have just kept the protocol, but as @rintaro pointed out, we don't actually do any dynamic swapping it in that way, but I'll just keep it around for now.

Otherwise, most of the stuff is just moving the previous translator stuff into FFMSwift2JavaGenerator, as well as a new FFM folder. The tests still assume we always test FFM, but we can perhaps look at how to tackle more generalized ways of testing both JNI and FFM once we get further and actually start generating JNI.

@madsodgaard madsodgaard requested review from ktoso and rintaro June 7, 2025 13:29
@madsodgaard madsodgaard changed the title [WIP] Introduce protocol-based structure for generating code Introduce protocol-based structure for generating code Jun 7, 2025
@madsodgaard madsodgaard marked this pull request as ready for review June 7, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants