ashinn / chibi-scheme

Official chibi-scheme repository
Other
1.21k stars 142 forks source link

`(chibi parse)`: Improve failure continuation for parse-commit #822

Open nmeum opened 2 years ago

nmeum commented 2 years ago

Currently, the parse-commit procedure from (chibi parse) unconditionally uses a failure continuation which simply returns #f, i.e. (lambda (s i r) #f):

https://github.com/ashinn/chibi-scheme/blob/5b8e196e0f9f6a65ff194aa6a64ffa88858e1be2/lib/chibi/parse/parse.scm#L524-L526

I encountered this while working on a parser which uses (chibi parse) with a custom error handling procedure / failure continuation as passed initially to call-with-parse. Since parse-commit does (correctly) not use the fk procedure argument it ends up not invoking this "top-level" failure continuation and simply causes call-with-parse to return #f which I found somewhat surprising and caused a bug in my code.

I was wondering if there was any interest in enhancing the parse-commit behavior in this regard. For example, by storing the initial "top-level" failure continuation (passed to call-with-parse) in the Parse-Stream record type and invoking that from parse-commit directly (thus still preventing backtracking) or by (optionally) allowing a custom failure continuation to be passed to parse-commit, e.g.:

-(define (parse-commit f)
-  (lambda (source index sk fk)
-    (f source index (lambda (res s i fk) (sk res s i (parse-stream-fk source))) fk)))
+(define (parse-commit f . o)
+  (let ((commit-fk (if (pair? o) (car o) (lambda (s i r) #f))))
+    (lambda (source index sk fk)
+      (f source index (lambda (res s i fk) (sk res s i commit-fk)) fk))))

I would personally find the former approach more intuitive but maybe that's just me. Thoughts?

ashinn commented 2 years ago

I'm fine with passing a custom failure continuation to parse-commit. This is more general in that it allows you to commit only up to a certain point if desired.

There are different ways this could be managed more conveniently which we could consider later. I'm not so sure about storing this in the Parse-Stream.

nmeum commented 2 years ago

I created #824 to allow passing a custom failure continuation as a second argument and adjusted the documentation accordingly (also emphazising that prior failure continuations are not picked up by default).

There are different ways this could be managed more conveniently which we could consider later. I'm not so sure about storing this in the Parse-Stream.

Yep, I agree that storing stuff like that in the Parse-Stream is not ideal. I think this problem can be addressed separately.