Bug Fix deprecations, unchecked operations, and other warnings

[Veracity]

KoLmafia's codebase has a ton of crufty code. Some of it is simply because Java has advanced a lot in the last 15 years and for a long time, we restricted the source to a very old version. We have upgraded a lot of code, but a lot remains untouched.

If we compile with "lint" checking, we get warnings for:

[rawtypes] not using generics on declarations
[unchecked] operating on such variables
[fallthrough] in switch statements
[cast] redundant casts
[serial] serializable classes with no definition of serialVersionUID
[deprecation] usage of methods or fields (eventually) to be removed
[static] calling a static method as if it were an instance method

For testing, I changed the "compile" target in build.xml to onclude this:

		<javac
			compiler="modern"
			source="14"
			target="14"
			srcdir="${src}"
			destdir="${build}"
			classpath="${build}"
			failonerror="false"
			debug="on"
			debuglevel="lines,vars,source"
			deprecation="on"
			encoding="utf-8"
			nowarn="off"
			includeantruntime="false"
			errorProperty="compile.failed">
                        <compilerarg value="-Xlint"/>
                        <compilerarg value="-Xmaxwarns"/>
                        <compilerarg value="5000"/>
		</javac>

That is used for files in the "src" tree. I also tried it for the "lib" tree and saw a bunch of things I don't currently feel like fixing.

I ended up with over 2600 warnings.

I spend the afternoon grinding through some of the packages. It is super tedious. When I submit my changes, we'll be down to 2267 warnings.

I found at least one genuine bug: TravelingTraderRequest.java:

	private static final Map<Integer, Integer> buyPrices = CoinmastersDatabase.getNewMap();
...
		Map prices = TravelingTraderRequest.buyPrices;
...
			prices.put( cname, price );

cname was a string, but the map key needed to be an integer.

I'll submit this cleaned up code and let it bake over night. Hopefully I didn't introduce any bugs, but we'll see soon enough, I expect.

 

[Veracity]

Revision 20304 has all those fixes. 2267 warnings remain in Java 14.

View attachment compile-warnings.txt

 

[gausie]

Ooh exciting. I secretly love grunt work for cleaning up squiggly yellow lines in a codebase so I'll get on it too

 

[fronobulax]

I'll gladly respond to specific tasking. I will try and pass my initiative roll and become self-tasking.

 

[Veracity]

Revision 20306 eliminates all the "[static]" warnings - calling a static method (like Interpreter.isTracing) as if it were an instance method.
Thus: interpreter.isTracing() -> Interpreter.isTracing()

Frono, if you want some specific tasking, how about if you start with

swingui/menu/ScriptMRUList.java
swingui/menu/PartialMRUList.java

since you like MRU lists, if I recall.

I'm inclined to ignore the [serial] warnings, for now. We are not serializing GUI elements.

For [fallthrough], you can supposedly use @SuppressWarnings("fallthrough") on an element - which might be a switch statement or a method. If so, I'm not entirely happy with that, sinced we have very large switch statements (in ChoiceManager or UseItemRequest, for example) where we might want judicious use of case labels falling through, but where we'd like newly added cases to be protected by the warning if they accidentally fall through. Not sure you can do that. Will research.

I'm going to look at all the warnings in ASH now.

 

[fronobulax]

Frono, if you want some specific tasking, how about if you start with

swingui/menu/ScriptMRUList.java
swingui/menu/PartialMRUList.java

Partial wasn't mine but I think I figured it out anyway

 

[MCroft]

Is this still a project? I re-ran the lint build and got 1986 warnings in src (476 are serial, so we can ignore.)

What are the priorities? I was also going through this article on modernizing old code, and I concluded that most of what it wants is good to do if I'm otherwise touching old code, but I probably don't need to search-and-replace for loops with enhanced for loops...

Anyway, here's a summary of the warnings by type (mostly):

  1  [deprecation] getModifiers() in InputEvent has been deprecated
   1  [deprecation] getXmlAsString(TagNode) in XmlSerializer has been deprecated
   1  [deprecation] newInstance() in Class has been deprecated
   2  [deprecation] Float(float) in Float has been deprecated
   3  [deprecation] Character(char) in Character has been deprecated
 21  [deprecation] getSelectedValues() in JList has been deprecated

   1  [overrides] Class Candy overrides equals, but neither it nor any superclass overrides hashCode method
   1  [overrides] Class UseItemEnqueuePanel.ConsumableComparator overrides equals, but neither it nor any superclass overrides hashCode method
   1  [overrides] Class UseItemEnqueuePanel.PotionComparator overrides equals, but neither it nor any superclass overrides hashCode method

   1  [static] static method should be qualified by type name, CargoCultistShortsRequest, instead of by an expression
   1  [static] static method should be qualified by type name, GearChangeFrame, instead of by an expression

  34  [fallthrough] possible fall-through into case

 106  [cast] redundant cast to <something or other>

 755  [rawtypes] found raw type
 
 572  [unchecked] unchecked call/cast/conversion

compile task output as of R20493 View attachment buildout.txt

 

[fronobulax]

I didn't think of it as a project as much as expanded permission to fix things if I was already there.

I use an IntelliJ command (Analyze?) rather than compiler switches to see what needs to be done. The compiler list is more comprehensive.

I personally will correct unnecessary static/final type declarations, things that are local to a class and declared but not used, String compares, StringBuffer/Builder, redundant casts and a couple of other language features that have evolved. I routinely do a few other things but I am not recalling them.

Of your list, I would fix uses of deprecated things, knowing that needs to be done on a case by case basis and not hack and slash without inspection and the overrides. Those will need to be dealt with eventually if we don't do it now. I have not seen it in KoLmafia but if you override equals there is actually a contract that a lot of coders don't know about, or they ignore, and on other projects that has created some difficult to track down bugs.

 

[MCroft]

I didn't think of it as a project as much as expanded permission to fix things if I was already there.

I have not seen it in KoLmafia but if you override equals there is actually a contract that a lot of coders don't know about, or they ignore, and on other projects that has created some difficult to track down bugs.

I think that’s a good distinction for the difference berween explicitly fixing deprecations and opportunistically fixing them as we work.

I read up on overriding equals and it seems like there’s at least an inssue if you don’t also override hashcode because your keys might hash differently and that would be, as you say, hard to debug.