Hey Arthur, Artyom,

On 11/15/24 16:12, Arthur A. Gleckler wrote:
> Scheme Request for Implementation 253,
> "Data (Type-)Checking",
> by Artyom Bologov,
> has gone into *final* status.
>
> The document and an archive of the discussion are available at https://
> srfi.schemers.org/srfi-253/ <https://srfi.schemers.org/srfi-253/>.
>

Thanks for finalizing this! I've gone ahead and forked the sample
implementation and started packaging it for CHICKEN Scheme.

<https://gitlab.com/ThatGeoGuy/srfi-253/-/merge_requests/1/commits>

I mostly work on GitLab, so apologies for switching over there. I've
added CI to my version of the repo to be able to have some automated
testing for the CHICKEN implementation, which is my justification for
the fork across forges.

Nevertheless, there were some issues that I think I wanted to address
with the CHICKEN versions of some procedures in the sample
implementation, and one minor issue related to imports / exports. I'll
say upfront that these are all implementation problems that I addressed
in the version I'm packaging for CHICKEN Scheme, but don't represent any
actual issue with the SRFI as proposed.

1. Assume / assert
2. values-checked
3. check-case
4. Exports

# Assume / Assert

There is a form in the sample implementation:

(cond-expand
  (srfi-145)
  ((or r6rs chicken loko)
   (define-syntax assume
     (syntax-rules ()
       ((_ expr rest ...)
        (begin (assert expr) #t)))))
  (debug
   (define-syntax assume
     (syntax-rules ()
       ((_ expr rest ...)
        (or (and expr #t)
            (error "assumption violated" 'expr rest ...))))))
  (else (define-syntax assume
          (syntax-rules ()
            ((_ rest ...)
             (begin #t))))))

I know nothing of r6rs, loko, or what you aim to do with the debug
feature in this cond-expand; however, CHICKEN's assert procedure is
pretty much already what you want for `assume`.

<https://api.call-cc.org/5/doc/chicken/base/assert>

Notably, there's two things that I think need to be changed in the above
form for the sample implementation:

1. CHICKEN should include the `rest ...` args when calling assert.
2. The `(srfi-145)` part of that cond-expand should be lower priority.

I think the former I can pretty well summarize in this commit:

<https://gitlab.com/ThatGeoGuy/srfi-253/-/merge_requests/1/diffs?commit_id=a47ebe4b43d13010c5b29d38734a0c8c1b670071>

Basically, you want `(begin (assert expr rest ...) #t)`, so that the
remaining arguments give a better error message. The latter may be
controversial, but I'll point out that CHICKEN's implementation of
SRFI-145 basically wraps the sample implementation, which makes the same
mistake. I can understand the desire to reuse code, but SRFI-145 (at
least in CHICKEN) isn't an ideal place for this to be fixed.

Further, installing SRFI-145 as a dependency for SRFI 235 is a lot of
overhead. It requires pulling the package off of the internet, as well
as actual time to compile the whole egg on top of SRFI 235 itself. Given
how modules / compilation units in CHICKEN work it simply makes more
sense to just use `assert` here in place of SRFI-145.

As a final note: I have noticed that for some SRFIs there are members of
the CHICKEN community who wrap the sample implementations in a pretty
bare-bones way, or in a way that might lead to performance degradations
(due to omitting optimizations, or by wrapping existing procedures in
such a way that CHICKEN's optimizer cannot always make good decisions
across compilation units). It's an implementation problem, and one that
is easily avoided if you just redefine syntax like `assume` in the crate
itself.

# values-checked

In the sample implementation of values-checked, you're using CHICKEN's
`the` operator to specify a type-check. This is probably a
misunderstanding of how that part of CHICKEN works — `the` will not
actually emit an error in the way you want it to. It will emit a warning
if you compile with specializations on (-O3 and higher), but it doesn't
do anything in the interpreter (csi) or if you compile with
specializations off. Instead, it will just return the last value
provided to the `(the ... value)` form.

Given that the SRFI specifies that it is an error if you don't have a
match here, I opted to just remove the CHICKEN-specialized version and
use the version in the `else` block in that same cond-expand. This
produces a version which passes the tests.

# check-case

I removed the CHICKEN-specific implementation of check-case as well,
opting for the portable version. Unlike values-checked, the provided
version mostly works, but there is one particular issue surrounding the
use of compiler-typecase:

<https://api.call-cc.org/5/doc/chicken/types/compiler-typecase>

While I think it would probably be correct to use something like
compiler-typecase for check-case, the problem is that this specific API
behaves very differently when run by the interpreter (csi) or with
specializations off (-O2 and lower).

Basically: it needs an else-clause all the time. When this else clause
is omitted and this is run during compilation with specializations
turned on, CHICKEN will raise this as a compiler error and emit the
correct message to the user.

As it stands this API is hard to make generic for all cases (you might
for example compile the SRFI-253 egg with -O3, but downstream users can
compile their own code with -O2 and run into issues). I would love to
fix this and add an else-clause that does the right thing in the right
place, but I don't really understand the macro well enough to make that
change (I spent about ~2 hours on it and couldn't crack it, but others
are welcome to try). As it stands the CHICKEN-specific impl in the
sample implementation repository fails this test:

     (test-assert (check-case "hello" (string? #t)))

Because if you run this in the interpreter (tests in CHICKEN eggs are
run in the interpreter after compilation and installation of the
library) it will fail complaining that you don't have an else-clause
where you need one. Note that executing that check-case can be all well
and good, but that else-clause still needs to be there.

If there is a way to crack this with compiler-typecase, I'd be more than
happy to take a patch for this on the GitLab repo I've shared (or just
at my email).

# Exports / Imports

The correct list of exports for the sample implementation is:

(export check-arg
         values-checked
         check-case
         lambda-checked
         define-checked
         case-lambda-checked
         define-record-type-checked)

However, the sample implementation still tries to export
opt-lambda-checked and define-optionals-checked, and fails to export
check-case. You might want to fix this for other Scheme implementations.

Aside from that, good job! Many of the tests "just worked" and diving in
wasn't too bad for someone who is not a macro expert. I think most of
these issues come down to familiarity with CHICKEN and a sort of
chicken-egg problem where packaging the code as an egg helps diagnose
some of these issues when trying to use CHICKEN overall.

I don't know if I'll wait for any feedback on this message to merge the
above MR on GitLab and tell the chicken-users mailing list, but I am
definitely open to any help that can be provided here to make the
CHICKEN implementation better.

Thanks, and Regards,
--
Jeremy Steward