Builtin lines function part II - Functionality

[hdwalters]

Add new LinesInvocation builtin to iterate over lines in a file, either efficiently calling code in an iteration loop, or setting or appending lines to an array.

[Ph0enixKM]

@hdwalters adding these functions to the Expr seems a bit much. The functions add little context and if you didn't explain what are they doing I wouldn't actually know without reading all the code where they're used. I think that we can just keep them in the lines.rs file and call them if detected that we are working on LinesInvocation:

// iter_loop.rs

fn translate(...) {
    // ...
    let iterator = match self.expr {
        ExprType::LinesInvocation => self.expr.translate_iter_loop(),
        _ => self.expr.translate()
    }
    // ...
}

And then we could remove the modifications made to Expr, Add, Shorthand Add and the variables. All that can be simply resolved if append_let utilized self.stmt_queue.push(...) to store the generated for loop instead of returning it as an expression result. The statement queue is a queue to which you can add statements that should be rendered before this particular statement. So for instance if we're parsing variable shorthand add:

let var += lines file

If we used the statement queue:

stmt_queue.push([
    format!("for line in read -r {lines}; do"),
    "result+=(line)",
    "done"
].join("\n"));
// ...
return "result"

we'd get the following bash output:

for line in read -r file; do
    result+=(line)
done

var+=("${result[@]}")

[hdwalters]

@hdwalters adding these functions to the Expr seems a bit much. The functions add little context and if you didn't explain what are they doing I wouldn't actually know without reading all the code where they're used. I think that we can just keep them in the lines.rs file and call them if detected that we are working on LinesInvocation:

Good suggestion; have implemented. The new version is a lot simpler, and touches fewer files.

[b1ek]

please don't merge this without me; im really not sure if this is ok to create a builtin like this

[hdwalters]

please don't merge this without me; im really not sure if this is ok to create a builtin like this

Ok. If you could explain your concerns, I can hopefully address them.

[Ph0enixKM]

Good job, @hdwalters 👏👏👏

I'll start working on the new translation layer once we merge this one and publish 0.4.0-alpha

[b1ek]

1. no failability like in std functions, like when a file doesn't exist
2. is this expected behaviour? there should be a test case for it

   trust $echo first > a$
   for line in lines("a") {
       trust $echo >> a$
       // loops indefinetely
   }
   
3. compiling this yields invalid bash code:

   for line in lines("file") { /* empty */ }
   

[hdwalters]

1. no failability like in std functions, like when a file doesn't exist

Fair enough, I'll investigate how to do that.

2. is this expected behaviour? there should be a test case for it

trust $echo first > a$
for line in lines("a") {
    trust $echo >> a$
    // loops indefinetely
}

I'm not sure how far we should go to prevent users shooting themselves in the foot like this; after all, there's nothing to stop you making exactly the same mistake writing a native Bash script.

And in any case, I don't see how we could catch everywhere this might happen; what if the file gets appended to in a function call, or series of nested function calls? I do not believe this kind of static code analysis is feasible.

3. compiling this yields invalid bash code:

for line in lines("file") { /* empty */ }

It generated valid Bash for me, after I removed your /* empty */:

hwalters@Ghostwheel ~/git/amber-lang (implement-builtin-lines-function-ii)
$ cat lines.ab
for line in lines("file") { }

hwalters@Ghostwheel ~/git/amber-lang (implement-builtin-lines-function-ii)
$ amber lines.ab -
#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.3.5-alpha
# date: 2024-11-14 08:06:50
while IFS= read -r line; do
    :
done <"file"

[hdwalters]

Okay, it does generate invalid Bash with a // comment:

hwalters@Ghostwheel ~/git/amber-lang (implement-builtin-lines-function-ii) 
$ cat lines.ab 
for line in lines("file") {
    // noop
}

hwalters@Ghostwheel ~/git/amber-lang (implement-builtin-lines-function-ii) 
$ amber lines.ab -
#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.3.5-alpha
# date: 2024-11-14 08:16:46
while IFS= read -r line; do
    # noop
done <"file"

But it does exactly the same with an array literal:

hwalters@Ghostwheel ~/git/amber-lang (implement-builtin-lines-function-ii) 
$ cat lines.ab 
for line in ["one", "two", "three"] {
    // noop
}

hwalters@Ghostwheel ~/git/amber-lang (implement-builtin-lines-function-ii) 
$ amber lines.ab -
#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.3.5-alpha
# date: 2024-11-14 08:18:20
__AMBER_ARRAY_0=("one" "two" "three")
for line in "${__AMBER_ARRAY_0[@]}"; do
    # noop
done

So the existence of a comment line seems to prevent it writing the "noop" : statement. But fixing this is outside the scope of this pull request.

[Ph0enixKM]

It seems that this case for the single comment happens not only in a loop but also in function declarations and if statements. I'll create a separate issue for it.

[b1ek]

leave a big warning that this function may fail at runtime if the file doesn't exist or no permission, so that the user will check first

[hdwalters]

1. no failability like in std functions, like when a file doesn't exist

@Ph0enixKM and I discussed this, and there doesn't seem to be an easy way to do that right now. We agreed that it would make sense to merge the PR, and come back to it at a later date.