UseEnumSetOf is too opinionated for Spring Boot / Java upgrades

[DidierLoiseau]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v8.16.1
• Maven plugin v6.25.0
• rewrite-recipe-bom 3.20.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project. I’m performing the Spring Boot 3.5 upgrade.

What is the smallest, simplest way to reproduce the problem?

class A {
    Set<DayOfWeek> days = Set.of();
}

What did you expect to see?

The code should stay unchanged.

What did you see instead?

The code becomes

class A {
    Set<DayOfWeek> days = EnumSet.noneOf(DayOfWeek.class);
}

I had declared an immutable Set, and now it became mutable. Moreover the benefit of the code change is debatable: less readable, and most likely without performance benefit (especially for the empty set).

Since this recipe implements an opinionated choice, it shouldn’t be executed as part of Spring Boot upgrades nor Java upgrades.

What is the full stack trace of any errors you encountered?

[WARNING] Changes have been made to src\main\java\…\A.java by:
[WARNING]         org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_5
[WARNING]             org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_4
[WARNING]                 org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_3
[WARNING]                     org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2
[WARNING]                         org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_1
[WARNING]                             org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_0
[WARNING]                                 org.openrewrite.java.migrate.UpgradeToJava17
[WARNING]                                     org.openrewrite.java.migrate.Java8toJava11
[WARNING]                                         org.openrewrite.java.migrate.UpgradeToJava8
[WARNING]                                             org.openrewrite.java.migrate.UpgradeToJava7
[WARNING]                                                 org.openrewrite.java.migrate.UpgradeToJava6
[WARNING]                                                     org.openrewrite.java.migrate.util.UseEnumSetOf

Are you interested in contributing a fix to OpenRewrite?

I could remove the recipe from UpgradeToJava6. It seems this change was introduced in e18534b mid-October by @timtebeek.

[timtebeek]

Thanks! Looks like you disagree with the folks at Spotify with regards to handling empty sets.

• Rewrite of empty Set.of() by UseEnumSetOf doesn't compile #490

So I've done what we have elsewhere as well: retain the old default for that recipe, but disable it when included in a larger composite:

• Avoid opinionated empty EnumSet where included with Java 6+ upgrades #959

I hope you'll agree that strikes the right balance for the empty sets case you've shared, while still seeing the benefits elsewhere.

[DidierLoiseau]

Thanks @timtebeek for the quick answer!

I think in #490, they did not really have a preference on whether or not to convert Set.of() specifically:

Either use EnumSet.noneOf or do not do any rewrite

Indeed there is probably no benefit of UseEnumSetOf in converting Set.of() to EnumSet.noneOf(…), but I did not want to focus specifically on that case – I only provided a minimal example that used it.

My point was that in general converting Set.of(…) to EnumSet.of(…) is opinionated:

• it drops immutability
• it does not improve readability
• it is unlikely to lead to any performance improvement (I did not find any article but I think one needs proof before applying such changes)

so it should not be included by default in the Java migrations (side note: Set.of(…) was introduced in Java 9, not 6).

[timtebeek]

You make a compelling case; removed from now and documented why with a link back here. Thanks for nudging for improvements!

• 7592e24