Leave files unchanged when legacy Base64 calls cannot be fully migrated

[martinfrancois]

Suggested review order: 41 of 52 (Score: 2)
Review first: #1196

What's changed?

UseJavaUtilBase64 now leaves a source file unchanged when it cannot migrate every use of the legacy coders in it. The legacy coders are sun.misc.BASE64Encoder and sun.misc.BASE64Decoder, which inherit their coder methods from sun.misc.CharacterEncoder and sun.misc.CharacterDecoder.

Current main uses ChangeType to retype the two legacy coders to java.util.Base64.Encoder and java.util.Base64.Decoder. It rewrites only those calls for which java.util.Base64 has an equivalent method, and leaves every other call as written, on a receiver whose type it has just changed.

Before

BASE64Encoder encoder = new BASE64Encoder();
String encoded = encoder.encode(bBytes);
encoder.encodeBuffer(bBytes, output);

Actual after the recipe

Using current main.

Base64.Encoder encoder = Base64.getEncoder();
String encoded = encoder.encodeToString(bBytes);
encoder.encodeBuffer(bBytes, output);  // Base64.Encoder has no encodeBuffer

Expected after the recipe

(unchanged)

The recipe makes no edit at all.

visitCompilationUnit keeps its existing whole-file check, alreadyUsingIncompatibleBase64, and runs a second one, usesLegacyTypeUntranslatably, after it. When the new check reports true, the file is returned unchanged. It reports true when the file contains any of:

• a call to a legacy coder method this recipe cannot rewrite, meaning any coder method other than the three named below, or a call to one of those three on a receiver whose type is not BASE64Encoder or BASE64Decoder itself;
• an expression whose declared type is CharacterEncoder or CharacterDecoder, which ChangeType never retypes;
• a class extending a legacy coder, or an instantiation with an anonymous class body, such as new BASE64Decoder() { };
• a method reference to a legacy coder, such as encoder::encode;
• a call passing a value to a parameter declared as CharacterEncoder or CharacterDecoder.

A parameter declared as BASE64Encoder or BASE64Decoder is none of those, since ChangeType does retype those two classes, so a file whose only legacy use is such a parameter is still migrated.

What's your motivation?

Recipe: org.openrewrite.java.migrate.UseJavaUtilBase64.

The two legacy coders inherit 16 public coder methods from those supertypes, 10 from CharacterEncoder and 6 from CharacterDecoder. The recipe rewrites 3 of them, the same 3 on current main and on this branch: encode(byte[]) and encodeBuffer(byte[]) become Base64.getEncoder().encodeToString(byte[]), and decodeBuffer(String) becomes Base64.getDecoder().decode(String). Calls to the remaining 13 are left as written, on a receiver current main has just retyped to Base64.Encoder or Base64.Decoder, neither of which declares them.

The file current main produces therefore does not compile: javac --release 11 rejects the output block above with cannot find symbol: method encodeBuffer(byte[],OutputStream). The recipe still reports that file as successfully changed, so nothing in the run tells the user the result is broken. UseJavaUtilBase64 runs inside Java8toJava11, UpgradeToJava17 and UpgradeToJava21, so any of those three can turn a Java 8 codebase that compiles into one that does not. Reproduced on 3.41.0 and on 3.42.0-SNAPSHOT built from current main.

Affected code in real projects

• openjdk/jfx WebEngine.java: the Android port of the JavaFX WebView converts a user stylesheet URL to a data URL with the stream overload new sun.misc.BASE64Encoder().encodeBuffer(in, out); the recipe from main replaces the allocation with Base64.getEncoder() but cannot rewrite the two-argument encodeBuffer, so the output calls Base64.getEncoder().encodeBuffer(in, out), a method Base64.Encoder does not declare, and the file no longer compiles.

Anything in particular you'd like reviewers to focus on?

No existing test expectation changed: the test file has added lines only, and the three tests already in UseJavaUtilBase64Test are untouched. Three points:

• The decision is per file, not per call, because ChangeType retypes the whole file, so a file mixing a call the recipe can rewrite with one it cannot is now left entirely alone. The check is deliberately wide, so it also refuses some files current main migrates correctly, for example a supported method called on a receiver whose declared type is a type variable, as in <T extends BASE64Encoder> void write(T coder) calling coder.encode(b), since a type variable is neither of the two receiver types the first bullet above accepts. I have not measured how often that shape occurs in real code.
• The check runs before super.visitCompilationUnit(...). Checking afterwards and discarding the visit result would not work: rewriting decodeBuffer(String) schedules UnnecessaryCatch through doAfterVisit, because the sun.misc decodeBuffer declares throws IOException while Base64.Decoder.decode does not, and a visitor scheduled that way still runs even when visitCompilationUnit returns the original tree.
• Two defects present on current main are left unfixed, because the new check reports true for neither: - The recipe adds no import java.util.Base64; when the file has no coder variable for ChangeType to retype. The check does not visit imports, and nothing else in such a file is flagged, so it is still migrated, still without the import.
• The recipe drops the receiver of a call it rewrites when that receiver is not a J.Identifier, because it puts the original receiver back only for identifiers. factory().encode(b) is still rewritten to Base64.getEncoder().encodeToString(b) and the call to factory() is lost. The check accepts it, because factory() is typed BASE64Encoder, which ChangeType does retype.

Fixing either one is a separate change.

Have you considered any alternatives or workarounds?

A skipped file is now skipped silently. One alternative is to report every skip with Markup.warn, as visitCompilationUnit already does when alreadyUsingIncompatibleBase64 finds a class named Base64 that is not java.util.Base64, with a message ending "Manual intervention required." That would tell users which files still need manual work, but it would also mark every skipped file in the diff with a /*~~(...)~~>*/ comment, which is noisy on a codebase with many sun.misc call sites. The switch is one line in visitCompilationUnit, plus an updated expected value in the six new tests that expect their input file back unchanged. Tell me which you prefer and I will change it.

Any additional context

This change adds 7 tests to UseJavaUtilBase64Test, bringing that class to 10. Without the code change in this pull request, these 6 tests fail:

• unsupportedLegacyOverloadsLeaveTheCompilationUnitAlone
• oneUnsupportedOverloadSuppressesTheSupportedRewritesInTheSameFile
• methodReferenceToLegacyCoderLeavesTheCompilationUnitAlone
• receiverDeclaredAsLegacySupertypeLeavesTheCompilationUnitAlone
• subclassOfLegacyEncoderLeavesTheCompilationUnitAlone
• anonymousSubclassOfLegacyDecoderLeavesTheCompilationUnitAlone

The seventh test, stillMigratesHelperMethodWithLegacyEncoderParameter, passes either way. It covers a helper method whose parameter is declared BASE64Encoder, which is still migrated.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

I ran the formatter with the repository's .editorconfig. It also wanted to re-indent lines that this change does not touch, so I left those alone and kept the diff limited to this change.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch