rune-rs / rune

An embeddable dynamic programming language for Rust.
https://rune-rs.github.io
Apache License 2.0
1.7k stars 84 forks source link

Feature request: Warn about unused Result #732

Open VorpalBlade opened 1 month ago

VorpalBlade commented 1 month ago

Now, this might be tricky in a dynamic language, but I found this one to be quite a nasty footgun.

Given this example rune code (with a nasty little bug):

pub fn generate_config(cmds, settings) {
    cmds.mkdir("/etc/some")?;
    cmds.write("/etc/some/config.conf", b"value=Hello, world!")?;

    cmds.add_pkg("pacman", "xviewer")?;
    cmds.add_pkg("pacman", "zsh");
    cmds.add_pkg("pacman", "zsh-completions")?;
    Ok(())
}

Here cmds is an external (Rust defined) type that gets passed in to this custom entry point.

There is an issue on the line adding zsh in that there is no ?. In this short example it isn't too tricky to spot. In a much longer function defining all the packages to be installed on the system (with conditional logic for system specific packages) this is not immediately obvious at all, and will lead to silently ignoring errors.

This means that returning results is not a good API here (way too much of a footgun). I should either panic (in the Rust code) or use tracing/log to record issues. That creates overhead/complexity in the Rust code: If I want to both log and still return errors for the Rune code to handle I can't purely use the convenience of ? for early returns in the Rust code any more. Or I have to put in wrapper functions everywhere.

Instead I would suggest adding warnings about unhandled results. Since the language is dynamic this would have to happen at runtime I believe.

udoprog commented 1 month ago

My preference would be to introduce type annotations and add the equivalent of must_use.

Once #733 is merged it will also possible to add another runtime hook when a Result is constructed and written with discard, but this to me seems less useful in general, however for your use case using Rune as a configuration language it would work since you only run everything once and can easily collect and display those warnings.