Hi Jeremy,

> 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.

No worries, I have a Gitlab account and can contribute if need be. But
let's figure out the issues from the email first.

> 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`.

Yes, that's why I'm using it in the sample implementation.

> 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.

Yes, that'd be perfect. The only reason I didn't include it is that rest
arguments in assert are not portable. Feel free to pass rest args to
assert in your implementation, I don't see any problem in that.

> 2. The `(srfi-145)` part of that cond-expand should be lower priority.

Well, the sample implementation is trying to be as portable as possible,
relying on most portable features. First goes the standard (R7RS), then
SRFIs, then old standards (R6RS) and implementations. So the order more
or less makes sense.

As for many implementation-specific expectations, cond-expand can safely
be dropped and replaced with raw Chicken-specific code. I'll certainly
do this for (upcoming, I'm okay if someone beats me to it) Kawa and
Guile implementations.

> 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.

Yes, please go ahead and fix that, I'm totally fine with it.

> # 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.

Note that the SRFI uses a meaning of "it is an error" that's
Scheme-specific. It (confisingly) doesn't mandate throwing an error, but
rather works akin to undefined behaviors in other language. So you can
throw errors, but you don't have to. Thus the use of `the` is compliant
with the SRFI.

I'm okay with using a generic implementation, but I put a lot of thought
into the `the` use, so I'll be grateful if you include it.

> # 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).

It seems that compiler-typecase is indeed not the best solution for
it. Let's omit it and use the generic implementation instead.

> # 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.

Oh, right, I'll open a pull request in the original repo. In the
meanwhile, feel free to proceed with the right exports!

> 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.

Thanks a lot! I'm glad there wasn't many problems and I totally share
your opinions on most changes you've made!

> 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.

I hope I'm not too late with my comments...

Best of luck,
--
Artyom Bologov
https://aartaka.me