Amber icon indicating copy to clipboard operation
Amber copied to clipboard

[Feature] Change `unsafe` keyword to `trust`

Open KrosFire opened this issue 1 year ago • 14 comments

According to docs: """ unsafe - the discouraged way to handle failing. This modifier will treat commands as if they have completed successfully and will allow them to be parsed without any further steps. """

The way the unsafe keyword works, seems pretty safe to me. I'd think, that if I use unsafe keyword my program can crash at this point, because it's not safe and I don't handle possible errors. But all it does is it ignores any potential failures and keeps going. While it can be "unsafe" in a long run, it's not very intuitive (imho).

I think that a better keyword for this behaviour is ignore or some variation of it.

Oh, and one more thing. "will treat commands as if they have completed successfully and will allow them to be parsed without any further steps.". I believe this sentence is incorrect. It should be something like this: "will treat commands as if they have completed successfully and will run further code"


So we're sticking with trust keyword then.

  • [x] amber-lang/vsc-amber-extensions#1
  • [x] amber-lang/zed-amber-extension#1
  • [ ] Make sure that when someone uses unsafe the code works but gets deprecation warning

KrosFire avatar Jul 11 '24 12:07 KrosFire

This can be a part of other breaking design changes that we should batch together to avoid forcing people to update their scripts every single time

Ph0enixKM avatar Jul 12 '24 16:07 Ph0enixKM

unsafe - the discouraged way to handle failing.

imo $...$ failed {} is a BAAAD way to handle errors. it encourages repetitive code and reimplementing the same thing over and over again

we should make our own error handling that would output a nice error in the terminal, like this:

$do_stuff$ // no "unsafe" keyword

infallible $do_stuff$

$do_stuff$ failed { echo "oh no" }
amber_error() {
    echo "\x1b[31mRuntime error!\x1b[0m" > stderr
    echo "$1" > stderr
}

amber_run_cmd() {
    ret=$($@)
    AS=$?
    if [ "$AS" != "0" ]; then
        amber_error "Command $1 exited with code $AS!"
        exit $AS
    fi
    echo $ret
}


amber_run_cmd do_stuff


do_stuff


do_stuff
AS=$?
if [ "$AS" != "0" ]; then
    echo oh no
fi

b1ek avatar Jul 13 '24 16:07 b1ek

This makes no sense @b1ek as there is ? operator that does just that.

Ph0enixKM avatar Jul 14 '24 15:07 Ph0enixKM

This makes no sense @b1ek as there is ? operator that does just that.

thats not at all what i was talking about.

lets view the error codes as exceptions in usual programming languages. right now amber recommends the following approach: handle the exception manually in a sort of function block. that will cause inconsistency between how different users implement the failed block.

its pretty much the same thing as if rust wouldn't have panic!s or .unwraps and would require you to do something like this for each Result<_, _>:

let data = match res {
    Ok(v) => v,
    Err(err) => { println!("{err:?}"); exit(1) } // this can clearly be implemented in an undefined amount of different ways
}

you can clearly see that its: 1. not very comfortable to use; 2. causes the user to implement error handling themselves, which will be different and breaks DRY.

this is something that is always been handled by the language, each time you throw an exception, it will print it out in a nice way and terminate the program, if it is not caught

b1ek avatar Jul 14 '24 16:07 b1ek

There are unwraps in Amber and these are called unsafe (we could rename them to ignore). If you need to unsafe (or ignore) more commands just do this:

unsafe {
	$cmd1$
	$cmd2$
	$cmd3$
	// ...
}

Ph0enixKM avatar Jul 16 '24 08:07 Ph0enixKM

failed keyword is the same as catch keyword in for example JS. I don't see it as an obstacle, especially when this syntax is not required, just recommended. Throwing errors is possible with adding ? to every call of failable function.

Inconsistencies in how different programmers write code is immanent in any language. I don't see any possible solution to standardize how people would want to handle errors.

But I can agree that there could be a usecase for a method to stop the script immediately without possible recovery. Something like exit 1, that ends everything with a given status.

KrosFire avatar Jul 17 '24 15:07 KrosFire

Actually ignore is not the best keyword either. It means that the command will be ignored which is not true. We have to find a better keyword for this.

Ph0enixKM avatar Jul 19 '24 21:07 Ph0enixKM

The appropriate keyword for this use case would be neverfails or infailable.

However I looked up how the keyword unsafe is actually used in the Rust ecosystem. Here is the link to the documentation. What I found out is that they're doing something similar to what Amber does.

The unsafe keyword has two uses:

  • to declare the existence of contracts the compiler can’t check (unsafe fn and unsafe trait),

In Amber world this would mean a command that does not fail by exiting with code other than zero but rather requires additional command to see if the previous command failed. The good example would be systemctl to check if service that was started is running or not.

  • and to declare that a programmer has checked that these contracts have been upheld (unsafe {} and unsafe impl, but also unsafe fn – see below).

In Amber world this would also mean that if programmer is sure that this command will work - then they can run this command unsafely without handling any error.

Summary

I think that the keyword unsafe is actually a good choice and shouldn't be changed. If you disagree, please let me know what am I missing.

Ph0enixKM avatar Jul 20 '24 12:07 Ph0enixKM

Let me illustrate with an example:

$ls . | grep -q "file"$ failed {
  unsafe $touch "file"$
}

unsafe $cat file$

We have 2 usecases of unsafe keyword. Both are used correctly with how rust thinks of this keyword - telling compiler "I know what I'm doing, you're too stupid to know that, but I know that it won't fail, chill". But in rust when we do something unsafe, like for example getting element at some index that is possibly out of bands, it's not that just compiler ignores possible contract violations, if the contracts are not satisfied, program will fail.

let array: [i32; 2] = [0; 2];
unsafe {
    array.get_unchecked(10);
};

unsafe in rust is truly unsafe, because when you use it, rust compiler can't guarentee that everything will work fine. If you are smart, like in the first example and know that there's nothing to fear, go ahead and do it. But if you didn't think of everything and the command fails, there should be || exit added. Otherwise it's not unsafe to me.

KrosFire avatar Jul 20 '24 16:07 KrosFire

@KrosFire do you propose any better idea for this keyword?

Ph0enixKM avatar Jul 20 '24 16:07 Ph0enixKM

I think that this behaviour is better described by assume. We assume that this code works and never check if it really does.

But if we want unsafe keyword it should be adding || exit. It's like compiler saying "Ok, I trust that you know what you are doing, but I will check you at runtime".

KrosFire avatar Jul 20 '24 16:07 KrosFire

Assume sounds good. This issue need to also cover update in the docs and in the plugins. In the compiler if „unsafe” encountered let’s tell user to use „assume” instead and spit out an error

Ph0enixKM avatar Jul 20 '24 18:07 Ph0enixKM

I don't like assume as keyword as it isn't clear for who aren't skilled in Rust or scompiled language, but here we are talking about a scripting language. unsafe it is more clear but I understand the issue for this cases.

I am thinking for something like exec after all the command will be executed anyway, or maybe something force as in the other case there is the failed {}.

Mte90 avatar Jul 22 '24 10:07 Mte90

The community has chosen the trust keyword:

https://github.com/amber-lang/amber/discussions/333

Ph0enixKM avatar Aug 08 '24 17:08 Ph0enixKM