Skip to content

Add Support for Nested Classes #115

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 11 commits into from
Oct 26, 2024

Conversation

jrosen081
Copy link
Contributor

Closes #19

This adds support for nested classes. There are 2 parts here:

  1. Recursively going through when translating classes
  2. Using a stack to go through all classes to add to the classes to translate.

This also migrates some places where we were using getCanonicalName when we should be using getName (to make sure that we use the $)

@DougGregor
Copy link
Member

Hah! I didn't see your PR and went off and took at stab at this myself (#118). It looks like we found some of the same things (e.g., the getCanonicalName() -> getName() bit), you handled the Jar reading better, I fixed up the plugin. I'll review yours and rebase mine on top

}

// Translate all of the Java classes into Swift classes.
for javaClass in javaClasses {
translator.startNewFile()
let swiftClassDecls = translator.translateClass(javaClass)
let swiftClassDecls = try! translator.translateClass(javaClass)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the be a try rather than a try!?

Suggested change
let swiftClassDecls = try! translator.translateClass(javaClass)
let swiftClassDecls = try translator.translateClass(javaClass)

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 was just following what was there before (there was a try! inside, I just moved it out). I'm fine making this a try

)
}
}

private func names(from javaClassNameOpt: String) -> (javaClassName: String, swiftName: String) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pulling this out

@@ -265,7 +274,7 @@ extension JavaTranslator {
)

if !enumConstants.isEmpty {
let enumName = "\(swiftTypeName)Cases"
let enumName = "\(swiftTypeName.splitSwiftTypeName().name)Cases"
Copy link
Member

Choose a reason for hiding this comment

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

This can be swiftInnermostTypeName, right?

Suggested change
let enumName = "\(swiftTypeName.splitSwiftTypeName().name)Cases"
let enumName = "\(swiftInnermostTypeName)Cases"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep missed that

Comment on lines +235 to +244
while let internalClass = classes.popLast() {
if let internalClass {
let (javaName, swiftName) = names(from: internalClass.getName())
// If we have already been through this class, don't go through it again
guard translator.translatedClasses[javaName] == nil else { continue }
let currentClassName = swiftName
let currentSanitizedClassName = currentClassName.replacing("$", with: ".")
classes.append(contentsOf: internalClass.getClasses())
translator.translatedClasses[javaName] = (currentSanitizedClassName, nil, true)
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should always recurse into nested classes. Rather, I think we should allow specification of nested classes in Java2Swift.config, and teach the Jar walker that produces Java2Swift.config to walk nested classes.

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 went back and forth on this a bit. I do like the idea of recurring down nested classes, due to the fact that the majority of uses of nested classes (at least in the JDK) are from the original class (i.e. methods that return nested classes from the main class)). I'm open to either, but like this as making a more complete class translation

Copy link
Member

Choose a reason for hiding this comment

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

It does make the result more complete with less work on the config file, I agree. I have two concerns, but now I'm thinking that they aren't that important and can be addressed in the future.

One is that this means there is no way to disable the generation of the Swift import of a nested class: maybe we never need that, but at least for the moment I've avoided importing some Java classes because they trip up the translation in ways I wasn't able to immediately deal with. That said, if it became a problem, we could invent a way to do this.

The other is in how the output is structured. With this automatic recursion, the emitted file Attributes.swift now has the Attributes.Name type in it as well, i.e., the whole nested class definition is in one file. With the explicit recursion approach, Attributes.Name ends up in a separate file Attributes+Name.swift. To do that, the config file needs to list out the Attributes$Name class as its own entry so that the Java2SwiftPlugin can know all of the files that will be emitted. It's more tidy for Swift, but it's not strictly necessary... and it would be relatively straightforward in the future to support being explicit in this way in addition to the recursion that you've implemented. For example, we could say that if you have an entry in the Java2Swift.config file for a nested class, we skip that nested class in the recursion because it's been handled at the top level already.

That's all a very, very longwinded way of saying that I agree with you now and will likely implement the "explicit" mechanism I described on top of your PR. Let's merge away!

Copy link
Member

Choose a reason for hiding this comment

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

I updated #118 to build on top of this. The Jar reader now emits nested classes (which is great), so we have to deal with some nested classes being explicitly-specified in Java2Swift.config and others that are discovered through the recursion. Thank you for working on nested classes!

Comment on lines +101 to +103
extension Attributes {
@JavaClass("java.util.jar.Attributes$Name")
public struct Name {
Copy link
Member

Choose a reason for hiding this comment

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

This is a wonderful improvement, but I think it should be driven by adding

"java.util.jar.Attributes$Name" : "Attributes.Name",

to this module's Java2Swift.config.

@DougGregor DougGregor merged commit ec76084 into swiftlang:main Oct 26, 2024
11 checks passed
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.

Swift wrappers for nested Java classes
2 participants