-
Notifications
You must be signed in to change notification settings - Fork 100
feat: array destructuring #690
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
base: staging
Are you sure you want to change the base?
feat: array destructuring #690
Conversation
I also tried to not modify any extra files, so no refactor was needed. |
Made it a draft cause obviously I haven't added any tests for my changes yet. |
syntax(meta, &mut *self.expr)?; | ||
|
||
if is_destructured && !self.expr.get_type().is_array() { | ||
panic!("Expected array type for destructured variable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obviously a panic is bad, I am not yet familiar with the existing codebase, so this is temporary
this likely won't work because we can't check the array length at compile time, to ensure that there is enough items in the array:
i myself have experimented with this kind of features (namely #466, #432), and none of them worked because our compiler is very dumb meaning it doesn't understand how code works |
@b1ek It's not a problem to make our compiler "smarter". We just have to store additional information in ctx of how long arrays are declared or if we ever know how long they can be when we assign an array to a variable. |
@ArjixWasTaken before we code review this PR, please check if the problems that @b1ek mentioned can be resolved somehow (and how). I like this idea for a feature and would like to see it working correctly in Amber. |
cd8c841
to
347154b
Compare
After some thought, I think the best course of action is to raise a warning if we cannot determine the size of the array at build time. And the user would be able to hide the warning by using a compiler flag. That way const [a, b] = [1, 2]; would be safe but const [a, b] = split("1,2", ","); would be unsafe |
This comment was marked as outdated.
This comment was marked as outdated.
is there a reason to do it like that? unless there is a special bash mode that throws an error instead? |
Not too sure. I assumed that it would. I'd have to test it first on my computer when I get back home. Scratch the idea I have written above. It wouldn't work with the current type system. It would need to assign each variable with Failable type since it doesn't know if it's |
@ArjixWasTaken if we want to support dynamic sized arrays, then we can check if the array length matches destructed array and fails if not. Like: const [a, b] = get_random_array() failed {
echo "Variables couldn't be destructed from this array since it's length is less then the amount of the variables"
} But this introduces inconsistency with the case when we know how many elements array has: const [a, b] = ["a", "b"] // No `failed` here! |
Now assuming that we want to implement this syntax version: const [a, b] = get_random_array() failed {
echo "Variables couldn't be destructed from this array since it's length is less then the amount of the variables"
} Does this |
Closes #239
--
An initial implementation for array-destructuring.
I am not sure if my approach is "correct", I took inspiration from the AST of EcmaScript you may say.
EcmaScript has the concept of multiple variable definitions in one declaration (
let a = 1, b = 1;
).Which inspired me to do the same (talking about the data structures), but for destructuring.
I am open to re-doing this from scratch in a different way if you want to.
--
This PR allows the following syntax:
which gets compiled to
--
implementation details
VariableInit
now holds a list ofVariableDef
--
PS: I am not very proficient in bash, so I do not claim that this is a good implementation.