From 6439b8a51252224dc6795806adf41bdac2d754b6 Mon Sep 17 00:00:00 2001 From: Vest Date: Thu, 25 Jun 2026 22:13:58 +0200 Subject: [PATCH 1/2] SpellCasterChoiceSet: delete asymmetric equals/hashCode, inherit parent Parent ChoiceSet.equals accepts any ChoiceSet (via pattern matching on the parent type) and compares setName + pcs. The subclass override required the other side to be a SpellCasterChoiceSet, so the equality relation was not symmetric: parentChoiceSet.equals(spellCaster) -> true (setName + pcs match) spellCaster.equals(parentChoiceSet) -> false (rejected by instanceof) hashCode was likewise inconsistent: parent hashes setName + pcs, subclass hashed types + primitives. Equal-by-parent objects could end up in different buckets. The subclass's types / primitives fields are not part of identity beyond what the parent already captures via the typePCS passed to super(...), so the right pattern (already used by the sibling ChoiceSet.AbilityChoiceSet in a7cd6c1504 / #7628) is to delete the override and inherit. The class gets @SuppressFBWarnings(EQ_DOESNT_OVERRIDE_EQUALS) with the rationale. Adds SpellCasterChoiceSetEqualsTest pinning the symmetry contract; the test would have failed against the pre-fix override. --- .../cdom/choiceset/SpellCasterChoiceSet.java | 45 +------ .../SpellCasterChoiceSetEqualsTest.java | 121 ++++++++++++++++++ 2 files changed, 126 insertions(+), 40 deletions(-) create mode 100644 code/src/test/pcgen/cdom/choiceset/SpellCasterChoiceSetEqualsTest.java diff --git a/code/src/java/pcgen/cdom/choiceset/SpellCasterChoiceSet.java b/code/src/java/pcgen/cdom/choiceset/SpellCasterChoiceSet.java index 6a92e9af275..cfff07680ad 100644 --- a/code/src/java/pcgen/cdom/choiceset/SpellCasterChoiceSet.java +++ b/code/src/java/pcgen/cdom/choiceset/SpellCasterChoiceSet.java @@ -39,12 +39,15 @@ /** * A SpellCasterChoiceSet contains references to PCClass Objects. - * + * * The contents of a SpellCasterChoiceSet is defined at construction of the * SpellCasterChoiceSet. The contents of a SpellCasterChoiceSet is fixed, and * will not vary by the PlayerCharacter used to resolve the * SpellCasterChoiceSet. */ +@SuppressFBWarnings(value = "EQ_DOESNT_OVERRIDE_EQUALS", + justification = "Parent ChoiceSet.equals compares setName + pcs; pcs here is the typePCS passed to super, " + + "which captures the relevant identity. Overriding equals here would break symmetry with the parent.") public class SpellCasterChoiceSet extends ChoiceSet implements PrimitiveChoiceSet { @@ -273,49 +276,11 @@ public Set getSet(PlayerCharacter pc) return returnSet; } - @Override - public int hashCode() - { - return (types == null ? 0 : types.hashCode() * 29) + (primitives == null ? 0 : primitives.hashCode()); - } - - @Override - public boolean equals(Object obj) - { - if (obj == this) - { - return true; - } - if (obj instanceof SpellCasterChoiceSet other) - { - if (types == null) - { - if (other.types != null) - { - return false; - } - } - else - { - if (!types.equals(other.types)) - { - return false; - } - } - if (primitives == null) - { - return other.primitives == null; - } - return primitives.equals(other.primitives); - } - return false; - } - /** * Returns the GroupingState for this SpellCasterChoiceSet. The * GroupingState indicates how this SpellCasterChoiceSet can be combined * with other PrimitiveChoiceSets. - * + * * @return The GroupingState for this SpellCasterChoiceSet. */ @Override diff --git a/code/src/test/pcgen/cdom/choiceset/SpellCasterChoiceSetEqualsTest.java b/code/src/test/pcgen/cdom/choiceset/SpellCasterChoiceSetEqualsTest.java new file mode 100644 index 00000000000..d34683fda4e --- /dev/null +++ b/code/src/test/pcgen/cdom/choiceset/SpellCasterChoiceSetEqualsTest.java @@ -0,0 +1,121 @@ +/* + * Copyright 2026 Vest + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the + * Free Software Foundation; either version 2.1 of the License, or (at your + * option) any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this library; if not, write to the Free Software Foundation, + * Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ +package pcgen.cdom.choiceset; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.Set; + +import pcgen.cdom.base.ChoiceSet; +import pcgen.cdom.base.PrimitiveChoiceSet; +import pcgen.cdom.enumeration.GroupingState; +import pcgen.core.PCClass; +import pcgen.core.PlayerCharacter; + +import org.junit.jupiter.api.Test; + +/** + * Regression test for the SpotBugs EQ_OVERRIDING_EQUALS_NOT_SYMMETRIC finding + * that previously affected {@link SpellCasterChoiceSet}. Before the fix the + * subclass overrode {@code equals} to require {@code SpellCasterChoiceSet} on + * the other side, which broke symmetry with {@link ChoiceSet#equals(Object)}. + * The fix removes that override and inherits the parent equality. + */ +class SpellCasterChoiceSetEqualsTest +{ + + /** + * Minimal stub PrimitiveChoiceSet identified by name; equals compares names. + * Suitable for use as both the parent ChoiceSet's pcs and the subclass's + * typePCS so that both ChoiceSets share the same {@code pcs} reference. + */ + private static PrimitiveChoiceSet stubPcs(String name) + { + return new PrimitiveChoiceSet<>() + { + @Override + public Class getChoiceClass() + { + return PCClass.class; + } + + @Override + public String getLSTformat(boolean useAny) + { + return name; + } + + @Override + public Set getSet(PlayerCharacter pc) + { + return Collections.emptySet(); + } + + @Override + public GroupingState getGroupingState() + { + return GroupingState.ANY; + } + + @Override + public boolean equals(Object o) + { + return (o instanceof PrimitiveChoiceSet) && name.equals(((PrimitiveChoiceSet) o).getLSTformat(true)); + } + + @Override + public int hashCode() + { + return name.hashCode(); + } + }; + } + + @Test + void parentAndSpellCasterAreSymmetricWhenSharingNameAndPcs() + { + PrimitiveChoiceSet sharedPcs = stubPcs("CASTERS"); + + // Parent ChoiceSet, name + pcs only — the identity ChoiceSet.equals checks. + ChoiceSet plainSet = new ChoiceSet<>("SPELLCASTER", sharedPcs); + + // SpellCasterChoiceSet that passes the same pcs to super(...) so the + // parent-visible identity matches. + SpellCasterChoiceSet casterSet = + new SpellCasterChoiceSet(null, List.of(), sharedPcs, null); + + // Symmetry of equals: both directions must agree (used to fail). + assertEquals(plainSet.equals(casterSet), casterSet.equals(plainSet), + "ChoiceSet.equals(SpellCasterChoiceSet) must equal SpellCasterChoiceSet.equals(ChoiceSet)"); + + // And the parent-defined identity says they are equal. + assertTrue(plainSet.equals(casterSet), "parent considers them equal (same setName + pcs)"); + assertTrue(casterSet.equals(plainSet), "subclass must agree (symmetry)"); + + // Objects.equals goes through the receiver-side equals; verify both routes. + assertEquals(Objects.equals(plainSet, casterSet), Objects.equals(casterSet, plainSet)); + + // hashCode contract: equal objects must hash the same. + assertEquals(plainSet.hashCode(), casterSet.hashCode(), + "equal ChoiceSet/SpellCasterChoiceSet must hash equally"); + } +} From b459385700da9d1f13f36f781daa2606a25d09e2 Mon Sep 17 00:00:00 2001 From: Vest Date: Thu, 25 Jun 2026 22:14:19 +0200 Subject: [PATCH 2/2] DescriptionActor.process: guard against null PC (and null bean) PlayerCharacterTrackingFacet.getPC documents (lines 40-47) that it may return null in unit-test paths where no PC has been associated with the CharID. SpringHelper.getBean also returns null when no bean is registered for the requested type. Both nulls used to flow straight into desc.getDescription(aPC, ...), producing an NPE downstream. SpotBugs flags this as NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE at DescriptionActor.java:71 (charStore loaded, then dereferenced at line 72 without a guard). Returns an empty wrapped string for the no-PC case, matching the existing 'no benefits' early-out a few lines above. --- code/src/java/pcgen/output/actor/DescriptionActor.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/code/src/java/pcgen/output/actor/DescriptionActor.java b/code/src/java/pcgen/output/actor/DescriptionActor.java index 1a403aed28b..5c7ccb77a95 100644 --- a/code/src/java/pcgen/output/actor/DescriptionActor.java +++ b/code/src/java/pcgen/output/actor/DescriptionActor.java @@ -69,7 +69,14 @@ public TemplateModel process(CharID id, PObject d) throws TemplateModelException return FacetLibrary.getFacet(ObjectWrapperFacet.class).wrap(id, Constants.EMPTY_STRING); } PlayerCharacterTrackingFacet charStore = SpringHelper.getBean(PlayerCharacterTrackingFacet.class); - PlayerCharacter aPC = charStore.getPC(id); + PlayerCharacter aPC = (charStore == null) ? null : charStore.getPC(id); + if (aPC == null) + { + // SpringHelper.getBean returns null when no bean is registered, and + // PlayerCharacterTrackingFacet.getPC documents that null may be returned + // in unit-test paths where no PC has been associated with the CharID. + return FacetLibrary.getFacet(ObjectWrapperFacet.class).wrap(id, Constants.EMPTY_STRING); + } final StringBuilder buf = new StringBuilder(250); boolean needSpace = false; for (final Description desc : theBenefits)