fix: reject indexing a String instead of segfaulting

[martian56]

Summary

s[i] on a String passed the type checker and then segfaulted at run time. The type checker gave the expression type Char, but the code generator has no String case for index access — lower_index_access lowers every index through the List path, so a String header was reinterpreted as a List header. The bounds check ran against a garbage length and the load dereferenced a garbage pointer, making an out-of-bounds read reachable from ordinary Raven code with no FFI and no raw pointers.

This rejects the receiver in the type checker rather than implementing String indexing, because under the current String model there is nothing coherent for s[i] to return:

• String is a byte string. length() counts bytes, and char_at(i) is documented as "Byte offset, not codepoint: for a multi-byte character this returns one byte of the encoding."
• Char is a Unicode scalar value, and one byte of a multi-byte encoding is not a valid one — so returning the byte at offset i as a Char is not well defined.
• Returning the i-th codepoint instead would make [] the only codepoint-indexed operation in an otherwise byte-indexed API (inconsistent with length()), and would turn indexing into an O(n) scan — a performance trap inside a loop.

The error points at the byte-oriented accessors that already cover the use case unambiguously.

If O(1) codepoint indexing is wanted later, that is a separate design change (a codepoint-indexed string type, or a chars() view) rather than a fix for this crash.

Before

$ raven build s1.rv -o s1     # compiles cleanly, no diagnostics
$ ./s1
Segmentation fault (core dumped)

After

error: cannot index into a `String`
  ┌─ s1.rv:3:13
  │
3 │     let c = t[0]
  │             ^^^^
  │
  help: a String is a byte string: use `char_at(i)` for the one-byte substring at byte offset `i`, or `byte_at(i)` for that byte as an `Int`; both come from `std/string`

Changes

• src/tycheck/expr.rs — check_index replaces the Ty::Str => Ok(Ty::Char) arm with a TypeError::Custom carrying a hint that names char_at and byte_at. The comment records why the receiver is rejected rather than lowered, so the arm is not "fixed" back into a crash later.
• src/tycheck/tests.rs — three regression tests: indexing a String is rejected, assigning through a String index is rejected, and list indexing still type checks to the element type.
• docs/v2/guide/language-reference.md — a note in the strings section stating that a String is not indexable and pointing at the byte accessors, with a worked example.

Both the read path and the assignment-target path (t[0] = 'z') route through check_index, so the write side of the crash is covered by the same change.

Test plan

• cargo check
• cargo test — 971 passed, 0 failed across the workspace (968 before this branch, plus the 3 new tests). No regressions.
• Manual verification performed:
  • let c = t[0] and t[0] = 'z' are both rejected with the new diagnostic.
  • xs[1] on a List<Int>, char_at(0), and byte_at(0) all still compile and produce 2, a, 97.
  • The example added to the language reference was compiled and run, not just written.

Related issues

Closes #894

Notes for reviewers

The main thing to sanity-check is the decision to reject rather than implement. The alternative — lowering Str indexing to a bounds-checked runtime call — is not much more code, so the argument for rejecting is a semantic one, laid out above and in the issue. If you would rather s[i] work and return a codepoint, that changes the String model (indexing and length() would disagree about units) and is worth deciding deliberately.

Nothing depended on the old behavior: it crashed 100% of the time, and no example, corpus entry, or stdlib module indexes a String. The s[0] in examples/v2/use_cmp.rv is a List<Int> returned by sort, not a string.

Not addressed here, since they are separate issues: the corrupt binary-operator diagnostic (#895), and the fact that the type checker's Ty::Str support for indexing was never exercised by a codegen test — a golden case per indexable receiver type would have caught this.

---

Generated by Claude Code

Summary by CodeRabbit

• Bug Fixes

  • String indexing now produces a clear error instead of being accepted incorrectly.
  • Error messages recommend using char_at(i) or byte_at(i) for string access.

• Documentation

  • Clarified that strings are byte-based, cannot be indexed directly, and report byte-based lengths.
  • Documented character and byte accessors, including behavior with multi-byte characters.

[coderabbitai]

📝 Walkthrough

Walkthrough

The type checker now rejects String indexing and indexed assignment. Documentation defines byte-oriented string access and recommends char_at(i) or byte_at(i). Tests confirm list indexing remains valid.

Changes

String indexing validation

Layer / File(s)	Summary
Define String access semantics docs/v2/guide/language-reference.md	Documents byte-based String values, length(), and the char_at(i) and byte_at(i) accessors.
Reject invalid String indexing src/tycheck/expr.rs, src/tycheck/tests.rs	check_index reports an error for String indexing and indexed assignment. Tests confirm the diagnostic and preserve valid list indexing.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary fix: rejecting String indexing that previously caused runtime segmentation faults.
Description check	✅ Passed	The description covers the problem, rationale, implementation, tests, related issue, and reviewer focus using the required sections.
Linked Issues check	✅ Passed	The changes satisfy issue #894 by rejecting String reads and writes, preserving list indexing, adding diagnostics, and documenting the byte-oriented alternatives.
Out of Scope Changes check	✅ Passed	All changes are directly related to issue #894 and its fix; the documentation and regression tests support the requested behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/fix-string-index-segfault

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/tycheck/tests.rs (1)

134-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the accessor hint in the rejection tests.

These tests verify cannot index into a \String`, but they do not verify the promised char_at(i)andbyte_at(i)` guidance. Assert that the diagnostic hint or rendered diagnostic contains both accessor names.

Also applies to: 151-164

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tycheck/tests.rs` around lines 134 - 149, Update the rejection tests
around indexing a String, including indexing through the related access path, to
assert that the rendered diagnostic or hint contains both `char_at(i)` and
`byte_at(i)`. Preserve the existing assertion for the `cannot index into a
`String`` message.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/tycheck/tests.rs`:
- Around line 134-149: Update the rejection tests around indexing a String,
including indexing through the related access path, to assert that the rendered
diagnostic or hint contains both `char_at(i)` and `byte_at(i)`. Preserve the
existing assertion for the `cannot index into a `String`` message.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7a03d071-ef54-4a4c-8646-a53b1e1efd64

📥 Commits

Reviewing files that changed from the base of the PR and between 898efbe and 14bc284.

📒 Files selected for processing (3)

• docs/v2/guide/language-reference.md
• src/tycheck/expr.rs
• src/tycheck/tests.rs