Skip to content

fix(stdlib): error instead of panicking on reversed slice ranges 🔪#174

Merged
timfennis merged 3 commits into
masterfrom
bugfix/reversed-slice-panic
Jun 2, 2026
Merged

fix(stdlib): error instead of panicking on reversed slice ranges 🔪#174
timfennis merged 3 commits into
masterfrom
bugfix/reversed-slice-panic

Conversation

@timfennis
Copy link
Copy Markdown
Owner

Context

The fuzzer didn't catch this, but a slice whose start index lands after its end ("hello"[3..1]) crashes the interpreter with attempt to subtract with overflow instead of returning an error. Found while hunting for panics in the lexer/parser/analyser/compiler/vm pipeline.

Lists and tuples already handle reversed ranges gracefully because they slice via slice.get(from..to), which returns None. Strings, deques, and slice assignment instead compute take(to - from), which underflows when from > to.

Changes

  • ndc_stdlib/src/index.rs: extract_vm_offset now rejects a range whose forward start exceeds its end, returning a {from}..{to} out of bounds error. Centralizing the check covers every container type (string, deque, slice assignment) and keeps the existing list/tuple error message unchanged.
  • Three regression tests under tests/functional/programs/009_slicing/ cover the string, deque, and slice-assignment paths that used to panic.
  • manual/src/snippets/slices.md: documents that a start-after-end range is out of bounds.

The behavior now matches what lists and tuples already did, so the existing 006_negative_range.ndc test still passes against the same message.

🤖

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 90471caf4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ndc_stdlib/src/index.rs Outdated
The reversed-range check ran after the inclusive +1 adjustment, so an
off-by-one inclusive range like `1..=0` survived it and silently returned
an empty slice instead of erroring. Move the check before the widening.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bf004cc13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ndc_stdlib/src/index.rs Outdated
The pre-widening check compared clamped indices, so a reversed range
whose endpoints both sit at or beyond the length (e.g. `6..=5` on a
5-char string) collapsed to equal indices and returned an empty slice
instead of erroring. Detect reversal from the unclamped endpoints.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@timfennis timfennis merged commit 7090596 into master Jun 2, 2026
1 check passed
@timfennis timfennis deleted the bugfix/reversed-slice-panic branch June 2, 2026 07:18
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