feat: variable string substitution functions

[wolfv]

Started to implement some functions in the style of:

• ${VAR:2:3} (substring starting at index 2 length 3)
• ${VAR:-DEFAULT} (use VAR if it exists, else DEFAULT).

Also parses ${VAR:=FOO} and ${VAR:+FOO} which are not properly implemented yet.

More docs: https://mywiki.wooledge.org/BashGuide/Parameters#Parameter_Expansion

However, this implementation falls a little short.

For dealing with the assignment we would need to change the implementation a bit.

Also arguments can be expanded themselves, e.g. ${FOO:-${OTHER}} should work, so the options would technically have to be parsed as Variables themselves and evaluated on demand.

[prsabahrami]

I'm taking care of this but I think it is better to merge #169 first.

[codecov]

Codecov Report

Attention: Patch coverage is 69.16667% with 111 lines in your changes missing coverage. Please review.

Project coverage is 62.64%. Comparing base (9ecd2c3) to head (8f9986a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/deno_task_shell/src/shell/types.rs	45.34%	47 Missing ⚠️
crates/deno_task_shell/src/shell/execute.rs	80.95%	40 Missing ⚠️
crates/deno_task_shell/src/parser.rs	63.93%	22 Missing ⚠️
crates/deno_task_shell/src/shell/command.rs	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   61.84%   62.64%   +0.79%     
==========================================
  Files          30       30              
  Lines        2967     3183     +216     
==========================================
+ Hits         1835     1994     +159     
- Misses       1132     1189      +57     

Files with missing lines	Coverage Δ
crates/deno_task_shell/src/shell/command.rs	34.75% <33.33%> (+0.40%)	⬆️
crates/deno_task_shell/src/parser.rs	60.56% <63.93%> (-0.16%)	⬇️
crates/deno_task_shell/src/shell/execute.rs	75.10% <80.95%> (+2.36%)	⬆️
crates/deno_task_shell/src/shell/types.rs	46.27% <45.34%> (+1.04%)	⬆️

... and 1 file with indirect coverage changes

[prsabahrami]

I'll look into why this is failing on Windows tomorrow. Meanwhile @certik let me know if you have any ideas.

@wolfv One thing I noticed while working on this issue is that the state – more specifically env vars can be modified through arithmetic operations and hence each function at any point should contain a list of changes because this change can happen almost anywhere now. This led to implementation of WordPartsResult, WordResult, TextPart and Text structs.
For example:

flag=0 && echo ${FOO:-$((flag=1, 5))}

This is supported now.
This has caused me being forced to clone the state in some places.
For now, the default value expansion e.g. echo $(FOO:-5} is complete. I'll finish the rest tomorrow as well.
In the meantime, please let me know if you see ways we can workaround the clones.
I'll try to add a more detailed explanation tomorrow on which functions exactly take mutable reference and which don't.

[prsabahrami]

Just added support for all three of substring expansion, default value expansion, and alternate value expansion.
A new grammar was needed for parameters because they are essentially treated as quoted words but it also has tilde expansion etc. for example:

echo ${var:-val is not set}
echo ${HOMEDIR:-~}

This should adding for loop support much easier.
In regards to the state being modified at any time:
Any execution lower than the AST node pipeline is getting state as mutable ref. However, every single execution step adds up the changes from the lowest level and all the way to the root of execution which is a sequential_list.

[wolfv]

Looks great, works great!

One thing I tried (not really important) doesn't work, which is arithmetic expression in the substring function:

~/Programs/shell(string-subst)$ echo ${FOO:2:$((2+2))}
Syntax error:   × Failed to parse input
  ╰─▶ Failure to parse at Pos((1, 14))
   ╭────
 1 │ echo ${FOO:2:$((2+2))}
   ·              ┬
   ·              ╰── expected NUMBER
   ╰────
  help: expected NUMBER

The error message is top notch though!!

[certik]

Great job!

I have two questions:

• Is the implemented functionality strictly a subset of Bash? Or is there some case which shell can do, but Bash would not?
• Have you checked all the test cases with Bash? (I am asking because for example the if test some time ago didn't actually work with Bash.)

It's really easy to accidentally implement something that is actually extending Bash, like we did with the if statement in the past. As long as we stay with a strict subset, then we are fine and can merge this without worries.

[prsabahrami]

@wolfv This is now supported along with negative value for the start as well:

FOO=12345 && echo ${FOO: -4: 2}
FOO=12345 && echo ${FOO: -2}
FOO=12345 && echo ${FOO: -2:-1}

[prsabahrami]

@certik

Is the implemented functionality strictly a subset of Bash? Or is there some case which shell can do, but Bash would not?

I am essentially checking Bash's behaviour from Bash's reference manual. My goal is also to keep the same behaviour as Bash.
However, I think it's best for me to implement a framework to be able to run the tests in the tests crate locally and cross check the outputs with Bash.

Have you checked all the test cases with Bash? (I am asking because for example the if test some time ago didn't actually work with Bash.)

I have checked the outputs of the tests added in the tests crate locally and the outputs are correct. However, if you recall regarding the if statement case, Bash's behaviour on my local machine was different than yours and I think the only way to prevent this is to figure out a way to check the outputs with Bash's behaviour in CI or at least have a proper framework to run locally.

[certik]

@prsabahrami excellent. Looks like you did the best we can do so far, so that's good enough.

Eventually we should indeed create our test suite in such a way so that we can execute it via Bash / Zsh at our CI, to actually guarantee that all our tests work there too. Until then, we just have to do our best manually.

[prsabahrami]

@certik I agree. I think in terms of priority, #62 is in priority. I have started working on this. I think after that I can create a proper test suite because as we go on the testing gets more complicated as well.

[certik]

The changes are extensive and the Codecov comments are really distracting to see the bigger picture (why don't we just disable them completely, and just review Codecov on an external website?). However the test changes look good, the grammar I think is ok. I have not carefully reviewed all the changes in the shell subdirectory.

@wolfv, @Hofer-Julian if the changes look good to you, then we can merge.

@prsabahrami I agree about #62 being the priority.

[prsabahrami]

@certik Got rid of the comments!
It should be easier to review the changes now.

[prsabahrami]

@Hofer-Julian Any thoughts on this?

[Hofer-Julian]

@prsabahrami sorry, didn't get around playing with that yet. Will check it out tomorrow!

[Hofer-Julian]

I've tried the following: echo "${x:-100}"
In bash this gives "100", in shell it gives 1.

Is this expected?

I also saw that we underused miette::bail! and changed a few in a commit

[prsabahrami]

@Hofer-Julian I've added double quoted tests here as well. Also, tried echo "${x:-100}" locally and it seems to print 100. Is it possible that you might have assigned a value to x beforehand when using shell?

[Hofer-Julian]

@Hofer-Julian I've added double quoted tests here as well. Also, tried echo "${x:-100}" locally and it seems to print 100. Is it possible that you might have assigned a value to x beforehand when using shell?

I tried it again, and indeed I couldn't reproduce it. Sorry for the noise :)

[Hofer-Julian]

Great work Parsa and Wolf ✨