Hi everyone,
It seems this hasn't garnered much attention.
Nevertheless, meanwhile I've realised the code had a few bugs, in
particular around the backwards-compatibility flag.
I've done a major rewrite:
- fixed couple of bugs I found in the original implementation
- made implementation more robust, simpler, and backwards-compatible:
- added new expect-chars macro to support the new
non-backwards-compatible matcher procedure signature.
- removed need for expect-pass-char? backwards-compatibility feature
flag. expect and expect-strings macros work as in ice-9.
- improved interact macro, to allow specifying separate expect/input
and interact/output ports.
The code can be found here
<https://github.com/dadinn/common/blob/feature/expect4/expect.scm>.
Thanks,
Daniel
On Sat, 15 Apr 2023 at 16:10, Daniel Dinnyes <[email protected]> wrote:
> Hi everyone,
>
> I am new to this mailing list, and writing this because I have encountered
> some issues with ice-9 expect
> <https://git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/expect.scm>
> :
>
> The procedural expect macro has a serious problem:
>
> This syntax expects a procedure as the test expression, with 2 arguments
> (actual content read, and boolean EOF flag). The primary problem is that
> when the test expression is not a identifier bound to such a procedure, but
> instead an expression which creates such a procedure (aka "factory"), then
> the expression will be evaluated anew for each character read from
> expect-port. This is a serious problem: when constructing that procedure
> has significant computational costs, or side effects (eg. opening a port to
> a new log file), as it can be seen in my code here
> <https://github.com/dadinn/system-setup-tests/blob/expect-bug/run.scm#L476>.
> (This is a branch of some of my KVM-based E2E testing code, which uses the
> original expect macro implementation, just to demonstrate the problem).
>
> I had written a workaround for this problem, by binding the evaluation of
> the test expression outside the expansion of the expect macro (as it can be
> seen here
> <https://github.com/dadinn/system-setup-tests/blob/master/run.scm#L29>).
> The problem with this was, that it doesn't support the full capabilities of
> the original expect macro, and debilitates it by only allowing for a single
> conditional branch. Also, I haven't verified, but I suspicious it would
> possibly even break the behaviour for the => syntax of the expect-strings
> macro.
>
> To implement a more complete version of this (and also as an exercise to
> understand hygienic macros), I've attempted to rewrite my workaround using
> syntax-case. I've ran into some issues, and my implementation didn't
> expand as I expected. After some discussion on IRC, we've concluded
> <http://snailgeist.com/irc-logs/?message_id=136306> that this was because
> the current (ice-9 expect) implementation is written using unhygienic
> defmacro syntax, which doesn't work well together with the hygienic
> syntax-case. It was suggested I should rather rewrite expect completely
> to be hygienic instead.
>
> I've eventually completed the rewrite using syntax-case here
> <https://github.com/dadinn/common/blob/feature/expect/expect.scm>, and
> would be happy to contribute it back to the ice-9 module!
> I would point out the following about this implementation:
>
> 1. It works!!! I had some quite extensive E2E testing code written
> before here
> <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm>,
> which I think really stretches the possibilities with (ice-9 expect).
> Switching to the new implementation was simple, and works flawlessly!
> 2. It is backwards compatible! I've gone extra lengths to make sure
> the code would be a backwards compatible, a drop-in replacement.
> 3. Introduced new feature for the procedural expect macro, such that
> the second argument of the procedure gets passed the currently read char,
> or #f if it was an EOF character. There is multiple reasons this is
> superior to passing just a boolean eof? flag argument:
> 1. the same logic can be implemented as with the eof? boolean, just by
> negating the condition
> 2. passing the currently read char avoids having to do the
> inefficient operation to retrieve the currently read char by cutting
> off
> the last character from the end of the content. Currently it is
> necessary
> to do this, if the logic wants something additional to be done with
> the
> current char (eg. as it can be seen in my code here
>
> <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L200>
> )
> 3. one such case of additional use is making the expect-char-proc
> property obsolete, as the predicate procedure is now able to execute
> such
> logic too, besides other more advanced logging scenarios (like it
> can be
> seen in my code here
>
> <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L205>:
> here besides logging everything to STDOUT, and also to a main
> log-file, I
> also log for each expect macro call the contents it tries to compare
> against into a separately named minor log-file (useful for
> debugging), and
> also to ensure that we actually do not write to STDOUT when using
> expect on
> multiple parallel processes)
> 4. To ensure that everything stays backwards compatible, this
> new feature is only enabled if the new expect-pass-char? binding
> is set to #t in the lexical context (like here
>
> <https://github.com/dadinn/system-setup-tests/commit/21bddce3cd1c14f8834adaee4ff75f5b1b7a7b04>).
> Otherwise, by default this feature is disabled, and boolean eof?
> flag argument is passed just like before.
> 4. There is a better support for using default value/expressions
> for the various parameters the macros depend on in their lexical contexts.
> The old macro had to export default values defined in its module, so as to
> ensure the defaults are available. This either clutters the module's global
> context where (ice-9 expect) is used, or the defaults are not
> available when using something like ((ice-9 expect) #:select (expect
> expect-string)). The new defaults are calculated based on the lexical
> context where the macros are used, and expands into the correct default
> values when they are not defined. For example when expect-port is not
> defined in the lexical context, then the default value used is going to be
> the result of the (current-input-port) expression. I would like
> someone to review whether I've implemented this correctly, as it seems the
> current-input-port identifier default for expect-port gets captured in
> the expansion. Not sure why that happens.
> 5. The expect-strings macro supports the => syntax, which passes the
> output(s) of the test predicate to a procedure, instead of just evaluating
> a body. This is fully supported, but additionally, the => syntax now
> also works with the procedural expect macro too! I haven't verified if the
> original implementation did this too, but at least it wasn't described in
> the documentation!
> 6. As an added bonus, I have added a simplistic implementation for an
> interact macro. It uses threads to read from STDIO, and expects expect-port
> to be an input-output-port, so that it can output the lines read from STDIO
> into the same port. Not too advanced, as it doesn't support a full terminal
> interface, with autocomplete, etc... but works well enough for my use-case
> to interact with a KVM process via serial port.
> 7. mnieper said <http://snailgeist.com/irc-logs/?message_id=136211>
> that I am not using syntax-case idiomatically, and my style is more in the
> form of a syntax-rules-based state-machine.
> I am quite happy with how it is currently, but would be open to be
> convinced if there is a superior way of expressing all this.
> 8. I currently have various tests sprinkled around my implementation,
> to manually demonstrate expansions, and executions.
> Also, I have added some automated unit tests for the bind-locally
> helper procedure, which I used for checking whether parameters are defined
> in the lexical scope.
> 9. I am not happy with how I am managing my project structure, and how
> my modules can reference each other (having to call add-to-load-path
> everywhere).
> Also, I think the way I am using srfi-64 tests is a bit bothersome, as
> they always get executed when my modules are loaded, and I have to globally
> disable logging to files.
> I would like some recommendations how to do this better!
>
> If the community likes this implementation, I would be happy to
> apply/rebase my changes onto the official repo, and then we can go from
> there!
>
> Best regards,
> Daniel Dinnyes
>
>
>
