Separate Slang runtime from REPL implementation

[lthibault]

@spy16 I've made a few changes to the Slang implementation.

The goal here is to have a generic REPL for running languages built with Sabre. The repl package defines a Runtime interface, which contains all of the language's implementation logic; slang.Slang satisfies repl.Runtime.

All that's needed for me to develop my language is to write an implementation of Runtime.

What do you think?

[spy16]

Oh great!. I was planning to do this eventually too. At the moment had kept it slang specific since it was in that package. I will review it sometime tomorrow and merge it. Thanks for the PR.

[lthibault]

Seems like we're on the same wavelength 😃

I will review it sometime tomorrow and merge it.

Perfect. I'm going to fix a few more things, so expect updates to the PR between now and tomorrow morning.

Thanks for the PR.

My pleasure - this is a super useful library!

[lthibault]

Hi Again -- I just have a couple of updates for when you get around to this.

I've refactored a few things, and quite like the result. The two major design changes are:

1: abstract away readline

Pull the github.com/chzyer/readline dependency out of the shared library code and place it in package main. Readline utilities are an application-specific thing, so it feels like Sabre users should be able to opt-out. Such opt-out requires keeping readline out of public library packages (namely: sabre, repl and slang).

2: replace repl.REPL.Run with iterator pattern

I'm personally not very fond of the repl.REPL.Run method, as it currently exists. It takes a context to manage loop state, yet it doesn't run anything in a separate goroutine. This is unexpected (at least for me), as context.Context is intended to be the Go way of doing thread-local storage.

This PR eschews repl.REPL.Run for an iterator pattern, which is more functional in style. IMHO, this has the following upsides:

• Factors out the need for context.Context
• Factors github.com/pkg/errors error-handling out of library code, and places it into package main. Error handling is much more explicit in this way.
• The main() function is made more readable by a single call to loop(repl) (though this may just be a matter of taste...).

---

All in all, these changes have me feeling quite enlightened. Writing my own language on top of Sabre is suddenly simple, obvious, and (I dare say) aesthetically pleasing.

My language (called wetware) has a somewhat complicated init routine, so I use the fx framework for dependency injection. Remarkably, everything stays quite clean and readable!

import (
    "errors"
    "fmt"
    "io"

    "github.com/urfave/cli"

    "go.uber.org/fx"

    "github.com/lthibault/wetware/internal/boot"
    ww "github.com/lthibault/wetware/pkg"
)

func main() {
    app := cli.NewApp()
    app.Flags = flags

    app.Action = run

        runApp(app)
}

func run(c *cli.Context) error {
    var (
        runtime ww.Ww  // implements wetware runtime; satisfies repl.Runtime
        linereader *readline.Instance  // I was able to opt-in to chzyer/readline
    )

    app := fx.New(
                // provide ww's dependencies.  This what tells Fx how to instantiate `runtime`
        // and `linereader`.
        boot.Provide(c),

        // Instantiate the above dependencies.
        fx.Populate(&runtime),
    )

    // start Fx
    if err := start(app); err != nil {
        return err
    }
    defer stop(app)

    wwrepl := repl.New(runtime,
        repl.WithBanner(banner),
        repl.WithInput(linereader),
        repl.WithOutput(linereader.Stdout())))

    // main loop
    return loop(wwrepl)
}

func loop(repl *ww.REPL) (err error) {
    for {
        if err = repl.Next(); err != nil {
            switch {
            case errors.Is(err, io.EOF):
                return nil
            case errors.Is(err, readline.ErrInterrupt):
            default:
                fmt.Fprintf(repl, "%+v", err)
            }
        }
    }
}

[spy16]

I like keeping dependencies away from shared libraries. So I like the readline abstraction. And i like the separate repl package and the options.

Second part I'm not really sure. Because a REPL is expected to have the Loop part in it and anyone using sabre to build a REPL would have to rewrite the loop function again. Also, context is meant mainly for cancellations (along with thread local storage too.). A function that runs forever is a good candidate for this even if it doesn't spawn a goroutine (which I think libraries shouldn't do anyway). This is useful when someone trying to embed slang as a scripting layer for their application where REPL would be in a goroutine and their application logic would in the main goroutine. (For example I have plans to use slang for a spiking neural net simulation where main will be running actual simulation and repl will be on separate goroutine and gives simulation control. When I'm done , I could stop the repl by cancelling context, then save all the simulation state and then exit). Another example is when the REPL is exposed via a socket (like nREPL). In that usage, you would want to be able to stop a REPL when the client terminates the socket connection. (Of course you can do this with the iterator design as well, but context is a perfect candidate for this i think. You can create a cancellable context and defer cancel() in the socket handler function)

