Skip to content
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

Return the generated path from FileSpec.writeTo(). #1514

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

fejesjoco
Copy link
Contributor

@fejesjoco fejesjoco commented Apr 9, 2023

Hi,

I have a use case where it would be nice to know the path that is generated in the writeTo method.

This is similar to how JavaPoet's JavaFile.writeToPath() and JavaFile.writeToFile() work.

Note: I haven't updated all methods in FileWritingTest because I wanted to get a round of review first. I'll be happy to do the same to the rest of the file for consistency.

Closes #1719

@Throws(IOException::class)
public fun writeTo(directory: File): Unit = writeTo(directory.toPath())
public fun writeTo(directory: File): File = writeTo(directory.toPath()).toFile()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reasons I don't understand, Path.toFile() may throw UnsupportedOperationException so technically this could be a regression even if nobody wants to know the return value. But JavaPoet does the same here, and since we already had a File to begin with, I hope it should be fine.

@JakeWharton
Copy link
Collaborator

This is a binary breaking change and this must be done as a new method with a JvmName that differs from the existing one and the existing one marked as hidden. That will retain binary compatibility and source compatibility for Kotlin (probably) but break source compatibility with Java callers, if there are any.

@fejesjoco
Copy link
Contributor Author

How about doing exactly what JavaPoet did: introduce new writeToPath/writeToFile methods which are the same as writeTo but with a return type. That way it should be source and binary compatible.

@JakeWharton
Copy link
Collaborator

Note that we had no choice in JavaPoet. Kotlin affords us the choice of using hidden methods. I think the risk is probably worth it.

@fejesjoco
Copy link
Contributor Author

This doesn't seem to work: https://pl.kotl.in/7_gQxvl0X . Also I would find it strange if the Java and Kotlin API surfaces differ only be the name of a method.

@lukellmann
Copy link

This doesn't seem to work: https://pl.kotl.in/7_gQxvl0X . Also I would find it strange if the Java and Kotlin API surfaces differ only be the name of a method.

It is possible with something like this:

@Throws(IOException::class)
@Deprecated("Binary compatibility", level = DeprecationLevel.HIDDEN)
@JvmName("writeTo") // preserve the old name in the bytecode for binary compatibility
public fun oldWriteTo(directory: Path) { // change the Kotlin name so the compiler won't complain about conflicting overloads
    writeTo(directory) // resolves to non-hidden variant
}

@Throws(IOException::class)
public fun writeTo(directory: Path): Path {
    // ...
}

That way both Kotlin and Java callers (after recompilation) would call into the variant with return value while still preserving binary compatibility and without needing a name change.

To verify this works you can look at the API dump of the Binary compatibility validator:

-public final fun writeTo (Ljava/nio/file/Path;)V
+public final fun writeTo (Ljava/nio/file/Path;)Ljava/nio/file/Path;
+public final synthetic fun writeTo (Ljava/nio/file/Path;)V

The old variant was only changed to have the ACC_SYNTHETIC flag set (not a binary-incompatible change) and will therefore be inaccessible for newly compiled Java sources (see @JvmSynthetic).

@Egorand Egorand changed the base branch from master to main July 5, 2023 09:23
@JakeWharton JakeWharton merged commit 1286d7b into square:main Nov 9, 2023
2 checks passed
@ebraminio
Copy link

I think #1730 is probably related to this.

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.

Add FileSpec output path
4 participants