Skip to content

Fix bare string to SetManager silently shredding into characters#240

Merged
derek73 merged 6 commits into
masterfrom
fix/issue-238-string-shredding
Jul 6, 2026
Merged

Fix bare string to SetManager silently shredding into characters#240
derek73 merged 6 commits into
masterfrom
fix/issue-238-string-shredding

Conversation

@derek73

@derek73 derek73 commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • Constants(titles='dr') silently replaced the entire default titles set with {'d', 'r'} because a bare str satisfies Iterable[str] — as an iterable of its characters — and SetManager.__init__ fed it straight to set(); names then parsed wrong with no error anywhere (closes Constants(titles='dr') silently shreds a bare string into characters #238)
  • SetManager now validates and normalizes at every entry point — constructor, all set operators (and reflections), with add()/remove() already covered:
    • Bare str/bytes raise TypeError with an actionable hint (wrap it in a list: ['dr']; bytes get decode it first: [b'dr'.decode()] since a wrapped bytes element would just be a quieter version of the same dead-config bug)
    • The operator paths needed their own guard: the ABC mixins' __or__/__and__ hand _from_iterable a generator, so c.titles |= 'esq' bypassed the constructor guard and silently added 'e', 's', 'q' as titles
    • Operands and constructor elements get the same lc() normalization as add() (lowercase, strip leading/trailing periods) — titles | ['Esq.'] kept a raw 'Esq.' that parser lookups could never match, titles & ['Dr.'] missed the stored 'dr', and Constants(titles=[...]) stored raw elements that silently never matched (this operator bug existed in released v1.2.1, not just on this branch)
    • Non-str elements (bytes, None, numbers) raise a curated TypeError instead of crashing context-free inside lc() or being silently transmuted (None became '')
  • This has to be a runtime check — the type system can't distinguish str from Iterable[str]

Performance

The naive implementation (mixin delegation + per-construction validation) made HumanName(constants=None) 6.8× slower than master. Final shape restores master parity (Constants() 8.2µs, HumanName(constants=None) 58µs): operators build results with plain set ops via a private _from_normalized constructor, _normalized_elements short-circuits already-normalized SetManager operands, and the nine default config sets are validated once at import and copied on identity-checked defaults.

Safety

  • All internal SetManager(...) call sites pass module-level tuples/sets (verified by grep); the shipped defaults are already lc()-normalized, so constructor normalization is a no-op for them
  • Pickle/deepcopy restore SetManager by state without re-running __init__, so round-trips are unaffected
  • Entries with internal periods ('univ.prof', 'ph.d') survive normalization — lc() strips leading/trailing periods only

Test plan

  • All behavior TDD'd with verified-failing tests: bare-string kwarg/constructor/operand rejection (all six operator spellings), bytes decode hint, non-str element rejection, constructor + operand normalization for all seven operator methods (__rxor__ pinned separately), and end-to-end parses proving operator-built and kwarg-built sets behave like add()-built ones
  • Full suite 1380 passed / 22 xfailed, mypy clean, ruff clean, Sphinx doctest build clean

Follow-up issues filed from review findings

🤖 Generated with Claude Code

Every set-backed Constants argument is annotated Iterable[str], which a
bare str satisfies — as an iterable of its characters. SetManager's
set(elements) then silently replaced the whole default set with single
characters (Constants(titles='dr') -> titles == {'d', 'r'}), producing
wrong parses with no error anywhere. The type system cannot catch this,
so it must be a runtime check.

Reject str and bytes in SetManager.__init__ with a TypeError that
includes the wrap-it-in-a-list fix, covering all nine set-backed
Constants arguments and direct SetManager construction in one place.
All internal call sites pass module-level tuples/sets, and pickling
restores SetManager by state without re-running __init__, so only the
buggy input path is affected.

Closes #238

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Jul 6, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.40%. Comparing base (192d35e) to head (3c705d4).
⚠️ Report is 1 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   97.26%   97.40%   +0.14%     
==========================================
  Files          13       13              
  Lines         805      849      +44     
==========================================
+ Hits          783      827      +44     
  Misses         22       22              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@derek73 derek73 self-assigned this Jul 6, 2026
@derek73 derek73 added the bug label Jul 6, 2026
@derek73 derek73 added this to the v1.3.0 milestone Jul 6, 2026
The Set mixin's __or__/__and__ hand _from_iterable a generator, so the
new __init__ guard never saw a bare-string operand: c.titles |= 'esq'
still silently added 'e', 's', 'q' as titles, while __sub__/__xor__
raised only by accident of constructing from the operand. Route all
four operators (and reflections) through a shared bare-string check.
typeshed declares narrower AbstractSet operands and omits the runtime
ABC's __rsub__, hence the targeted type ignores.

The bytes branch of the error hint was itself a trap: following "wrap
it in a list: [b'dr']" produces a set whose bytes elements never match
parsed str tokens — another silent failure. bytes now get "decode it
first: [b'dr'.decode()]".

Also correct the SetManager class docstring's "Only special
functionality" claim (the guards are user-visible API now) and drop
add()'s false "Can pass a list of strings" (that raises
AttributeError).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Set mixin operators build results from raw operand elements and
test them against the stored (normalized) ones, so even with the
bare-string guard, (titles | ['Esq.']) kept a raw 'Esq.' the parser's
lc() lookups could never match, and (titles & ['Dr.']) missed 'dr' —
the same silently-broken-config family as #238, one step from the
guarded case. Operands now pass through lc() in the existing operator
overrides, so operator results obey the same normalized-elements
invariant as add()-built sets. Idempotent for already-normalized
operands, so the internal suffixes_prefixes_titles union is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on the operand-normalization commit: the constructor
still stored raw elements, which that commit turned from a latent
inconsistency into active misfires — SetManager(['Dr.']) & ['Dr.']
returned empty and - silently no-opped against the exact spelling
stored in the set, Constants(titles=[...]) remained the last
silently-dead config path (raw 'Chemistry' can never match the
parser's lc() lookups), and titles vs the suffixes_prefixes_titles
union disagreed about the same token.

Fold the operand helper into _normalized_elements, shared by __init__
and all operators, so every entry is stored in the form lookups expect.
Junk elements now raise a curated TypeError instead of failing
cryptically inside lc() (bytes, int) or being silently transmuted
(None became '').

Also scope the docstring claim that operators normalize "the same way
add() does" (add() additionally decodes bytes), correct "strips
periods" to leading/trailing in prose, and pin __rxor__ separately.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
derek73 and others added 2 commits July 5, 2026 19:12
The operand-normalization work left a compounding perf regression:
operators delegated to the ABC mixins, whose _from_iterable re-entered
__init__ and re-validated the entire result element by element, and
Constants() re-checked ~1,400 already-normalized default constants on
every construction. Measured on the headline per-instance-config path,
HumanName(constants=None) was 6.8x slower than master (101 -> 690us)
and Constants() 21.6x slower (7.5 -> 162us).

Build operator results with plain set ops on already-normalized
elements via a private _from_normalized constructor (also dropping the
super()-delegation type ignores), short-circuit SetManager operands in
_normalized_elements (a SetManager is normalized by construction), and
validate the nine default config sets once at import, copied on
identity-checked defaults. Back to master parity: Constants() 8.2us,
HumanName(constants=None) 58us.

Also fold the single-caller _reject_bare_string into
_normalized_elements, note the deliberate bytes-policy divergence from
add_with_encoding(), merge two tests that pinned the same |= parse
end-to-end, trim the triple-stated operator rationale, and update the
stale AGENTS.md SetManager normalization note.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…docs, add tests

- Document that the _DEFAULT_* identity fast path in Constants.__init__
  snapshots module constants at import time, so mutating a raw constant
  (e.g. TITLES.add(...)) after import isn't picked up by later
  Constants() instances — only the documented CONSTANTS.titles.add(...)
  path is supported.
- Strengthen _from_normalized's docstring to state explicitly that it
  performs no validation and callers must pre-normalize elements.
- Add tests pinning __rsub__'s operand order, confirming Constants()
  instances don't alias mutable state via the identity fast path, and
  confirming an equal-but-not-identical titles list still validates
  fully instead of false-matching the fast path.
@derek73 derek73 merged commit 605e31f into master Jul 6, 2026
8 checks passed
@derek73 derek73 deleted the fix/issue-238-string-shredding branch July 6, 2026 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Constants(titles='dr') silently shreds a bare string into characters

2 participants