[SOLR-13658] Precommit fail Java "var" until 9x. Fail "var...<>" constructs entirely

• Type: Wish
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 8.2, 9.0
• Fix Version/s: 8.3
• Assignee: Erick Erickson
• Reporter: Erick Erickson
• Created: 29/Jul/19 15:31
• Updated: 21/Jan/20 16:57
• Resolved: 11/Aug/19 15:55

Description

Personally, I'm strongly against allowing the "var" construct in Lucene/Solr code. I think it's a wonderful opportunity to introduce bugs that won't be found until runtime as well as making maintainence significantly harder. I don't even think for a project like Solr it would save any time overall...

So let's discuss this ahead of time and see if we can reach a consensus. I'll start the discussion off:

My baseline argument is that for a large complex project, especially ones with many different people coding, I want the compiler to give me all the help possible. And the "var" construct takes away some of that help.

I’ve seen this argument go around at least 4 times in my career. The argument that “it takes longer to write if you have to type all this stuff” is bogus. Last I knew, 80% of the time spent is in maintaining/reading it. So the argument “I can write faster” means I can save some fraction of the 20% of the time writing the original code but spend many times that figuring out what the code is actually doing the other 80% of the time.

The IDE makes writing this slightly faster, admittedly.

Whatever what = new Whatever();
var kidding = what.getComplex();
var blivet = kidding.get("stuff");

But once that’s done, if I’m reading the code again I don't have any clue what

kidding or blivet

are. Here's the signature for getComplex:

Map<String, Map<Integer, Integer>> getComplex()

I have to go over to the definition (which I admit is easier than it used to be in the bad old days, but still) to find out.

HERE'S THE PART I REALLY OBJECT TO!

The above I could probably live with, maybe we could get the InteliJ developers and see if they can make hover show the inference. What I will kick and scream about is introducing bugs that are not found until runtime. Even this obvious stupidity fails with a ClassCastException:

var corny = new TreeMap<String, String>();
corny.put("one", "two");
corny.get(1);

But it's much worse when using classes from somewhere else. For instance, change the underlying class in the first example to return

Map<String, Map<String, Integer>>

.
This code that used to work now throws an error, but it compiles.

var kidding = what.getComplex();
var blivet = kidding.get("stuff");
var blah = kidding.get("stuff").get(1); //  generates ClassCastException: class java.lang.String cannot be cast to class java.lang.Integer

So in order to save some time writing (that I claim will be lost multiple times over when maintaining the code) we'll introduce run-time errors that will take a bunch more time to figure out, and won’t be found during unit tests unless and until we have complete code coverage.

If there's a way to insure that this kind of thing can't get into the code and we implement that, I could be persuaded, but let's make that an explicit requirement (and find a suitable task for the build system, precommit or whatever).

The floor is open...

Issue Links

• contains: SOLR-13671 Remove check for bare "var" declarations in validate-source-patterns in before releasing Solr 9.0