Implement basic std functions to manipulate arrays #626

hdwalters · 2024-12-02T08:48:52Z

Implements some standard library functions that allow to manipulate arrays:

first(array) to get the first element (merely a shorthand for array[0])
last(array) to get the last element (merely a shorthand for array[len(array) - 1])
remove_at(array, index) to remove an element at the index, translates to unset array[index]
extract_at(array, index) — same as remove_at, but also returns the element
pop(array) — remove and return the last element
shift(array) — remove and return the first element

Mte90 · 2024-12-02T09:07:53Z

Test fails right now

hdwalters · 2024-12-02T14:23:21Z

Yes I know, still working on it.

hdwalters · 2024-12-02T20:52:18Z

Cargo.toml

@@ -32,6 +32,7 @@ wildmatch = "2.4.0"
 [dev-dependencies]
 assert_cmd = "2.0.14"
 predicates = "3.1.0"
+pretty_assertions = "1.4.1"


Creates more readable output for assert_eq! macro. See https://crates.io/crates/pretty_assertions for more information.

hdwalters · 2024-12-02T20:58:30Z

src/modules/expression/binop/range.rs

-        if self.neq {
-            let to_neq = if let Some(ExprType::Number(_)) = &self.to.value {
-                to.parse::<isize>().unwrap_or_default().sub(1).to_string()
+        let to = if let Some(to) = self.to.get_integer_value() {


Use compile-time arithmetic to convert Amber [1..11] to Bash $(seq 1 10); fall back to calling bc if not supplied a number.

I think it's ok to assume we're given an integer not a decimal here, but we can always come back to this when we have a separate integer type.

hdwalters · 2024-12-02T21:16:57Z

src/modules/expression/binop/range.rs

+
+impl Range {
+    pub fn get_array_index(&self, meta: &mut TranslateMetadata) -> (String, String) {
+        if let Some(from) = self.from.get_integer_value() {


Use compile-time arithmetic to convert Amber array[10..=12] to Bash ${array[@]:10:3}; fall back to calling bc if not supplied a number.

hdwalters · 2024-12-02T21:21:50Z

src/modules/expression/expr.rs

@@ -101,6 +101,14 @@ impl Typed for Expr {
 }

 impl Expr {
+    pub fn get_integer_value(&self) -> Option<isize> {


This function is intended to return an integer for compile-time arithmetic, but only when given integer literals like 42 or -42. We're not worrying about float literals like 123.45 because this will only be called in contexts where an integer is required.

I was tempted to put this functionality into the TranslateModule trait, but held off because it's fairly niche at the moment. However, I think doing compile time arithmetic where possible cannot be a bad idea; and we may decide to refactor if it becomes more widely used.

hdwalters · 2024-12-02T21:34:07Z

src/modules/expression/unop/neg.rs

 use crate::docs::module::DocumentationModule;
 use crate::error_type_match;
+use crate::modules::expression::expr::Expr;


I think we should avoid nested use statements like the one I'm replacing here, as they're hard to read and maintain. RustRover has handy tools to "flatten use statements" and "optimize imports".

hdwalters · 2024-12-02T21:43:18Z

src/modules/variable/mod.rs

+fn allow_index_accessor(index: &Expr, range: bool) -> bool {
+    match (&index.kind, &index.value) {
+        (Type::Num, _) => true,
+        (Type::Array(_), Some(ExprType::Range(_))) => range,


Allow range indexing for echo array[2..5] but not array[2..5] = [100, 101].

hdwalters · 2024-12-02T21:48:41Z

src/std/array.ab

@@ -1,7 +1,7 @@
 /// Returns index of the first value found in the specified array.
 ///
 /// If the value is not found, the function returns -1.
-pub fun array_first_index(array, value): Num {
+pub fun array_index(array, value): Num {


I renamed this standard library function to more closely align with array_search. I think it's ok to do this while we're still in alpha, but further down the line we may want to have the compiler automatically use deprecated function names (with a warning) via a new annotation:

#[replaces_deprecated(array_first_index)] pub fun array_index(array, value): Num {

hdwalters · 2024-12-02T21:51:11Z

src/std/array.ab

    return result >= 0
 }
+
+/// Returns the first element in the array
+pub fun first(array) {


Should we also call these array_first() and array_last() while we're at it?

The major argument for more verbose standard library function names (e.g. containing at least one underscore) is that we're unlikely to want to reuse them for builtins (a case in which a #[replaces_deprecated] annotation would not help). Note that I have already renamed lines as split_lines to make way for the lines builtin.

With this in mind, we may want a separate discussion on renaming the wider set of standard library functions, before we release 0.4.0. I will create a discussion channel for this on Discord.

hdwalters · 2024-12-02T21:55:53Z

src/tests/stdlib/array_remove_at.ab

@@ -0,0 +1,25 @@
+import { remove_at } from "std/array"
+
+// Output


Together with array_extract(), this unit test is currently failing. I have boiled down the code under test to the simplest possible version, which I will post on Discord, along with a plea for help.

hdwalters · 2024-12-02T21:56:22Z

src/tests/stdlib/array_remove_at.ab

+// Array after 4: (4) [apple banana cherry date]
+
+fun test_remove(inner: [Text], index: Num): Null {
+    let value = remove_at(inner, index)


Copy/paste error. Note to self: remove let value = as value is unused here.

hdwalters · 2024-12-03T17:58:11Z

src/modules/expression/binop/add.rs

                let id = meta.gen_value_id();
                let name = format!("__AMBER_ARRAY_ADD_{id}");
                meta.stmt_queue.push_back(format!("{name}=({left} {right})"));
-                format!("{quote}${{{name}[@]}}{quote}")
+                format!("{quote}{dollar}{{{name}[@]}}{quote}")


This fixes the failing unit tests. We were passing the contents of the temporary array, rather than the name, in an eval context; and the contents were all getting smooshed together into a single array item.

mks-h · 2024-12-04T10:52:27Z

Please don't combine multiple issues into a single PR. This makes it nearly impossible to review, as there's too much context. The standard library changes should be in their own PR (as they were), while your range changes should be in another PR (which one should be merged first is up to you). Adding new dependencies which are not required for the work at hand should be in their own PR. And even more, stylistic changes of unrelated code ("optimize imports") should be their own PR.

I'm sorry, but I'll have to close this PR. Please decouple your changes as much as possible, and open separate PRs.

hdwalters · 2024-12-04T13:31:00Z

I'm sorry, but I'll have to close this PR. Please decouple your changes as much as possible, and open separate PRs.

Fair point. Since I needed the compiler changes to implement the stdlib functions, I put them in the same branch. But I don't mind separating them; it's just cherry picking.

hdwalters · 2024-12-04T13:32:58Z

The core changes should be merged first, but I'm not sure I agree that import optimisation needs its own PR, unless we touch the entire codebase (which I'm not suggesting btw).

mks-h and others added 13 commits December 1, 2024 10:03

std: Add function to get last item of array

b752d0f

std: Add function to remove element from array

eaf9317

std: Add function to extract element from array

525cf23

std: Add functions to pop and shift elements from array

061e40f

Use pretty assertions crate.

f3e9ac5

Rename existing array tests.

947d197

Rename and add array functions; rewrite tests.

a47eadd

Refactor variable translation; optimise imports.

89cce1a

Refactor array index by negative number.

6cb2d12

Support array index by range.

b31d4ff

Rewrite "remove at" standard library function.

98433af

Address Clippy warnings.

a09f0b0

Assert array length as well as contents in tests.

e14373b

hdwalters requested review from Mte90, Ph0enixKM, mks-h and b1ek December 2, 2024 08:48

hdwalters mentioned this pull request Dec 2, 2024

Implement basic std functions to manipulate arrays #625

Closed

hdwalters mentioned this pull request Dec 2, 2024

Implement basic std functions to manipulate arrays #601

Closed

hdwalters commented Dec 2, 2024

View reviewed changes

hdwalters added 2 commits December 3, 2024 17:48

Replace test strings in array tests.

e909825

Quote dollar for passed-by-ref array concatenation.

4c35d32

hdwalters commented Dec 3, 2024

View reviewed changes

Mte90 approved these changes Dec 4, 2024

View reviewed changes

mks-h closed this Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basic std functions to manipulate arrays #626

Implement basic std functions to manipulate arrays #626

hdwalters commented Dec 2, 2024

Mte90 commented Dec 2, 2024

hdwalters commented Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024 •

edited

Loading

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024 •

edited

Loading

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024 •

edited

Loading

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 2, 2024

hdwalters Dec 3, 2024

mks-h commented Dec 4, 2024

hdwalters commented Dec 4, 2024

hdwalters commented Dec 4, 2024 •

edited

Loading

		@@ -0,0 +1,25 @@
		import { remove_at } from "std/array"

		// Output

Implement basic std functions to manipulate arrays #626

Implement basic std functions to manipulate arrays #626

Conversation

hdwalters commented Dec 2, 2024

Mte90 commented Dec 2, 2024

hdwalters commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdwalters Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdwalters Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdwalters Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mks-h commented Dec 4, 2024

hdwalters commented Dec 4, 2024

hdwalters commented Dec 4, 2024 • edited Loading

hdwalters Dec 2, 2024 •

edited

Loading

hdwalters Dec 2, 2024 •

edited

Loading

hdwalters Dec 2, 2024 •

edited

Loading

hdwalters commented Dec 4, 2024 •

edited

Loading