Amber icon indicating copy to clipboard operation
Amber copied to clipboard

feat: array destructuring

Open ArjixWasTaken opened this issue 8 months ago • 11 comments

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:

let array = [1, 2, 3];
let [a, b] = array;

echo b;

which gets compiled to

#!/usr/bin/env bash
__AMBER_ARRAY_0=(1 2 3);
__0_array=("${__AMBER_ARRAY_0[@]}")
__ref_1i_i2_a_b=("${__0_array[@]}");
__1_a=${__ref_1i_i2_a_b[0]};
__2_b=${__ref_1i_i2_a_b[1]};

echo "${__2_b[@]}"

--

implementation details

  • VariableInit now holds a list of VariableDef
  • TranslateModule:
    • Added a temporary variable, named after all the definitions combined, to hold a reference to the array
    • Each definition is declared as a separate variable, and it uses the array reference to initialize

--

PS: I am not very proficient in bash, so I do not claim that this is a good implementation.

ArjixWasTaken avatar Apr 09 '25 20:04 ArjixWasTaken

I also tried to not modify any extra files, so no refactor was needed. All the tests using cargo test pass.

ArjixWasTaken avatar Apr 09 '25 20:04 ArjixWasTaken

Made it a draft cause obviously I haven't added any tests for my changes yet.

ArjixWasTaken avatar Apr 09 '25 20:04 ArjixWasTaken

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:

let array = [1, 2, 3];
if (some_dynamic_condition) {
    array = [1];
}
let [a, b] = 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 avatar Apr 10 '25 02:04 b1ek

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

Ph0enixKM avatar Apr 10 '25 09:04 Ph0enixKM

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

Ph0enixKM avatar Apr 10 '25 09:04 Ph0enixKM

@Ph0enixKM

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

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

ArjixWasTaken avatar Apr 15 '25 19:04 ArjixWasTaken

const [a, b] = split("1,2", ",");

@ArjixWasTaken could we simply translate this code to something like this in Bash (Here is a pseudocode):

var split_result = split("1,2", ",")
switch len(split_result) {
  case 0: {
    var a = null
    var b = null
  }
  case 1: {
    var a = split_result[0]
    var b = null
  }
  default: {
    var a = split_result[0]
    var b = split_result[1]
  }
}

We could think if this can be much simpler and shorter...

Ph0enixKM avatar Apr 16 '25 10:04 Ph0enixKM

is there a reason to do it like that? from my personal tests, bash works just fine even if you access an index out of range

unless there is a special bash mode that throws an error instead?

ArjixWasTaken avatar Apr 16 '25 10:04 ArjixWasTaken

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 Null / Num / Text or what

Ph0enixKM avatar Apr 16 '25 10:04 Ph0enixKM

@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!

Ph0enixKM avatar Apr 16 '25 10:04 Ph0enixKM

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 failed apply to the function or the array extraction? That's a thing that we should consider when attempting to go this way

Ph0enixKM avatar Apr 16 '25 10:04 Ph0enixKM

Oh god, I can't imagine how much the compiler has changed since I made this PR. It'll probably be easier to start from scratch ngl

ArjixWasTaken avatar Dec 03 '25 12:12 ArjixWasTaken