SpotBugs correctness: SpellCasterChoiceSet symmetry + DescriptionActor null PC#7637
Draft
Vest wants to merge 2 commits into
Draft
SpotBugs correctness: SpellCasterChoiceSet symmetry + DescriptionActor null PC#7637Vest wants to merge 2 commits into
Vest wants to merge 2 commits into
Conversation
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 a7cd6c1 / PCGen#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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two real correctness bugs surfaced during a SpotBugs investigation in the choiceset and output.actor packages.
SpellCasterChoiceSet: symmetry / hashCode contract
The parent
ChoiceSet.equalsaccepts anyChoiceSet<?>(pattern matching on the parent type) and comparessetName+pcs. The pre-fix subclass override required the other side to be aSpellCasterChoiceSet, breaking symmetry:parentChoiceSet.equals(spellCaster)returnedtruewhen setName + pcs matched.spellCaster.equals(parentChoiceSet)returnedfalse(rejected byinstanceof SpellCasterChoiceSet).hashCodewas inconsistent in the same way: parent hashedsetName ^ pcs, subclass hashedtypesandprimitives. Equal-by-parent objects could land in different hash buckets.The subclass's
types/primitivesfields are not part of identity beyond what the parent already captures via thetypePCSpassed tosuper(...). So the right pattern (already used by the siblingChoiceSet.AbilityChoiceSetin #7628 / commit a7cd6c1) is to delete the override and inherit the parent equality. The class gets@SuppressFBWarnings(EQ_DOESNT_OVERRIDE_EQUALS)with the rationale.SpellCasterChoiceSetEqualsTestpins the symmetry contract; it would have failed against the pre-fix override.DescriptionActor.process: null guard
PlayerCharacterTrackingFacet.getPCdocuments (lines 40-47) that it may returnnullin unit-test paths where no PC has been associated with the CharID.SpringHelper.getBeanalso returnsnullwhen no bean is registered for the requested type. Both nulls used to flow straight intodesc.getDescription(aPC, ...), producing an NPE downstream.SpotBugs flagged this as
NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUEat DescriptionActor.java:71. The fix early-returns an empty wrapped string for the no-PC case, matching the existing 'no benefits' early-out a few lines above.SpotBugs delta
NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE: 9 -> 8 (DescriptionActor.java gone)EQ_OVERRIDING_EQUALS_NOT_SYMMETRIC/EQ_DOESNT_OVERRIDE_EQUALS: unchanged (0)Tests
./gradlew :test --tests pcgen.cdom.* --tests pcgen.output.*-> 2356 tests, 0 failures, 0 errors.