spotbugs cleanups: drop dead fields/locals + small fixes

[Vest]

Conservative SpotBugs cleanup in cdom: removes the entire URF_UNREAD_FIELD and RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE bug-pattern categories from the report, plus one DLS_DEAD_LOCAL_STORE. No behavior changes other than dropping a Spring property that injected an unused reference.

Fixes

Pattern	Site	What changed
URF_UNREAD_FIELD	CalcBonusFacet.templateFacet	Drop the field, its setter, the TemplateFacet import, and the <property name="templateFacet"/> in applicationContext.xml. The matching addDataFacetChangeListener call in init() was already commented out, so behavior was effectively dead. (See deep dive below — researched the 2013 history to confirm this was a deliberate optimization, not a forgotten line.)
URF_UNREAD_FIELD	AbstractPCGenModifier.references	Field is retained — its existing javadoc says "DO NOT DELETE THIS EVEN THOUGH IT APPEARS UNUSED. Its use is holding the references so that they are not garbage collected." Add @SuppressFBWarnings(URF_UNREAD_FIELD) with the GC-anchor rationale.
URF_UNREAD_FIELD	CharID.myFacetCache	Debugger-visible view of the facet cache, documented in the existing javadoc. Add @SuppressFBWarnings(URF_UNREAD_FIELD).
URF_UNREAD_FIELD	DataSetID.myFacetCache	Same shape as CharID. Add @SuppressFBWarnings(URF_UNREAD_FIELD).
RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE	AddedTemplateFacet.dataRemoved	Drop the null-check on getFromSource(...); its javadoc and code already guarantee a non-null collection (the dataAdded sibling already uses isEmpty()).
DLS_DEAD_LOCAL_STORE	ClassSkillChoiceActor.applyChoice	Drop the levelIndex++ immediately before the loop break; — increment never read.

Plus a follow-up commit adding a Javadoc note on CalcBonusFacet itself (see deep dive).

Why deleting templateFacet was the right call — historical deep dive

The agent's initial pass spotted templateFacet as unread and removed it, but the surrounding context (a commented-out listener registration and a 2013 commit message about "template based dragons") warranted real investigation. Here's what the git history and code audit revealed:

1. Original design (2012)

Commit ab28c61112 (thpr, 2012-02-05, "Move BonusCalculation Trigger out to facet for Race, Template, Deity") introduced CalcBonusFacet with three listeners — race, deity, template — all firing aPC.calcActiveBonuses() on add/remove. Templates were treated as a canonical recalc trigger via the facet event chain.

2. The 2013 deactivation

Commit d9a5fcaed3 (jdempsey, 2013-12-01, "Potential speedup for template based dragons") commented out the template listener:

 public void init() {
     raceFacet.addDataFacetChangeListener(5000, this);
     deityFacet.addDataFacetChangeListener(5000, this);
-    templateFacet.addDataFacetChangeListener(5000, this);
+    //templateFacet.addDataFacetChangeListener(5000, this);
 }

The "dragons" reference is to D&D dragon NPCs being template-stacked: each template add or remove was firing a full calcActiveBonuses() walk, and on dragons this could fire many times per user action.

3. Why deactivation was safe

PlayerCharacter.addTemplate already calls calcActiveBonuses() explicitly (line 4364, and has done so since well before 2013). The TemplateFacet listener was firing a second, redundant recalc on every template add — pure waste.

For removal paths, all 7 callsites of PlayerCharacter.removeTemplate either recalc explicitly afterwards (e.g. CharacterFacadeImpl:3541 theCharacter.calcActiveBonuses();) or are inside an outer event whose recalc is still wired through the raceFacet listener (AddedTemplateFacet.dataRemoved cascades from RaceFacet.dataRemoved).

removeTemplate caller	Recalc coverage
PlayerCharacter:1567,1574 (level adjust loop)	Outer routine recalcs
PlayerCharacter:7778 (subtemplate from addTemplate)	Parent addTemplate recalcs
CharacterFacadeImpl:3540	Explicit calcActiveBonuses() on next line
TemplateInfoTab	Routes through CharacterFacadeImpl — covered
AddedTemplateFacet.dataRemoved (3 sites)	Outer Race/Class/CompanionMod removal wakes raceFacet listener
TemplateLst.removeChoice / add.TemplateToken.removeChoice	Choice-manager outer flow is responsible
PCGVer2Parser (load path)	Load path ends with one calcActiveBonuses()

4. Why the cleanup is safe

The 2013 commit was correct in intent but commented out a single line instead of deleting the field, setter, import, and Spring <property>. That left 12.5 years of undead code: Spring continued injecting a TemplateFacet reference into every CalcBonusFacet instance on every load, only for that reference to go nowhere. This PR removes the dead infrastructure.

5. What we deliberately did NOT do

I initially considered adding a symmetric calcActiveBonuses() call inside removeTemplate (mirroring addTemplate). After looking, rejected: every external caller already recalcs afterwards, so the added call would either be redundant work (a performance regression for the same dragon characters the 2013 commit was helping) or require a multi-file refactor to move the explicit calls from callers into removeTemplate. The latent footgun — "new code calling templateInputFacet.add directly without recalc'ing" — is real but no current call path exhibits it.

Existing test coverage for this also wouldn't catch a missing recalc: the relevant tests (PlayerCharacterTest in slowtest) exercise addTemplate → removeTemplate cycles but assert isNonAbility state, which goes through nonAbilityFacet, not the bonus graph.

6. What we DID add

A class-Javadoc note on CalcBonusFacet (commit 4b9ba73e32) capturing the convention so future contributors don't have to reverse-engineer the 2013 git history every time:

/**
 * ...
 * Note: only registers on raceFacet today. The original 2012 design also
 * listened to TemplateFacet and DeityFacet, but those recalcs were redundant
 * and got moved to explicit callers (commits d9a5fcaed3 in 2013 for
 * templates, 73c336821d for deities). New template mutation paths must call
 * PlayerCharacter.calcActiveBonuses() themselves; PlayerCharacter.addTemplate
 * already does.
 */

Deferred (not in this PR)

Pattern	Site	Why
DLS_DEAD_LOCAL_STORE	AbilityFromClassChoiceSet.java:144	Paired with an explicit // TODO This is a bug -> it was not properly gathering before comment; touching the dead store would mask the known bug.
PZLA_PREFER_ZERO_LENGTH_ARRAYS	FactSetParser.unparse	The return null on the error path (after addWriteMessage) matches the project-wide token unparse convention; callers in TokenSupport explicitly null-check. Returning new String[0] would break parity with hundreds of other token unparse sites.

SpotBugs delta

• URF_UNREAD_FIELD: 4 -> 0 in CDOM (whole category gone from report)
• RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE: 1 -> 0 (whole category gone)
• DLS_DEAD_LOCAL_STORE: 2 -> 1 in CDOM (1 deferred)
• No new findings introduced.

Tests

• ./gradlew compileJava — BUILD SUCCESSFUL
• ./gradlew :test --tests "pcgen.cdom.*" — 2355 tests, 0 failures
• ./gradlew :test --tests "plugin.lsttokens.*" — 9713 tests, 0 failures

The lsttokens run is included because the changes touch facets and applicationContext.xml.