Skip to content

Conversation

NoahStoryM
Copy link
Contributor

@NoahStoryM NoahStoryM commented Sep 23, 2025

Checklist

  • Bugfix
  • Feature
  • tests included
  • documentation

Description of change

Fix sequence-ref to stop consuming extra elements from stateful sequences.

Problem

The current implementation of sequence-ref uses #:unless (j . < . i) which causes the loop to continue after reaching the target index, leading to unnecessary consumption of an additional element from the sequence. See also #5338.

Example

> (define v 0)
> (define s
    (make-do-sequence
     (λ ()
       (define (pos->element _) v)
       (define (continue-with-pos? _) (set! v (add1 v)) (< v 100))
       (values pos->element void (void) continue-with-pos? #f #f))))
> (define-values (more? get) (sequence-generate s))
> (displayln (get))
1
> (displayln (sequence-ref s 0))
2
> (displayln (sequence-ref s 0))
4
> (displayln (sequence-ref s 1))
7
> (displayln (sequence-ref s 1))
10
> (displayln (get))
12
> (displayln (sequence-length s))
87

Replace `#:unless` with `#:final` in `sequence-ref` to prevent unnecessary
lookahead consumption from stateful sequences. The previous implementation
would consume one additional element after reaching the target index,
causing inconsistent behavior with sequences that have side effects.
@sorawee
Copy link
Collaborator

sorawee commented Sep 23, 2025

I could be missing something, but does this accomplish what you want?

(define (ref s i)
  (let ([v (for/first ([v (in-values*-sequence s)]
                       [j (in-range (add1 i))]
                       #:unless (j . < . i))
             (or v '(#f)))])
    (cond
      [(not v)
       (raise-arguments-error
        'sequence-ref
        "sequence ended before index"
        "index" i
        "sequence" s)]
      [(list? v) (apply values v)]
      [else v])))

@NoahStoryM
Copy link
Contributor Author

I think this version looks correct, and it should behave as intended.
I’m just not sure whether this approach is better compared to the sequence-generate version.

Since this one matches the behavior of the original implementation, I’ll go ahead and use it for now.

@sorawee
Copy link
Collaborator

sorawee commented Sep 23, 2025

An issue with the (for ([_ (in-range i)] ...) + sequence-generate revisions above is that it's O(max(i, |s|)) rather than O(min(i, |s|)). Maybe there's a way to make them work, but I haven't tried.

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.

2 participants