MigrateSecurityManagerMulticast: only migrate provably equivalent calls

[martinfrancois]

Suggested review order: 22 of 52 (Score: 6)
Review first: openrewrite/rewrite#8446

What's changed?

SecurityManager declares two overloads of checkMulticast: the deprecated checkMulticast(InetAddress maddr, byte ttl), whose second parameter is a time to live, and checkMulticast(InetAddress maddr), which has none. MigrateSecurityManagerMulticast rewrites a call to the first into a call to the second, so the time to live argument disappears from the source and is no longer evaluated at run time.

The recipe now performs that rewrite only when both of the following hold, and otherwise leaves the call exactly as it is:

• The receiver is a plain new SecurityManager() allocation, with no anonymous class body, so that virtual dispatch cannot reach an override of either overload in a subclass.
• The discarded argument is a literal, a read of a local variable or of a parameter of primitive type, or either of those under one or more casts to a primitive type. Both (byte) 100 and (byte) localTtl are accepted, and a cast is never required.

A second, smaller change concerns the method type attached to a migrated call. On main that type is synthesised by trimming the two argument overload's parameter names and types down to one, so the call keeps that overload's remaining data, including its deprecation flag, and anything inspecting it later still sees a deprecated method. The recipe now looks the declared checkMulticast(InetAddress) overload up on the declaring type, the way StringFormatted does in this repository, and leaves the call unchanged when it finds none. It touches the same few lines, so it is bundled here; say the word and I will split it out. The recipe description now states the new limits.

What's your motivation?

Recipe: org.openrewrite.java.migrate.lang.MigrateSecurityManagerMulticast.

Before

manager.checkMulticast(address, ttls[index++]);

Actual after the recipe

manager.checkMulticast(address);

Expected after the recipe

(unchanged)

The two overloads are different virtual methods, and on main the recipe replaces the first with the second on every match, with no check on the receiver and no check on the argument. The rewritten code still compiles, and the program behaves differently in three ways:

• A side effect of evaluating the discarded argument no longer happens, so a counter it incremented is no longer incremented.
• An exception that evaluating the discarded argument threw is no longer thrown, whether that is an ArrayIndexOutOfBoundsException from an array access or a NullPointerException from unboxing a null Byte.
• Where the receiver holds a SecurityManager subclass that overrides only the two argument form, that override no longer runs, so a project's own security check is skipped.

Reproduced on 3.41.0 and on 3.42.0-SNAPSHOT built from main. The recipe ships inside JavaLangAPIs, so it also runs as part of Java8toJava11, UpgradeToJava17, UpgradeToJava21 and UpgradeToJava25.

Confirmed real-world executions

All three executions used org.openrewrite.recipe:rewrite-migrate-java:3.42.0.

Project	Stars on 2026-08-16	Location
Dragonwell 8	4,317	MulticastSocket.java at adf16cc8
Tencent Kona 8	1,004	MulticastSocket.java at f9d1ef52
Bytecoder	940	NetMulticastSocket.java at 67bb3596

The released recipe removes the ttl argument from each two-argument call. These calls use a variable receiver, so the replacement changes overload dispatch and stops evaluating the discarded argument.

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

One pre-existing test changed. migrateCheckMulticast, the @DocumentExample, gained throws Exception in both its before and after sources: both declared public void method() { while calling InetAddress.getByName, which declares the checked exception UnknownHostException, so javac rejects either source compiled on its own. The test framework type attributes these snippets rather than running flow analysis over them, which is why the test passed on main all the same, so that defect is pre-existing in the test source. What the test asserts is otherwise unchanged, and that case still migrates.

examples.yml and recipes.csv are generated files checked into the repository, and their changed lines are regenerated content: the same throws Exception in the @DocumentExample sources, and the single row for this recipe, carrying the extended recipe description.

Limits this creates, all cases that main migrated and this branch leaves unchanged:

• Calls through a variable, a field or System.getSecurityManager() no longer migrate, since only a direct new SecurityManager() receiver satisfies the first condition.
• A super.checkMulticast(maddr, ttl) call inside a SecurityManager subclass, covered by retainCallDelegatingToSuper, is never migrated either, because its receiver is super rather than an allocation. A super call is not dispatched virtually, so migrating it would in fact have been safe. That is an accepted cost of the deliberately coarse receiver condition, not a defect in it.
• A variable of type Byte and a static final byte constant field are both left unmigrated: Byte is not a primitive type, and a field is neither a local variable nor a parameter. Instance fields, including volatile ones, likewise.

Have you considered any alternatives or workarounds?

The receiver condition is what makes the recipe narrow. Three options, any of which I will take:

1. Keep this pull request as it is, with both conditions.
2. Drop the receiver condition and keep only the argument check, if a SecurityManager subclass that
   overrides only one of the two overloads is too rare to protect against. That is deleting the single cannotDispatchToAnOverride(m.getSelect()) && condition at MigrateSecurityManagerMulticast.java:62 and the four tests covering it: retainCallWhenReceiverMayBeASubclass, retainCallWhenAnOverloadIsOverridden, retainCallDelegatingToSuper and retainCallOnAnonymousSubclass.
3. Retire the recipe altogether, since SecurityManager is deprecated for removal.

Tell me which and I will change this pull request.

Any additional context

Pre-existing tests changed: MigrateSecurityManagerMulticastTest.java.migrateCheckMulticast (updated).

This change adds 6 test methods to MigrateSecurityManagerMulticastTest, taking the focused class from 1 to 7 executions. They cover 13 scenarios. Without the code change in this pull request, 5 added methods fail and represent 12 failing scenarios. Four methods cover receivers that may dispatch to an override. retainPotentiallyObservableArguments contains 8 discarded-argument shapes, including mutation, unboxing, field reads, conditionals, switch expressions, division and allocation. migrateWhenReceiverIsExactAndTimeToLiveIsConstantOrLocal passes either way and pins down the cases that this change keeps migrating.

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