brundonsmith / rust_lisp

A Rust-embeddable Lisp, with support for interop with native Rust functions
234 stars 20 forks source link

Fix panic on invalid function syntax #13

Closed adri326 closed 2 years ago

adri326 commented 2 years ago

When constructing a Lambda instance in src/interpreter.rs, the value for argnames is not checked. This later causes an assertion error, as argnames is expected to be a List containing only Symbols. The following piece of code demonstrates this issue:

((lambda (3) 3))

Expected: The interpreter should return an Err, explaining that 3 is not a valid symbol for a function's argument Actual:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/interpreter.rs:336:49
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

With this PR: Runtime error: Argument names in function definition for "lambda" should only contain symbols, got 3

This PR fixes that, by introducing the requirement for argnames to be a List of Symbols as a class invariant for Lambda. This invariant is enforced on the creation of a new Lambda instance, and the argnames field has been made private and is now read-only through the new argnames() method.

The check in this PR is also catches instances of this issue that may seem less obvious, like (lambda (f) (f 3)): here f is interpreted as Value::False, which triggers the error message.

Notes

I see two reasons to reject this PR:

brundonsmith commented 2 years ago

I think what I'd rather do here is enforce it at a type level. The way I see it working is:

This way we're forced by Rust to validate the lisp form before we can even construct a Lambda struct, and we avoid a bunch of runtime checking complexity down the line

I can go ahead and implement this, or if you prefer, you're welcome to implement it yourself in the PR and we can get it merged that way (I don't want to rob you of the contribution if you care about having it under your name, since you've put effort into it already, but if you don't care about that then I'm also happy to make the changes)

brundonsmith commented 2 years ago

To your concern about it being a breaking change- I've just been incrementing the semver as needed when making breaking changes, so I'm not too worried about that

adri326 commented 2 years ago

We can also use the opportunity to introduce a call method on Lambda and maybe even currying.

I'm not too worried about the contribution thing, implementing my PR only took me about an hour. I think github supports co-authoring in the commit message, if you want to do it that way.