Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SRFI 35 support #1008

Merged
merged 3 commits into from
Nov 2, 2024
Merged

Add SRFI 35 support #1008

merged 3 commits into from
Nov 2, 2024

Conversation

dpk
Copy link
Contributor

@dpk dpk commented Oct 26, 2024

This PR is part of the work for #1003.

It adds support for SRFI 35, with an internal module including extra exports necessary to implement the (rnrs conditions) library.

It also adds support for printing conditions to the (chibi repl) module.

It also fixes a bug in the SRFI 99 implementation whereby shadowing field names in the procedural layer didn’t work. There is a bug in the design of SRFI 99 which means shadowing still doesn’t work if you pass rtd-constructor an explicit list of field names, but this at least fixes the problem for uses which don’t do this.

@dpk
Copy link
Contributor Author

dpk commented Oct 26, 2024

I should note there is still a problem with the REPL: it should return from continuable non-serious conditions (since in R6RS conditions are also used for warnings). At the moment it assumes all exceptions raised should not be continued. But this is a somewhat larger rewiring of the REPL’s exception-handling mechanism, so I haven’t attempted it.

@ashinn
Copy link
Owner

ashinn commented Oct 26, 2024

I should note there is still a problem with the REPL: it should return from continuable non-serious conditions (since in R6RS conditions are also used for warnings). At the moment it assumes all exceptions raised should not be continued. But this is a somewhat larger rewiring of the REPL’s exception-handling mechanism, so I haven’t attempted it.

I'm not sure what you mean here. The REPL puts no restrictions on exception handling, however if an exception is uncaught and escapes to the top-level the REPL is free to do what it wants, including crashing. For user convenience we instead print the exception and provide a new prompt. If you mean you want an interactive debugger with the ability to manually continue an exception, then that's a much larger project and clearly outside the scope of standards.

@dpk
Copy link
Contributor Author

dpk commented Oct 26, 2024

If the raised exception is a SRFI 35 condition, and does not have a condition type &serious (i.e. not serious-condition?), the REPL’s default exception handler should return some unspecified value, instead of calling the continuation it captures to escape back to the prompt.

Compare Chez Scheme:

> (define (example)
    (raise-continuable (make-warning))
    (display "got here\n")
    'returned)
> (example)
Exception occurred with condition components:
  0. &warning
got here
returned
> 

Because &warning is not a &serious condition, control returns to the point where the condition was raised.

(If you change raise-continuable to just raise, then there is a &serious condition, specifically &non-continuable. But in Chibi that will continue to be one of the native exception objects.)

Copy link
Owner

@ashinn ashinn left a comment

Choose a reason for hiding this comment

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

Thanks!

(or (and (type? p)
(rtd-field-offset p field))
(let ((i (field-index-of (type-slots rtd) field)))
(or (let ((i (field-index-of (type-slots rtd) field)))
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the fix!

lib/chibi/repl.sld Outdated Show resolved Hide resolved
lib/srfi/35/internal.scm Outdated Show resolved Hide resolved
@ashinn
Copy link
Owner

ashinn commented Oct 26, 2024

If the raised exception is a SRFI 35 condition, and does not have a condition type &serious (i.e. not serious-condition?), the REPL’s default exception handler should return some unspecified value, instead of calling the continuation it captures to escape back to the prompt.

... but there is no REPL in R6RS, isn't the behavior unspecified?

@dpk
Copy link
Contributor Author

dpk commented Oct 26, 2024

If the raised exception is a SRFI 35 condition, and does not have a condition type &serious (i.e. not serious-condition?), the REPL’s default exception handler should return some unspecified value, instead of calling the continuation it captures to escape back to the prompt.

... but there is no REPL in R6RS, isn't the behavior unspecified?

Oh, yes. Hmm. This should (also) apply to exceptions handled at the top level of a program. I guess importing (srfi 35) should also set up the default current-exception-handler to do this right … this might be tricky. Hmm.

But as SRFI 35 itself does not define any non-&serious condition types, nor impose any requirements on how conditions are handled once raised, this is not strictly a problem for this PR but maybe for the future as #1003 progresses.

Copy link
Owner

@ashinn ashinn left a comment

Choose a reason for hiding this comment

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

Thanks!

(make-simple-condition)
simple-condition?)

(define-record-type Compound-Condition
Copy link
Owner

Choose a reason for hiding this comment

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

On reviewing the SRFI I'm rather unhappy with it because of this distinction. It's ad-hoc and leads to a lot of ambiguity which doesn't exist in other exception systems. Cleaner is to always have one type which may have fields referring to other types (e.g. the underlying causes). I guess this was meant to make up for the lack of multiple inheritance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it’s any consolation, if you only use the (srfi 35) interface or the (rnrs conditions) interface, the representational distinction is pretty much non-observable. It’s only because (srfi 35 internal) wants to export everything needed to implement both interfaces (and, until you suggested the change below, (chibi repl) printing of conditions as well) that that library exposes the two different record types.

The ‘pretty much’ is because R6RS says a &condition is a non-sealed, non-opaque record type with no fields. (This is what requires implementing it this way for R6RS support, rather than the way chosen by the SRFI 35 sample implementation where the representations of simple and compound conditions are unified.) If you go in through the back door and make a record-predicate/rtd-predicate or a record-accessor/rtd-accessor for a condition type, you will find that it only works on simple conditions of that type – you have to use condition-predicate and condition-accessor instead. That’s the only place the representation breaks down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The model is not quite multiple inheritance, because by design the set of simple conditions that makes up each compound condition is specific to the instance, not to the type as a whole.

Also, like consing onto an alist, it has an historical property: if an exception handler adds a new component before re-raising and the new component has a type (or supertype) which was already in the compound condition, the old component can still be accessed through the simple-conditions procedure.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I wasn't clear in my complaint. The complaint is in the existence of, and design of, compound conditions, not any implementation details.

The fundamental problem is that in SRFI 35 a given condition doesn't have any single, or even most specific, type. You move the burden of taking apart and understanding the condition to the call site, which isn't in general prepared or qualified to deal with this. Instead the code raising the condition should take care to choose a single most appropriate type. If there is a root cause it can be attached as a field, so that where it really matters the caller can unwrap and understand the condition in more detail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The SRFI 35/R6RS solution works in practice, IMO, but I understand it is not flawless. Consider, for example, a raised condition of the form (condition (make-error) (make-irritants 'foo 'bar)). Let us assume that this exception is intercepted by an exception handler deciding that it should be a more serious condition for some reasons. So it wraps the condition by extending the compound condition which yields a new condition of the form (condition (make-violation) (make-irritants 'more-info) (make-error) (make-irritants 'foo 'bar)). With this model, it is not apparent which irritants belong to which part of the condition.

@@ -0,0 +1,206 @@
(define-record-type Simple-Condition
Copy link
Owner

Choose a reason for hiding this comment

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

I have to think about this, but long term it may be better to unify the native exceptions with Simple-Condition.

Copy link
Contributor Author

@dpk dpk Oct 28, 2024

Choose a reason for hiding this comment

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

I assumed this would be too much core change for you, but I’m happy to consider implementing that as an alternative approach if you’d be okay with it.

lib/chibi/repl.scm Outdated Show resolved Hide resolved
Copy link
Owner

@ashinn ashinn left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM,

@ashinn ashinn merged commit 679875d into ashinn:master Nov 2, 2024
2 of 3 checks passed
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.

3 participants