I understand that if we move Loop back into repl, error handling would be tricky. Let me think about this and get back to you.

[lthibault]

I like keeping dependencies away from shared libraries. So I like the readline abstraction. And i like the separate repl package and the options.

👍

Second part I'm not really sure. [...]

These are some good points, especially the REPL-over-socket bit (which is a feature planned on my end, too). Would you be open to having both context-based iteration and an iterator pattern? This would provide us with a network/simulation-friendly API while handling errors correctly. Here's my reasoning:

It sounds like we' re in agreement that readline should be abstracted away by an interface, which in which case we can't depend on readline.ErrInterrupt. This means users will need to wrap readline.Instance as such:

type linereader{
    *readline.Instance
}

func (lr linereader) Readline() (string, error) {
    s, err := lr.Instance.Readline()
    if err == readline.ErrInterrupt {
        err = sabre.ErrInterrupt
    }
    return s, err
}

On the surface, it seems like we must choose between:

• users wrap readline.Instance individually
• users write a main loop for each application

If that's correct, then one way of resolving this would be to provide a small utility package for working with a REPL iterator. I haven't given it very much thought, but it might look like this:

import (
    "github.com/spy16/sabre" 
    "github.com/spy16/sabre/util/loop"
)

 // example usage of loop.Run utility, which avoids REPL-loop boilerplate.

 err := loop.Run(context.Background(), repl,
     loop.WithInterrupt(readline.ErrInterrupt),  // specifies which error(s) should be treated as interrupts
     loop.WithFatal(io.EOF))  // specifies which error(s) should break out of the loop

In this way, we can make the REPL work as an iterator, but still offer a high-level API with context-based cancellation. From there, I'm able to work directly at the iterator-level if I prefer.

The trade-off is that we maintain two public-facing APIs which are interdependent. I think this is worth it since Sabre aims to be a scripting engine rather than a language per se; it gives us a very flexible API for building languages and interpreters.

Looking forward to hearing your thoughts. Of course, feel free to disagree with any of this! 😃

[spy16]

I like the idea of configuring what error to use as exit signal and interrupt signal. What do you think about providing WithInterrupt(errValue) and WithFatal(errValue) REPL options.

Or we can make it even more flexible by changing WithInput(input) to WithInput(input, func(error) error) option. Here the second argument is an error mapper for the Input. If the second argument is nil, we simply assume a default behavior (which is basically exit on any error from reader)

Usage:

errMapper := func(err error) error {
    if err == readline.ErrInterrupt {
         // this can even return repl.ErrInterrupt if we want specific error based flow    
         return nil           // this error is nothing to worry about. continue the loop.
    }
    return err               // can't continue anymore, reader encountered fatal error
}

repl := repl.New(runtime,
           WithInput(input, errMapper),
)     

I was thinking both iterator and loop pattern would be a possibility too. Simply have Next() and Run(ctx) methods.. But with the above approach, would you still need the iterator pattern ? I feel like it is covering your use case as well. I want to keep public API as minimal as possible while not compromising flexibility. From user perspective, setting up REPL will be as simple as:

r := repl.New(runtime, WithInput().....)
r.Run(context.Background())

[lthibault]

👍 I like it. Do you want to take a stab at it?

Let's get rid of the iterator pattern for now. This ticks all the boxes :)

[spy16]

Great! I will merge these changes into slang branch and then work on it.

[lthibault]

One more thought: in my project I'm registering custom macros to the Sabre reader. How do you envision this being done with this new REPL?

[spy16]

One approach i was thinking about was to have a reader-factory function registered with REPL. So that REPL can init a reader instance using this factory function (instead of current sabre.NewReader(...)).

[lthibault]

One option i was think was to have a reader-factory function registered with REPL.

Yes, that's what I was thinking too. Let's do that!