Skip to content

Fix TypeError when comparing heap-allocated strings#159

Open
sathish-t wants to merge 1 commit intopydantic:mainfrom
sathish-t:fix/heap-string-comparison
Open

Fix TypeError when comparing heap-allocated strings#159
sathish-t wants to merge 1 commit intopydantic:mainfrom
sathish-t:fix/heap-string-comparison

Conversation

@sathish-t
Copy link

Bug

A human user encountered this bug while using Monty via the JavaScript bindings. This PR was authored with Claude Code to diagnose the root cause and implement the fix.

sorted(), min(), max(), and comparison operators (<, >, <=, >=) fail with:

TypeError: '<' not supported between instances of 'str' and 'str'

when operating on strings created at runtime (e.g. from str.split()), even though the same operations work fine on string literals.

Reproduction

const { Monty, runMontyAsync } = require("@pydantic/monty");

// Works: sorted on literal strings
const m1 = new Monty('sorted(["banana", "apple", "cherry"])', { externalFunctions: [] });
const r1 = await runMontyAsync(m1, { limits: { maxDurationSecs: 5 } });
// => ["apple", "banana", "cherry"]

// Fails: sorted on strings from split()
const m2 = new Monty('sorted("banana,apple,cherry".split(","))', { externalFunctions: [] });
const r2 = await runMontyAsync(m2, { limits: { maxDurationSecs: 5 } });
// => TypeError: '<' not supported between instances of 'str' and 'str'

Equivalent Python reproduction:

sorted(['banana', 'apple', 'cherry'])          # works (InternString literals)
sorted('banana,apple,cherry'.split(','))        # fails (heap-allocated Str)

Root Cause

Value::py_cmp() in value.rs only handled string ordering for InternString vs InternString (compile-time string literals). String literals are interned at compile time, but strings created at runtime (from split(), concatenation, etc.) are heap-allocated as Value::Ref(HeapData::Str).

The (Ref, Ref) match arm in py_cmp() only compared LongInt values, returning Ok(None) for any other heap type — which sorted() and min()/max() interpret as "comparison not supported", triggering the TypeError.

Note: py_eq() already handled all three string representation combinations correctly — py_cmp() was simply missing the same coverage.

Fix

Extended Value::py_cmp() with three new comparison cases:

  1. Ref(Str) vs Ref(Str) — two heap-allocated strings (added to the existing Ref vs Ref arm alongside LongInt)
  2. InternString vs Ref(Str) — interned literal compared with heap string
  3. Ref(Str) vs InternString — heap string compared with interned literal

Tests

Added tests to existing test files:

  • builtin__more_iter_funcs.pysorted(), min(), max() with heap strings from split()
  • compare__mixed_types.py — direct </>/<=/>= between heap-allocated and interned strings

All 760 test cases pass, plus the full make test-ref-count-panic suite.


🤖 Generated with Claude Code

`sorted()`, `min()`, `max()`, and comparison operators (`<`, `>`, `<=`,
`>=`) failed with `TypeError: '<' not supported between instances of
'str' and 'str'` when operating on heap-allocated strings (e.g. from
`str.split()`).

`Value::py_cmp()` only handled string ordering for `InternString` vs
`InternString`. The `Ref` vs `Ref` arm only compared `LongInt` values,
returning `None` for heap `Str` pairs. Added comparison support for:
- `Ref(Str)` vs `Ref(Str)`
- `InternString` vs `Ref(Str)` and vice versa

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 13, 2026

Merging this PR will not alter performance

✅ 13 untouched benchmarks


Comparing sathish-t:fix/heap-string-comparison (508ce8e) with main (a07e336)

Open in CodSpeed

@codecov
Copy link

codecov bot commented Feb 13, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/monty/src/value.rs 85.71% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@sathish-t
Copy link
Author

Hello I really like your repo! Makes it easy for me to interact with LLMs. Please let me know if you want any changes with the pull request here and I can gladly do it! I know some Rust as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant