From de3308f4ae8ad7bdecb55f8ef622d6a736eb7360 Mon Sep 17 00:00:00 2001 From: Vest Date: Thu, 25 Jun 2026 22:15:45 +0200 Subject: [PATCH 1/2] Region/Type: add value-equals; DisplayLocation: suppress identity-equals Fixes 3 EQ_COMPARETO_USE_OBJECT_EQUALS and 1 SI_INSTANCE_BEFORE_FINALS_ASSIGNED finding from SpotBugs. Region: add value-based equals/hashCode matching the case-insensitive compareTo. Region is used as a HashMap, ...> key in BioSet (ageMap, userMap) and .equals() is called directly in RegionFacet.matchesRegion. The intern map in getConstant() already made two getConstant(name) calls return the same instance, so the new equals/hashCode strengthens but does not change behaviour for any existing call site. Use String.CASE_INSENSITIVE_ORDER.compare(...) == 0 in equals to satisfy the find-sec-bugs IMPROPER_UNICODE rule (PR #7626 convention); hashCode folds via toLowerCase(Locale.ROOT) for a stable, locale-independent hash consistent with equals. Region: also rearrange the private static int ordinalCount = 0 above the NONE constant declaration to silence SI_INSTANCE_BEFORE_FINALS_ASSIGNED. JLS default-initialises primitive int to 0 before any field initialiser runs, so this is purely a static-layout cleanup. Type: add value-based equals/hashCode matching the case-sensitive compareTo. Type is used as HashSet/HashMap across 8 production files. Since the intern map is CaseInsensitiveMap, two distinct Type instances with fieldNames differing only in case cannot exist, so case-sensitive equals matches compareTo and preserves existing behaviour. DisplayLocation: identity-equals is already correct (singleton-by- construction via getConstant intern map, no .equals() callers, not used as a collection key). Method-level @SuppressFBWarnings on compareTo documents that explicitly. --- .../cdom/enumeration/DisplayLocation.java | 11 +++---- .../java/pcgen/cdom/enumeration/Region.java | 31 ++++++++++++------- .../src/java/pcgen/cdom/enumeration/Type.java | 19 +++++++----- 3 files changed, 35 insertions(+), 26 deletions(-) diff --git a/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java b/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java index eebd4d886d3..ea15ed10e19 100644 --- a/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java +++ b/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.Objects; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import pcgen.base.enumeration.TypeSafeConstant; import pcgen.base.util.CaseInsensitiveMap; @@ -161,15 +162,11 @@ public static void clearConstants() } @Override + @SuppressFBWarnings(value = "EQ_COMPARETO_USE_OBJECT_EQUALS", + justification = "Singleton-by-construction via getConstant intern map; " + + "no .equals() callers, not used as a collection key. Identity equality is correct here.") public int compareTo(DisplayLocation type) { - /* - * Note: Some tools will report a problem here because Type implements - * compareTo, but does not implement custom implementations of hashCode - * or equals(). Because this is intended as a TypeSafeConstant, and Type - * has a private constructor, it is unnecessary to implement a custom - * hashCode or equals. - */ return fieldName.compareTo(type.fieldName); } diff --git a/code/src/java/pcgen/cdom/enumeration/Region.java b/code/src/java/pcgen/cdom/enumeration/Region.java index 8995713ea64..ec8c6ad91b3 100644 --- a/code/src/java/pcgen/cdom/enumeration/Region.java +++ b/code/src/java/pcgen/cdom/enumeration/Region.java @@ -20,6 +20,7 @@ import java.lang.reflect.Field; import java.util.Collection; import java.util.Collections; +import java.util.Locale; import java.util.Objects; import pcgen.base.enumeration.TypeSafeConstant; @@ -35,6 +36,11 @@ */ public final class Region implements TypeSafeConstant, Comparable { + /** + * This is used to provide a unique ordinal to each constant in this class + */ + private static int ordinalCount = 0; + /** * A "None" region for universal use. */ @@ -45,11 +51,6 @@ public final class Region implements TypeSafeConstant, Comparable */ private static CaseInsensitiveMap typeMap; - /** - * This is used to provide a unique ordinal to each constant in this class - */ - private static int ordinalCount = 0; - /** * The name of this Constant */ @@ -202,14 +203,20 @@ public static void clearConstants() @Override public int compareTo(Region region) { - /* - * Note: Some tools will report a problem here because Region implements - * compareTo, but does not implement custom implementations of hashCode - * or equals(). Because this is intended as a TypeSafeConstant, and Type - * has a private constructor, it is unnecessary to implement a custom - * hashCode or equals. - */ return fieldName.compareToIgnoreCase(region.fieldName); } + @Override + public boolean equals(Object o) + { + return o instanceof Region other + && String.CASE_INSENSITIVE_ORDER.compare(fieldName, other.fieldName) == 0; + } + + @Override + public int hashCode() + { + return fieldName.toLowerCase(Locale.ROOT).hashCode(); + } + } diff --git a/code/src/java/pcgen/cdom/enumeration/Type.java b/code/src/java/pcgen/cdom/enumeration/Type.java index 31c50e48a08..47d8a4b63b5 100644 --- a/code/src/java/pcgen/cdom/enumeration/Type.java +++ b/code/src/java/pcgen/cdom/enumeration/Type.java @@ -219,16 +219,21 @@ public static Collection getAllConstants() @Override public int compareTo(Type type) { - /* - * Note: Some tools will report a problem here because Type implements - * compareTo, but does not implement custom implementations of hashCode - * or equals(). Because this is intended as a TypeSafeConstant, and Type - * has a private constructor, it is unnecessary to implement a custom - * hashCode or equals. - */ return fieldName.compareTo(type.fieldName); } + @Override + public boolean equals(Object o) + { + return o instanceof Type other && fieldName.equals(other.fieldName); + } + + @Override + public int hashCode() + { + return fieldName.hashCode(); + } + public static void buildMap() { TYPE_MAP.clear(); From 7a8bcf5d77edfb7a58464d7060505cb75999a1e1 Mon Sep 17 00:00:00 2001 From: Vest Date: Fri, 26 Jun 2026 12:23:47 +0200 Subject: [PATCH 2/2] Region/Type: document why equals/hashCode are value-based The original commit replaced the stale 'no equals/hashCode needed because TypeSafeConstant has a private constructor' comment with the actual equals/hashCode methods, but didn't leave anything in its place. Replaces the lost archival value with a short class-level Javadoc note explaining the design intent: equals/hashCode match compareTo (Comparable contract) and support use as a HashMap key (BioSet, RegionFacet, DataSet, GameMode, etc.). Captures the *why*, not the *what*, so future readers don't have to rediscover the contract reasoning from git history. --- code/src/java/pcgen/cdom/enumeration/Region.java | 7 ++++++- code/src/java/pcgen/cdom/enumeration/Type.java | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/code/src/java/pcgen/cdom/enumeration/Region.java b/code/src/java/pcgen/cdom/enumeration/Region.java index ec8c6ad91b3..9385b60eb9c 100644 --- a/code/src/java/pcgen/cdom/enumeration/Region.java +++ b/code/src/java/pcgen/cdom/enumeration/Region.java @@ -29,10 +29,15 @@ import pcgen.cdom.base.Constants; /** - * * This Class is a Type Safe Constant. It is designed to hold Regions in a * type-safe fashion, so that they can be quickly compared and use less memory * when identical Regions exist in two CDOMObjects. + * + *

{@code equals} and {@code hashCode} are case-insensitive on + * {@code fieldName} to match {@link #compareTo} (Comparable contract) and to + * support use as a {@code HashMap} key — see {@code BioSet}'s + * {@code DoubleKeyMap, …>} and the {@code .equals()} calls + * in {@code RegionFacet.matchesRegion}.

*/ public final class Region implements TypeSafeConstant, Comparable { diff --git a/code/src/java/pcgen/cdom/enumeration/Type.java b/code/src/java/pcgen/cdom/enumeration/Type.java index 47d8a4b63b5..9f2dddfd2c4 100644 --- a/code/src/java/pcgen/cdom/enumeration/Type.java +++ b/code/src/java/pcgen/cdom/enumeration/Type.java @@ -28,10 +28,15 @@ import pcgen.cdom.base.Constants; /** - * * This Class is a Type Safe Constant. It is designed to hold Types in a * type-safe fashion, so that they can be quickly compared and use less memory * when identical Types exist in two CDOMObjects. + * + *

{@code equals} and {@code hashCode} are value-based on {@code fieldName} + * (case-sensitive, matching {@link #compareTo}). Type is used as a + * {@code HashSet}/{@code HashMap} key across the codebase (DataSet, GameMode, + * BodyStructure, spell/skill lists, …); the value-equals contract holds even + * when an instance is constructed outside the {@link #getConstant} intern map.

*/ public final class Type implements TypeSafeConstant, Comparable {