Implement builtin glob function #488

hdwalters · 2024-09-22T14:51:27Z

Fixes #463, by creating a builtin glob function.

hdwalters · 2024-09-22T15:00:58Z

build.rs

@@ -1,3 +1,4 @@
 fn main() {
- build_helper::rerun_if_changed("src/tests/**");
+ // See [https://doc.rust-lang.org/cargo/reference/build-scripts.html].
+ build_helper::rerun_if_changed("src/tests");


The "rerun-if-changed" directive takes a directory name, not a "**" extended glob; this was rebuilding Amber even if nothing had changed in "src/tests".

I didn't know this was commented. Good job!

hdwalters · 2024-09-22T15:02:37Z

src/main.rs

@@ -175,15 +175,13 @@ fn handle_docs(cli: Cli) -> Result<(), Box<dyn Error>> {
 }
 }

-/*
-#[cfg(target_os = "windows")]
+#[cfg(windows)]


If we have a conditional build for Windows (even if not officially supported), surely it shouldn't be commented out?

src/modules/expression/expr.rs

hdwalters · 2024-09-22T15:06:22Z

src/modules/expression/literal/text.rs

- format!("{quote}{}{quote}", translate_interpolated_region(self.strings.clone(), interps, true))
+ let strings = translate_interpolated_region(self.strings.clone(), interps, true);
+ if self.unquoted {
+ strings


This is necessary so we generate Bash for item in file* not for item in "file*".

hdwalters · 2024-09-22T15:10:58Z

src/modules/function/glob.rs

+ token(meta, "glob")?;
+ token(meta, "(")?;
+ let mut new_is_unquoted_ctx = true;
+ swap(&mut new_is_unquoted_ctx, &mut meta.context.is_unquoted_ctx);


This pattern is dangerous, because the context value is not reverted to its previous value if the function does an early return, as it will if syntax(meta, &mut arg)? fails below. However, the source code is full of instances of this, so we need a comprehensive fix. I will create a separate issue for this.

hdwalters · 2024-09-22T15:20:32Z

src/modules/function/invocation.rs

@@ -58,8 +58,7 @@ impl SyntaxModule<ParserMetadata> for FunctionInvocation {
 // Get the function name
 let tok = meta.get_current_token();
 if let Some(ref tok) = tok {
- self.line = tok.pos.0;
- self.col = tok.pos.1;
+ (self.line, self.col) = tok.pos;


It's generally more readable to use tuple destructuring than explicit indices.

hdwalters · 2024-09-22T15:28:48Z

src/tests/validity/glob_absolute_missing_file.ab

+import * from "std/text"
+
+// Output
+// ===


These separators are necessary because if the script creates no output (as in this case) the unit test actually behaves as if we had written:

// Output // Succeded

hdwalters · 2024-09-22T19:10:05Z

src/modules/expression/literal/text.rs

- format!("{quote}{}{quote}", translate_interpolated_region(self.strings.clone(), interps, true))
+ let strings = translate_interpolated_region(self.strings.clone(), interps, true);
+ if self.unquoted {
+ strings


This still allows for injection attacks, but I have pushed another commit to prevent this, by escaping spaces and semicolons:

if self.escaped { strings.replace(" ", "\\ ").replace(";", "\\;") } else { let quote = meta.gen_quote(); format!("{quote}{strings}{quote}") }

hdwalters · 2024-09-22T21:06:23Z

src/modules/loops/iter_loop.rs

+ [
+ format!("for {name} in {expr}"),
+ "do".to_string(),
+ format!("if {conditional}"),


Please note, this will only work with string literals:

#!/usr/bin/env amber echo "[works]" loop index, item in glob("*.md") { echo "{index}: {item}" } echo "[fails]" let wildcard = "*.md" loop index, item in glob(wildcard) { echo "{index}: {item}" }

I do not see any way around this (short of calling find, which has its own problems) because Bash glob expansion does not work with quoted globs.

hdwalters · 2024-09-22T21:50:54Z

src/tests/stdlib/change_owner.ab

@@ -13,5 +12,5 @@ main {
 } else {
 echo "File {tmpdir}/amber-symbolic not exists"
 }
- unsafe $rm {tmpdir}/amber-symbolic$
+ unsafe $rm -fr {tmpdir}$


There were several places in the standard library unit tests where we failed to clean up after ourselves, so /tmp/ filled up with temporary directories. Fixed this while I was here.

hdwalters · 2024-09-22T21:59:46Z

src/tests/validity/glob_absolute_with_spaces.ab

+ }
+
+ echo "==="
+ loop file in glob("{tmpdir}/*with spaces*") {


The glob builtin does not expect the calling code to escape spaces in its own string literals; it has to do this itself, to prevent injection attacks.

Mte90 · 2024-09-23T09:13:25Z

src/utils/context.rs

@@ -114,6 +114,8 @@ pub struct Context {
 pub is_main_ctx: bool,
 /// Determines if the context is in an unsafe block
 pub is_unsafe_ctx: bool,
+ /// Determines if text should be escaped e.g. for file globs
+ pub is_escaped_ctx: bool,


this is interesting, I think that can be used for:

Shellcheck wrap variables #376

Remove sed for condition #381

[Bug] Escape rightly bash function #270

src/tests/validity/glob_relative_multiple_globs.ab

Mte90 · 2024-09-23T09:19:01Z

A Bash generated:

for file in missing* file* *.csv; do
    if [ -e "${file}" ]; then
        echo "${file}"
    fi
done

I like this approach using the bash for.

hdwalters · 2024-09-23T17:10:17Z

A Bash generated:
for file in missing* file* *.csv; do
    if [ -e "${file}" ]; then
        echo "${file}"
    fi
done
I like this approach using the bash for.

Please bear in mind that the glob builtin as currently written can only support string literals:

for file in glob("*.md") {
    echo file
}

Not:

let files = "*.md"
for file in glob(files) {
    echo file
}

Or:

let files = [ "*.md" ]
for file in glob(files) {
    echo file
}

Ph0enixKM

From the syntax perspective, since glob is a built-in and echo is a built-in and glob requires parentheses but echo does not - does it mean we introduce a new builtin type?

I'd keep the new built-in just like the others: glob "value"

b1ek

why is there so many changes, unrelated to this PR? mainly the refactoring ones.

can you cherry pick the commits with the refactoring changes to a separate branch and make another PR for it? and leave only the relevant ones in this one

hdwalters · 2024-09-30T21:35:23Z

why is there so many changes, unrelated to this PR? mainly the refactoring ones.

can you cherry pick the commits with the refactoring changes to a separate branch and make another PR for it? and leave only the relevant ones in this one

Yeah, fair enough. I tend to make changes as I go (though I do try to limit them to the vicinity of the functional changes). However, I would like to get to the bottom of the "are we ok with builtin functions that behave like standard library functions" question, before I do any more work in this branch.

hdwalters · 2024-09-30T22:23:36Z

I've created a new pull request #495 for this purpose, including all the refactoring from this PR, and some other general tidying I've spotted while working on other branches. If we can get that merged, I'll rebase this one, and hopefully it will become a lot easier to review.

hdwalters · 2024-10-01T22:06:14Z

Yes, I see there are merge conflicts (which I expected) following the merge of #495. I will rebase this branch when we've resolved the questions raised in #277.

hdwalters · 2024-10-02T21:33:25Z

Now that I've rebased the branch onto the latest "master", can I get another review please?

hdwalters requested review from Mte90, Ph0enixKM, b1ek and KrosFire September 22, 2024 14:51

hdwalters commented Sep 22, 2024

View reviewed changes

Mte90 reviewed Sep 23, 2024

View reviewed changes

src/tests/validity/glob_relative_multiple_globs.ab Outdated Show resolved Hide resolved

hdwalters requested a review from Mte90 September 23, 2024 17:12

Ph0enixKM requested changes Sep 23, 2024

View reviewed changes

b1ek reviewed Sep 30, 2024

View reviewed changes

hdwalters added 3 commits October 2, 2024 22:16

Create syntax handler for glob loop.

e1d9cb7

Generate Bash code for glob loop.

09f7236

Check if file exists in glob loop.

d23ca60

hdwalters force-pushed the implement-builtin-glob-function branch from f8e8de8 to d23ca60 Compare October 2, 2024 21:26

hdwalters changed the title ~~Implement builtin glob function~~ Implement builtin glob expression Oct 2, 2024

hdwalters changed the title ~~Implement builtin glob expression~~ Implement builtin glob function Oct 2, 2024

hdwalters requested review from Ph0enixKM and b1ek October 2, 2024 21:36

Mte90 approved these changes Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement builtin glob function #488

Implement builtin glob function #488

hdwalters commented Sep 22, 2024 •

edited

Loading

hdwalters Sep 22, 2024

Ph0enixKM Sep 23, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024 •

edited

Loading

hdwalters Sep 22, 2024

hdwalters Sep 22, 2024

Mte90 Sep 23, 2024

hdwalters Sep 22, 2024

Mte90 Sep 23, 2024

Mte90 commented Sep 23, 2024

hdwalters commented Sep 23, 2024

Ph0enixKM left a comment

b1ek left a comment

hdwalters commented Sep 30, 2024

hdwalters commented Sep 30, 2024 •

edited

Loading

hdwalters commented Oct 1, 2024

hdwalters commented Oct 2, 2024

Implement builtin glob function #488

Are you sure you want to change the base?

Implement builtin glob function #488

Conversation

hdwalters commented Sep 22, 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

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 Sep 22, 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

Choose a reason for hiding this comment

Mte90 commented Sep 23, 2024

hdwalters commented Sep 23, 2024

Ph0enixKM left a comment

Choose a reason for hiding this comment

b1ek left a comment

Choose a reason for hiding this comment

hdwalters commented Sep 30, 2024

hdwalters commented Sep 30, 2024 • edited Loading

hdwalters commented Oct 1, 2024

hdwalters commented Oct 2, 2024

hdwalters commented Sep 22, 2024 •

edited

Loading

hdwalters Sep 22, 2024 •

edited

Loading

hdwalters commented Sep 30, 2024 •

edited

Loading