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] Change unsafe keyword to trust #296

Closed
2 of 3 tasks
KrosFire opened this issue Jul 11, 2024 · 14 comments · Fixed by #498
Closed
2 of 3 tasks

[Feature] Change unsafe keyword to trust #296

KrosFire opened this issue Jul 11, 2024 · 14 comments · Fixed by #498
Assignees
Labels
compiler enhancement New feature or request

Comments

@KrosFire
Copy link
Member

KrosFire commented Jul 11, 2024

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.

@KrosFire KrosFire added the enhancement New feature or request label Jul 11, 2024
@Ph0enixKM
Copy link
Member

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

@b1ek
Copy link
Member

b1ek commented Jul 13, 2024

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

@Ph0enixKM
Copy link
Member

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

@b1ek
Copy link
Member

b1ek commented Jul 14, 2024

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

@Ph0enixKM
Copy link
Member

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$
	// ...
}

@KrosFire
Copy link
Member Author

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.

@Mte90 Mte90 added the compiler label Jul 19, 2024
@Ph0enixKM
Copy link
Member

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

Ph0enixKM commented Jul 20, 2024

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.

@KrosFire
Copy link
Member Author

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.

@Ph0enixKM
Copy link
Member

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

@KrosFire
Copy link
Member Author

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".

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 20, 2024

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

@Mte90
Copy link
Member

Mte90 commented Jul 22, 2024

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 {}.

@Ph0enixKM Ph0enixKM changed the title [Feature] Change unsafe keyword to ignore [Feature] Change unsafe keyword to something else Jul 22, 2024
@Ph0enixKM Ph0enixKM changed the title [Feature] Change unsafe keyword to something else [Feature] Change unsafe keyword to trust Aug 8, 2024
@Ph0enixKM
Copy link
Member

The community has chosen the trust keyword:

#333

@Ph0enixKM Ph0enixKM self-assigned this Oct 1, 2024
@Ph0enixKM Ph0enixKM linked a pull request Oct 1, 2024 that will close this issue
@Ph0enixKM Ph0enixKM added this to the Amber 0.4.0-alpha milestone Oct 3, 2024
@Mte90 Mte90 closed this as completed in #498 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants