Feat/new builtins #996

kryske · 2026-01-04T20:13:07Z

I added a bunch of Builtins + Tests, as described in #908

rm
rmdir
sleep -> ~~current test seems unstable with the use of SECONDS~~
ls
pwd
clear -> I don't know how to test this
wait -> ~~current test seems unstable with the use of SECONDS~~

I also added

cp
pid -> returns pid of last executed process

Builtins described in #908 but not added:

yes
- I don't get why this would be needed and therefor how this should be implemented.
lsof
- Cloud be implemented to return [Text], but I also don't really get why? Especially considering the rather large amount of optional arguments needed to get an output one would actually want/need.
watch, timeout, trap
- All of these expected a command as a parameter, which in my opinion without the use of Higher Order Functions isn't appropriate.
- It could be done when the builtin expects the command as an String and the use of the nameof builtin but this seems rather hacky.

I also fixed some variable names in the stdlib because they where using reserved keywords.

Ref: #908

* added builtins: clear, ls, pwd, rm, rmdir and sleep

* added tests for builtins ls, rm, rmdir and pwd

* added builtin function for wait command

* added new builtin for cp * added test for builtin cp

* added builtin to get pid of last command * updated kill test to use pid builtin * don't use reserved keyword pid in stdlib

* added grammar for builtins

* removed const with name lines from pgrep and pgrep_exact as it is a reserved keyword

* added test for sleep builtin

* silenced Terminate message, so it doesn't interfere with the expected output

codecov · 2026-01-04T21:13:00Z

Codecov Report

❌ Patch coverage is 80.70588% with 82 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/modules/builtin/ls.rs	86.71%	17 Missing ⚠️
src/modules/builtin/rm.rs	84.15%	16 Missing ⚠️
src/modules/builtin/cp.rs	78.68%	13 Missing ⚠️
src/modules/builtin/clear.rs	33.33%	12 Missing ⚠️
src/modules/builtin/sleep.rs	76.92%	6 Missing ⚠️
src/modules/builtin/wait.rs	76.92%	6 Missing ⚠️
src/modules/expression/literal/status.rs	33.33%	6 Missing ⚠️
src/modules/builtin/pid.rs	87.50%	3 Missing ⚠️
src/modules/builtin/pwd.rs	87.50%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

* added test for wait builtin

* removed deprecated calling method for new builtin cp

Mte90

There are some changes to do but others are internal discussion for Amber.

src/modules/builtin/cp.rs

src/modules/builtin/ls.rs

src/modules/builtin/pid.rs

Mte90 · 2026-01-05T13:02:41Z

src/modules/builtin/rm.rs

+
+impl TranslateModule for Rm {
+    fn translate(&self, meta: &mut TranslateMetadata) -> FragmentKind {
+        fragments!("rm -f ", self.value.translate(meta))


I think that we should have parameters in this builtin, like for the f and r parameter.

Mte90 · 2026-01-05T13:03:16Z

src/modules/builtin/rmdir.rs

+
+impl TranslateModule for RmDir {
+    fn translate(&self, meta: &mut TranslateMetadata) -> FragmentKind {
+        fragments!("rm -rf ", self.value.translate(meta))


As I said in the ticket maybe we can use the rmdir command instead of rm.

Mte90 · 2026-01-05T13:04:23Z

src/modules/builtin/wait.rs

+
+impl TranslateModule for Wait {
+    fn translate(&self, meta: &mut TranslateMetadata) -> FragmentKind {
+        fragments!("wait ", self.pids.translate(meta))


About this maybe it is better to implement it as block? #950 @Ph0enixKM

@Mte90 I'd rename this builtin to be await and let it accept single parameter or a list of parameter:

/// WARNING: This is future Amber code let pid = async foo() // Waits until foo finishes await(pid) let pids = [async foo(), async bar()] // Runs in parallel foo and bar and waits for them to finish await(pids) // Runs in parallel foo and bar and waits for them to finish await([async foo(), async bar()])

seems a good idea

src/tests/validity/cp.ab

src/tests/validity/kill_using_pid.ab

src/tests/validity/ls.ab

Mte90 · 2026-01-05T13:09:06Z

First of all, Thanks!

There are some fixes in tests to do, I suggest to move the discussion about the missing builtin in the ticket itself.
About clear you are right because it is a bash builtin, I think that we should add that in the docs or comments about it.

* fixed typo * use touch builtin when possible * make tests that use SECONDS less volatile

* added force and recursive flag to rm builtin * added test for rm recursive

* use rmdir when translating * added argument to ignore errors for non empty dirs * updated testcase

* removed argument for ignore errors for non empty dirs since it doesn't exist on MacOS

* added optional options to ls builtin

Mte90 · 2026-01-07T10:33:25Z

src/modules/builtin/rm.rs

+        let force_expr = meta.push_ephemeral_variable(force_var_stmt);
+        meta.stmt_queue.extend([
+            fragments!(
+                raw_fragment!("read -rd '' -a {} < <([[ ", force_expr.get_name()),


what is the purpose of this code?

The rm builtin now expects an optional 2nd and 3rd param for the -f and -r flag, that are an expr of type Bool. The builtin probably is most often called like rm("file.txt", true, false) where this translation would be somewhat obsolete. But since the params are an expr it could also be called like rm("file.txt", isF(), isR()) where you'd need to evaluate if the flag should be set during runtime, which the translation above does.
This could benefit from Constant folding since in the 1st case, we can tell during compile time that -f has to be set.

Alternatively I could also rewrite the builtin to just expect a [Text] for the params, so it's called like rm("file.txt", ["-f", "-r"]). This of course leaves more room for unexpected behavior during runtime.

I think that we should accept bool just like we do it right now. If anyone wants to use some less common option parameter, they can just run a regular command.

yeah I think that a boolean is the best, until we don't have dictionaries is the right way

src/tests/validity/kill_using_pid.ab

Mte90 · 2026-01-07T10:37:02Z

I think that we need to update the stdlib to use where needed those new builtins.
Apart that we need a review from the others ;-)

Ph0enixKM

@kryske you did a really good job. I'm really impressed. There are some things that have to be addressed though. Nevertheless I like how cleanly you wrote the rust side of code. Good job!

Ph0enixKM · 2026-01-07T17:00:18Z

src/modules/builtin/ls.rs

+                options_frag.with_quotes(false),
+                path_fragment
+            ),
+            BlockFragment::new(vec![handler], true).to_frag(),


Consider unwrapping the handler from within BlockFragment unless it's here on purpose.

Suggested change

BlockFragment::new(vec![handler], true).to_frag(),

handler,

Ph0enixKM · 2026-01-07T17:02:26Z

src/modules/builtin/pid.rs

+        token(meta, "pid")?;
+        token(meta, "(")?;
+        token(meta, ")")?;


We sould unify this with status syntax. In this case I think that we should update status to also require parentheses:

// Before status // After status()

Ph0enixKM · 2026-01-07T17:03:36Z

src/modules/builtin/ls.rs

+                options_frag.with_quotes(false),
+                path_fragment
+            ),
+            BlockFragment::new(vec![handler], true).to_frag(),


Consider unwrapping handler from within BlockFragment unless this was made on purpose.

Suggested change

BlockFragment::new(vec![handler], true).to_frag(),

handler,

Ph0enixKM · 2026-01-07T17:04:16Z

src/modules/builtin/pid.rs

+    fn translate(&self, meta: &mut TranslateMetadata) -> FragmentKind {
+        let id = meta.gen_value_id();
+        let var_stmt =
+            VarStmtFragment::new("__int", Type::Int, fragments!("$!")).with_global_id(id);


Consider calling it __pid instead.

Suggested change

VarStmtFragment::new("__int", Type::Int, fragments!("$!")).with_global_id(id);

VarStmtFragment::new("__pid", Type::Int, fragments!("$!")).with_global_id(id);

Ph0enixKM · 2026-01-07T17:05:35Z

src/modules/builtin/ls.rs

+        };
+        let id = meta.gen_value_id();
+        let var_stmt =
+            VarStmtFragment::new("__array", Type::array_of(Type::Text), FragmentKind::Empty)


Consider calling this variable __ls instead.

Suggested change

VarStmtFragment::new("__array", Type::array_of(Type::Text), FragmentKind::Empty)

VarStmtFragment::new("__ls", Type::array_of(Type::Text), FragmentKind::Empty)

Ph0enixKM · 2026-01-07T17:27:48Z

src/modules/builtin/rm.rs

+        let force_expr = meta.push_ephemeral_variable(force_var_stmt);
+        meta.stmt_queue.extend([
+            fragments!(
+                raw_fragment!("read -rd '' -a {} < <([[ ", force_expr.get_name()),


I think that we should accept bool just like we do it right now. If anyone wants to use some less common option parameter, they can just run a regular command.

Ph0enixKM · 2026-01-07T17:34:00Z

src/modules/builtin/ls.rs

+    fn new() -> Self {
+        Ls {
+            value: Box::new(None),
+            options: Box::new(None),


Let's add only the most important and common flags. I propose to add support for -a (all) and -R (recursive) in the same format as rm does. This makes it easy to switch on/off certain flags with boolean variables. If someone wants to use some less common flag, they can always run a command.

I think -A would be better for the all flag in this case. I don't see the value in returning a "." and ".." entry.

You're absolutely right, @kryske. Let's do -A instead of -a.

Ph0enixKM · 2026-01-07T17:47:08Z

src/modules/builtin/rm.rs

+            let recursive_var_stmt = VarStmtFragment::new("__recursive", Type::Bool, FragmentKind::Empty).with_global_id(recursive_id);
+            let recursive_expr = meta.push_ephemeral_variable(recursive_var_stmt);
+            meta.stmt_queue.extend([
+                fragments!(raw_fragment!("read -rd '' -a {} < <([[ ", recursive_expr.get_name()),


This command reads array from recursive_expr which is of type Bool. Consider rendering the following logic gate:

[[ ${recursive_expr} -neq 0 ]] && __recursive="-r" || __recursive=""

Or even cleaner version which uses bash arithmetic to determine the result

(( recursive_expr )) && __recursive="-r" || __recursive=""

Consider doing the same for force_expr too in this function.

Ph0enixKM · 2026-01-07T17:48:53Z

src/modules/builtin/rmdir.rs

I don't see a point of introducing this builtin since we can already rm("directory", true). I think that we can scrap this module entirely (remember to update grammar.ebnf)

or maybe do an alias to that builtin internally?
rmdir is a command already available/

I don't like the current approach where one builtin can do other builtin's behavior. Let's simplify them to make them all self contained.

Idea 1

Let's either unify them, making the rm(val, true) run rmdir which is a safer option for shell scripting

Idea 2

Let's separate these two builtins so that only one can do its own thing:

rm / rmfile - removes only files with rf and fails to remove directories

rmdir - removes only directories

Idea 3

Let's just use rm for both files and directories. This could render to the following condition:

[[ -d $target ]] && rmdir "$target" || rm "$target"

Ph0enixKM · 2026-01-07T18:01:28Z

src/modules/builtin/wait.rs

+
+impl TranslateModule for Wait {
+    fn translate(&self, meta: &mut TranslateMetadata) -> FragmentKind {
+        fragments!("wait ", self.pids.translate(meta))


@Mte90 I'd rename this builtin to be await and let it accept single parameter or a list of parameter:

/// WARNING: This is future Amber code let pid = async foo() // Waits until foo finishes await(pid) let pids = [async foo(), async bar()] // Runs in parallel foo and bar and waits for them to finish await(pids) // Runs in parallel foo and bar and waits for them to finish await([async foo(), async bar()])

* switch recursive and force param * made force param optional and default to false * simplified translation * updated tests

* refactored naming in pid builtin

* added parentheses to status to unify syntax * added deprecation warning for use with parentheses * updated use of status throughout the project

* simplified pwd builtin translation * updated prefix of var

* removed rmdir builtin * removed grammer for rmdir builtin

* renamed builtin wait to await * updated grammar

* updated params of builtin ls to only provider optional bool params to provide the -A and -R flag * updated tests

* fixed test because ls -R returns something different on mac * some code cleanup

Mte90 · 2026-01-08T08:49:30Z

src/tests/validity/await.ab

+
+main {
+    trust $ SECONDS=0 $
+    trust $ sleep 2 & $


we should use the new builtin here

kryske added 9 commits January 4, 2026 19:55

feat(builtin): add new builtins

f257e27

* added builtins: clear, ls, pwd, rm, rmdir and sleep

test(builtin): added tests for some builtins

41dcbeb

* added tests for builtins ls, rm, rmdir and pwd

feat(builtin): added builtin for wait

e9bcc63

* added builtin function for wait command

feat(builtin): added new cp builtin

f0267f6

* added new builtin for cp * added test for builtin cp

feat(builtin): added pid builtin

2d58dc3

* added builtin to get pid of last command * updated kill test to use pid builtin * don't use reserved keyword pid in stdlib

feat(builtin): added grammar for builtins

cb1782b

* added grammar for builtins

fix(stdlib): lines is reserved keyword

725e7fb

* removed const with name lines from pgrep and pgrep_exact as it is a reserved keyword

test(builtin): added test for sleep

9d3aaeb

* added test for sleep builtin

test(builtin): fixed kill_using_pid test

edee41f

* silenced Terminate message, so it doesn't interfere with the expected output

kryske added 2 commits January 4, 2026 22:22

test(builtin): added test for wait builtin

1e23b82

* added test for wait builtin

feat(builtin): updated cp builtin

0ffaaae

* removed deprecated calling method for new builtin cp

kryske marked this pull request as draft January 4, 2026 21:44

Mte90 requested review from Mte90, Ph0enixKM and lens0021 and removed request for Mte90 January 5, 2026 12:55

Mte90 requested changes Jan 5, 2026

View reviewed changes

Mte90 linked an issue Jan 5, 2026 that may be closed by this pull request

[Feature] New builtins proposals #908

Open

Mte90 mentioned this pull request Jan 5, 2026

[Feature] New builtins proposals #908

Open

kryske added 5 commits January 5, 2026 19:28

test(builtin): updated tests for builtins

8d5d3e3

* fixed typo * use touch builtin when possible * make tests that use SECONDS less volatile

feat(builtin): added flags to rm builtin

53fdfe7

* added force and recursive flag to rm builtin * added test for rm recursive

feat(builtin): updated rmdir builtin

ad4d194

* use rmdir when translating * added argument to ignore errors for non empty dirs * updated testcase

feat(builtin): updated rmdir builtin

36ffdd7

* removed argument for ignore errors for non empty dirs since it doesn't exist on MacOS

feat(builtin): added options to ls

e27c016

* added optional options to ls builtin

Mte90 reviewed Jan 7, 2026

View reviewed changes

src/tests/validity/kill_using_pid.ab Show resolved Hide resolved

Ph0enixKM requested changes Jan 7, 2026

View reviewed changes

kryske added 8 commits January 7, 2026 22:01

feat(builtin): updated rm builtin params

54666d2

* switch recursive and force param * made force param optional and default to false * simplified translation * updated tests

refactor(builtin): refactor naming in pid

297a50a

* refactored naming in pid builtin

feat(expression): unify status syntax

140826c

* added parentheses to status to unify syntax * added deprecation warning for use with parentheses * updated use of status throughout the project

perf(builtin): simplified pwd translation

8afb090

* simplified pwd builtin translation * updated prefix of var

revert(builtin): remove obsolete rmdir builtin

d2479aa

* removed rmdir builtin * removed grammer for rmdir builtin

refactor(builtin): renamed wait to await

4619d10

* renamed builtin wait to await * updated grammar

feat(builtin): updated ls builtin params

8a368a9

* updated params of builtin ls to only provider optional bool params to provide the -A and -R flag * updated tests

test(builtin): fixed test

50b5a6f

* fixed test because ls -R returns something different on mac * some code cleanup

Mte90 reviewed Jan 8, 2026

View reviewed changes

	VarStmtFragment::new("__int", Type::Int, fragments!("$!")).with_global_id(id);
	VarStmtFragment::new("__pid", Type::Int, fragments!("$!")).with_global_id(id);

	VarStmtFragment::new("__array", Type::array_of(Type::Text), FragmentKind::Empty)
	VarStmtFragment::new("__ls", Type::array_of(Type::Text), FragmentKind::Empty)

Feat/new builtins #996

Are you sure you want to change the base?

Feat/new builtins #996

Uh oh!

Conversation

kryske commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mte90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mte90 commented Jan 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mte90 commented Jan 7, 2026

Uh oh!

Ph0enixKM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ph0enixKM Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Idea 1

Idea 2

Idea 3

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kryske commented Jan 4, 2026 •

edited

Loading

codecov bot commented Jan 4, 2026 •

edited

Loading

Ph0enixKM Jan 8, 2026 •

edited

Loading