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

Failable types -> failable functions #642

Open
wants to merge 10 commits into
base: staging
Choose a base branch
from

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented Dec 22, 2024

as discussed in #478

i didn't actually change the syntax, just the way how it is handled internally: no more failable types, but failable functions instead

@Ph0enixKM
Copy link
Member

@b1ek i think that we should keep failable types because later on we can match on them:

match failabe_func() {
    failed {
        echo "This function failed!"
    }
    ok {
        echo "This function succeeded!"
    }
}

Or even in the future run methods on them:

let a = failable_func()
echo a.is_ok()

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Dec 23, 2024

Also I'm pretty confident that I'd want to do this:

$ command $ or "default value"

And current state of things gives us ease of detecting failable value

@b1ek
Copy link
Member Author

b1ek commented Dec 24, 2024

we should keep failable types because later on we can match on them

that can still be achieved with failable functions. my issue with failable types is that they exist only for a brief period of time, don't actually store any data and shouldn't even be considered types:

fun may_fail(): Num? { // declared
    if cond {
        fail 1 // created
    }
    return 123
}

fun accepts_failure(me: Num?): Null { // not allowed to pass as parameter
    ...
}

let a = may_fail(); // not allowed to have a variable with a failable type in it
let b = 123 as Num?; // not an option either

the only use case for it is when a function may fail, which i dont think should be treated as a data type internally:

fun may_fail(): Num?;

let data = may_fail() failed {
    echo status
}
echo "not failed: {data}"

@b1ek
Copy link
Member Author

b1ek commented Dec 24, 2024

i just want to clarify that i haven't changed the syntax one bit here. all code that has been written before will work exactly the same. but it will be handled differently internally

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we decide to go with this approach, I cannot see anything major wrong with your changes, but have made some minor suggestions.

src/modules/function/declaration.rs Outdated Show resolved Hide resolved
src/modules/function/declaration.rs Show resolved Hide resolved
src/modules/function/declaration_utils.rs Outdated Show resolved Hide resolved
src/modules/function/invocation.rs Outdated Show resolved Hide resolved
Co-Authored-By: hdwalters <[email protected]>
hdwalters
hdwalters previously approved these changes Dec 24, 2024
src/modules/function/declaration_utils.rs Outdated Show resolved Hide resolved
@Ph0enixKM
Copy link
Member

@b1ek I can't found a counterargument. You convinced me on this one

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the proposed message is the biggest common denominator:

  1. It is different enough (The two sentences start with a different letter)
  2. It's written the same way

Other changes look good

src/modules/function/declaration_utils.rs Outdated Show resolved Hide resolved
@mks-h
Copy link
Member

mks-h commented Dec 25, 2024

I implemented Failable as a type under an assumption that we might or might not turn it into an actual type. I have no issues with the idea of implementing them through an attribute of a function, but do note that this will cement the current way of handling failability (as a function attribute, rather than a type) — and that is probably a good thing.

@b1ek b1ek requested a review from Ph0enixKM December 26, 2024 00:00
@hdwalters
Copy link
Contributor

Looks good thanks.

@Ph0enixKM
Copy link
Member

We can always pivot this logic if we hit the wall

@Ph0enixKM
Copy link
Member

One question though. Are we releasing 0.4.1 with this or we implement it as a feature of 0.5?

@hdwalters
Copy link
Contributor

One question though. Are we releasing 0.4.1 with this or we implement it as a feature of 0.5?

I thought we were not planning to target features for different releases, but instead merge everything to staging, and bump the version number according to impact of changes?

@Ph0enixKM
Copy link
Member

Exactly, then this PR has to be rebased

@Ph0enixKM Ph0enixKM changed the base branch from main to staging December 28, 2024 17:07
@Ph0enixKM Ph0enixKM dismissed hdwalters’s stale review December 28, 2024 17:07

The base branch was changed.

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing appears to have changed since the rebase; reapproving.

Copy link
Member

@mks-h mks-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apart from a little code quality change.

Comment on lines -17 to +26
"fail" => is_failable = true,
"fail" => {
if !declared_failable && returns_tok.is_some() {
return error!(meta, Some(tok), "Failable functions must have a '?' after the type name");
}
is_failable = true
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this part to as it was. This function is only supposed to tell whether the parsed amber function is de-facto failable. Further error handling should be done outside.

Comment on lines +219 to +221
let (index_begin, index_end, is_failable) = skip_function_body(meta, declared_failable, &returns_tok)?;
if !is_failable && declared_failable {
return error!(meta, returns_tok, "Infallible functions must not have a '?' after the type name");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already do some error handling here, so lets keep all of it here.

Comment on lines -9 to 14
pub fn skip_function_body(meta: &mut ParserMetadata) -> (usize, usize, bool) {
pub fn skip_function_body(
meta: &mut ParserMetadata,
declared_failable: bool,
returns_tok: &Option<Token>,
) -> Result<(usize, usize, bool), Failure> {
let index_begin = meta.get_index();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you extract the error handling, you also won't need to pass these arguments or make the function return a Result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants