Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Warn about unused Result #732

Open
VorpalBlade opened this issue Jul 14, 2024 · 1 comment
Open

Feature request: Warn about unused Result #732

VorpalBlade opened this issue Jul 14, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@VorpalBlade
Copy link
Contributor

VorpalBlade commented Jul 14, 2024

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
Copy link
Collaborator

udoprog commented Jul 15, 2024

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.

@udoprog udoprog added the enhancement New feature or request label Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants