Skip to content

fix(reflect): return None for string lookups on non-string maps - #470

Merged
iainmcgin merged 2 commits into
anthropics:mainfrom
Yong-yuan-X:fix/map-value-get-str-non-string
Sep 26, 2026
Merged

iainmcgin merged 2 commits into
anthropics:mainfrom
Yong-yuan-X:fix/map-value-get-str-non-string

Conversation

@Yong-yuan-X

Copy link
Copy Markdown
Contributor

Summary

MapValue::get_str documents that lookups on non-string-keyed maps return None, but a debug assertion caused those calls to panic in debug builds.

Remove the assertion and retain the existing allocation-free binary search. Integer- and boolean-keyed maps now return None consistently, including through ReflectMap.

Add regression coverage for all non-string key variants and a changelog fragment.

Validation

  • Confirmed the regression test fails before the fix and passes afterward.
  • All 70 buffa-descriptor library tests pass with all features enabled.
  • Formatting, workspace Clippy, and no_std library compilation pass.
  • Workspace tests pass after skipping the existing Windows-specific systemtime_roundtrip_post_epoch failure caused by nanosecond precision loss.

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Only a string key compares Equal in get_str's comparator, so a hit is
always a string entry and the re-check after the search was dead. Add a
test over a map holding both non-string and string keys: it fails if the
non-string arm returns Greater, or if String stops being MapKey's last
variant. Reword the changelog fragment and cite the PR.
@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code] Thanks for this. The fix is right: MapValue::get_str and ReflectMap::get_str both document None for a map that is not string-keyed, the MapView, HashMap and BTreeMap implementations already return it, and only this one panicked, in debug builds. With the assertion put back, your test fails on it; with your change, all 70 buffa-descriptor library tests pass. The comment you replaced was also wrong: the comparator is total over a mixed-key map, because every non-string variant sorts before String in MapKey's derived Ord.

I pushed one maintainer commit (bd453a4) on top:

  • A second test, over a map that holds both non-string and string keys. Your test has one entry per map, so it passes whether the non-string arm returns Less or Greater; this one fails with Greater, and would fail if String stopped being MapKey's last variant.
  • The .and_then after the search re-checked that the hit is a string entry. Only a string key compares Equal, so it is now a plain .map, and the comment says why.
  • The changelog fragment cites (#470) and leads with the panic.

@iainmcgin
iainmcgin added this pull request to the merge queue Sep 26, 2026
Merged via the queue into anthropics:main with commit e16a334 Sep 26, 2026
11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants