sasagawa888 / eisl

ISLisp interpreter/compiler
Other
272 stars 22 forks source link

unwind-protect does not obey exiting rules #267

Closed gtnoble closed 1 year ago

gtnoble commented 1 year ago

This example should print:

"should be printed first"
"should be printed second"
"should be printed third"

It now prints:

"should be printed first"
"should be printed third"
"should be printed second"
(defun unwind-protect-test-case-1 ()
  (unwind-protect 
    (progn
      (catch 'tag
           (unwind-protect
                  (throw 'tag "thrown")
                  (print "should be printed first")))
      (print "should be printed second"))
    (print "should be printed third")))

There is a non-local exit for the inner unwind-protect block only. The outer unwind-protect exits normally, so the outer cleanup-form should be evaluated last.

It looks like every call to unwind-protect adds the cleanup forms to a global stack:

https://github.com/sasagawa888/eisl/blob/126691908980ebcd70164f923774f4d95efb2e92/syntax.c#L1255-L1265

When catch is called, all cleanup forms in the global stack are evaluated. This includes cleanup forms that correspond to outer forms that have not exited yet.

https://github.com/sasagawa888/eisl/blob/126691908980ebcd70164f923774f4d95efb2e92/syntax.c#L1132-L1138

gtnoble commented 1 year ago

Here is another example: The example should print:

"should be printed first"
"should be printed second"

It now prints:

"should be printed second"
"should be printed first"
(defun unwind-protect-test-case-2 ()
  (unwind-protect
    (progn
      (catch 'tag
           (throw 'tag "thrown"))
      (print "should be printed first"))
    (print "should be printed second")))

the throw and catch forms do not cause the program to exit the unwind-protect form, so the cleanup form should be evaluated last.

sasagawa888 commented 1 year ago

Thank you.

sasagawa888 commented 1 year ago

I improved catch-function. second example is OK. but first example is bad.

Easy-ISLisp Ver2.93
> (load "tests/bug.lsp")
T
> (unwind1)
"should be printed second"
"should be printed first"
"should be printed third"
NIL
> (unwind2)
"should be printed first"
"should be printed second"
NIL
> 

I pondered. In the first example, throw moves control to catch. "second" seems to run first.

gtnoble commented 1 year ago

I improved catch-function. second example is OK. but first example is bad.

Easy-ISLisp Ver2.93
> (load "tests/bug.lsp")
T
> (unwind1)
"should be printed second"
"should be printed first"
"should be printed third"
NIL
> (unwind2)
"should be printed first"
"should be printed second"
NIL
> 

I pondered. In the first example, throw moves control to catch. "second" seems to run first.

Are you saying that you think

"should be printed second"
"should be printed first"
"should be printed third"

is the correct ordering for unwind1?

sasagawa888 commented 1 year ago

I think so. But I'm not sure.

sasagawa888 commented 1 year ago

The fix didn't work. Try again.

gtnoble commented 1 year ago

I think so. But I'm not sure.

For https://github.com/sasagawa888/eisl/blob/ac48472cd9a6e60e06d7d00197a7a026d86b979b/tests/bug.lsp#L1-L9 (throw 'tag "thrown") transfers control to catch, which is outside the inner unwind-protect form. This should cause the cleanup form (print "should be printed first") to be evaluated immediately. So, I think "should be printed first" should appear first.

sasagawa888 commented 1 year ago

I checked with OKI-ISLisp. bandicam 2023-05-21 06-26-11-422

sasagawa888 commented 1 year ago

I will radically rethink the design.

sasagawa888 commented 1 year ago

Fixed. Is it working? bandicam 2023-05-21 10-21-24-937

sasagawa888 commented 1 year ago

Problem remains with block & return-from .

sasagawa888 commented 1 year ago

Fixed block & return-from for unwind-protect too.

gtnoble commented 1 year ago

I think this now works for the interpreter. I will close the issue. Thank you for your continued effort Mr. Sasagawa!

sasagawa888 commented 1 year ago

I have a deep understanding of the ISLisp specification. thank you.