Java 10+ Local-Variable Type Inference should be used RSPEC-6212

[yeikel]

This is controversial and the Sonar team had to remove it from the default "Sonar Way" profile

https://rules.sonarsource.com/java/tag/java10/RSPEC-6212

[tkvangorder]

This would be straightforward to implement with OpenRewrite, it can exist as a standalone recipe. This leverage UsesJavaVersion in its applicability test.

[MBoegers]

I'd like to work on this one and would like to clarify a few things.

To be sure, do we want to implement RSPEC-6212 100% Sonar like? I would argue against it because it also suggests using var for primitives, which is not recommended, as you can see in Project Ambers Local Variable Type Inference - G7.

[MBoegers]

Things I thought about, anything to add?

1. transform local Variables like: MyClass myClass = new MyClass(); to var myClass = new MyClass();
2. keep null assignments like String line = null
3. special treatment for generics and the diamond operator List<String> lines = new ArrayList<>() to var lines = new ArrayList<String>(), keep also var list = List.of() in mind
4. We depend on prior good naming, otherwise some information will be lost. But I think that's out of scope here, right?

[timtebeek]

Thank you for your offer to help out on this one! I think you have a decent list of restrictions in place already to limit applicability. I'd indeed say not to use this for primitives, and even then it's still not always a clear improvement, which makes it a tricky recipe to recommend.

Might be good to take into account the upcoming changes for OpenRewrite 8.0, which will have change the applicability tests for instance.

Maybe we can start with a suite of unit tests and do a round of feedback on that? Then we can more clearly decide which cases are worth covering, before we look into the finer details of the recipe implementation. Feel free to to open a draft pull request with what cases you would (not) want to cover as part of this work!

[yeikel]

I'd like to work on this one and would like to clarify a few things.

To be sure, do we want to implement RSPEC-6212 100% Sonar like? I would argue against it because it also suggests using var for primitives, which is not recommended, as you can see in Project Ambers Local Variable Type Inference - G7.

boolean seems to be an exception for this rule, right? (so not all primitives are excluded)

[MBoegers]

boolean seems to be an exception

Good point, not all usages of primitives are bad.

• boolean, char and String are okay
• float, double and long are okay, iff initialized with correct typed var aLong = 1.0L; var aFloat = 1.0F; var aDouble = 1.0; var otherDouble = 1.0D
• short and byte are dangerous in most cases
• reuse for initialization is dangerous static final float INITIAL = 3.0f; /*...*/ double temp = INITIAL; is possible but with var inferred as float

So for primitives I suggest:

IF String, boolean, char, float with 'F', long with 'L', double THEN
   apply var transformation
IF shot, byte THEN
   do not apply var transformation
IF initalized by variable AND left hand side type matches THEN
    apply var transformation

[MBoegers]

What about not correctly used generics?
List<> strs = new ArrayList<>(); is totally okay, if you really want a List<Object> :D same for List strs = new ArrayList();
It may occur in refactored/migrated code from older java versions.
Do no harm does not apply, because it's already there.
I can think of:

1. print a warning and transform
2. print a warning and don't transform
3. transform to explicit version var strs = new ArrayList<Object>()
4. fail the recipe

Maybe via configuration?

[timtebeek]

What about not correctly used generics? List<> strs = new ArrayList<>(); is totally okay, if you really want a List<Object> :D same for List strs = new ArrayList(); It may occur in refactored/migrated code from older java versions. Do no harm does not apply, because it's already there.

I think it might be best to keep this recipe focused on applying var only where it makes sense and does not introduce ambiguity, and to back off from making changes in the face of uncertainty about what was originally intended. We could still add tests here, and have those assert that we do not make changes (yet).

Instead it's probably better to surface other "sub-optimal" code patterns such as incorrect generics, in a separate recipe, and give users the choice there what they want to do: find only with a marker, or transform in some way. Probably a find-only recipe could be helpful to start with, then run that through Moderne on a broad list of projects, and deduce possible remediations from there, if and only if broadly applicable.

Does that sounds like a decent approach for this one?

[timtebeek]

Closed #218 just now, which covers some of the above; we can continue new developments in #237, or other similar new issues.