Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape backslashes in search and replacement #592

Draft
wants to merge 7 commits into
base: staging
Choose a base branch
from

Conversation

lens0021
Copy link
Contributor

Fixes #589.

@Ph0enixKM
Copy link
Member

Wow! Tested and this works. Thanks! 🙌

Ph0enixKM
Ph0enixKM previously approved these changes Nov 14, 2024
@Mte90
Copy link
Member

Mte90 commented Nov 15, 2024

@lens0021 the tests doesn't work. Can you check what is happening or you need help?

Copy link
Member

@Mte90 Mte90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests does't work

@lens0021

This comment was marked as resolved.

src/std/text.ab Show resolved Hide resolved
src/std/text.ab Show resolved Hide resolved
@Mte90
Copy link
Member

Mte90 commented Nov 15, 2024

So I think that the bash generated is not very ok.

#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.3.5-alpha
# date: 2024-11-15 17:31:52
replace__0_v0() {
    local source=$1
    local search=$2
    local replace=$3
    __AMBER_VAL_0=$(echo ${search//\\/\\\\})
    __AS=$?
    search="${__AMBER_VAL_0}"
    __AMBER_VAL_1=$(echo ${replace//\\/\\\\})
    __AS=$?
    replace="${__AMBER_VAL_1}"
    __AMBER_VAL_2=$(echo ${source//${search}/${replace}})
    __AS=$?
    __AF_replace0_v0="${__AMBER_VAL_2}"
    return 0
}
# Output
# apple apple
# apple

replace__0_v0 "banana banana" "banana" "apple"
__AF_replace0_v0__8_10="${__AF_replace0_v0}"
echo "${__AF_replace0_v0__8_10}"
replace__0_v0 "\\" "\\" "apple"
__AF_replace0_v0__9_10="${__AF_replace0_v0}"
echo "${__AF_replace0_v0__9_10}"

This is an example of the bash generated for the replace test, https://www.shellcheck.net/ with that code report the various lines.

This amber code: search = trust $ \$\{search//\\\\/\\\\\\\\} $ generates __AMBER_VAL_0=$(${search//\\/\\\\}) but it should be __AMBER_VAL_0=${search//\\/\\\\}.
Do you have suggestions @Ph0enixKM?

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Nov 16, 2024

@lens0021 @Mte90 It seems that only the replace_once fails. It fails also on my local machine. When I modify the contents of said function to:

/// Replaces the first occurrence of a pattern in the content with the provided replace text.
pub fun replace_once(source, search, replace) {
    search = trust $ echo \$\{{nameof search}//\\\\/\\\\\\\\} $
    replace = trust $ echo \$\{{nameof replace}//\\\\/\\\\\\\\} $
    return trust $ echo \$\{{nameof source}/{search}/{replace}} $
}

echo replace_once("Hello, World\\", "World\\", "Rust")

It suddenly works.

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Nov 16, 2024

But still these two tests fail:

    tests::stdlib::test_stdlib_src_tests_stdlib_replace_regex_basic_ab
    tests::stdlib::test_stdlib_src_tests_stdlib_replace_regex_ext_ab

I can't investigate this problem further now. Maybe later today or tomorrow I can play around it

@Mte90
Copy link
Member

Mte90 commented Nov 24, 2024

the tests are still failing

@lens0021
Copy link
Contributor Author

There are some changes on bash 5.2

$ docker run --rm -v $PWD:/tmp bash:5.1.16 bash /tmp/replace_once.sh
Succinctly\
$ docker run --rm -v $PWD:/tmp bash:5.2.0 bash /tmp/replace_once.sh
Succeeded

I will try to write a compatible code...

@lens0021 lens0021 marked this pull request as ready for review December 12, 2024 16:26
src/std/text.ab Outdated
Comment on lines 5 to 12
// TODO: Revisit after https://github.com/amber-lang/amber/issues/632
const bash_major = trust $ echo \"\$\{BASH_VERSINFO[0]}\" $ as Num
const bash_minor = trust $ echo \"\$\{BASH_VERSINFO[1]}\" $ as Num
if bash_major > 5 or (bash_major == 5 and bash_minor > 1) {
replace = trust $ echo \$\{{nameof replace}//\\\\/\\\\\\\\} $
} else {
replace = trust $ echo \$\{{nameof replace}//\\\\/\\\\} $
}
Copy link
Contributor Author

@lens0021 lens0021 Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is dirty. When #622 is resolved, this could be improved.

@lens0021
Copy link
Contributor Author

Now the both versions are supported.

$ cargo run -- build src/tests/stdlib/replace_once.ab src/tests/stdlib/replace_once.sh
   Compiling amber v0.3.5-alpha (/usr/local/git/amber)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.52s
     Running `target/debug/amber build src/tests/stdlib/replace_once.ab src/tests/stdlib/rep
lace_once.sh`

$ docker run --rm -v $PWD:/tmp bash:5.1.16 bash /tmp/src/tests/stdlib/replace_once.sh
Succeeded

$ docker run --rm -v $PWD:/tmp bash:5.2.0 bash /tmp/src/tests/stdlib/replace_once.sh
Succeeded

@lens0021 lens0021 requested a review from Mte90 December 16, 2024 16:59
@Mte90 Mte90 requested a review from hdwalters December 16, 2024 17:50
src/std/text.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved

main {
echo replace("banana banana", "banana", "apple")
echo replace("\\", "\\", "apple")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm that replace replaces multiple instances, may I suggest testing multiple occurrences of the search text:

echo replace("\\ \\", "\\", "apple")
echo replace("\\banana\\", "\\", "\\\\")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also consider cloning the modified test, and overwriting replace_once.ab, with adjusted expected output.

Copy link
Contributor Author

@lens0021 lens0021 Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for replace_once and edited text.ab to make the test pass, though I did not know why it was broken.

Copy link
Contributor

@hdwalters hdwalters Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not entirely happy with these tests:

  1. I had in mind that both test files would contain calls to replace and replace_once, with identical parameters, to show the differences in behaviour.
  2. Each function call should test a specific use case; they don't all have to contain backslashes!
  3. Each function call should contain multiple copies of the search text.

Copy link
Contributor

@hdwalters hdwalters Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

import * from "std/text"

// Output
// TWO TWO TWO
// a\\b\\c\\d
// first..second..third..fourth

main {
    echo replace("one one one", "one", "TWO")
    echo replace("a\\b\\c\\d", "\\", "\\\\")
    echo replace("first\nsecond\nthird\nfourth", "\n", "..")
}

@lens0021
Copy link
Contributor Author

  • replace(text, "\n", replace) seems to be broken.

pub fun replace(source, search, replace) {
return "\$\{source//{search}/{replace}}"
search = trust $ echo \$\{{nameof search}//\\\\/\\\\\\\\} $
// TODO: Revisit after https://github.com/amber-lang/amber/issues/622
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this comment would be better placed above the bash_version function definition, as it's defined once but called multiple times?

Copy link
Contributor

@hdwalters hdwalters Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, now you've moved the BASH_VERSINFO test to a helper function, you no longer need the allow_absurd_cast annotation here.


main {
echo replace("banana banana", "banana", "apple")
echo replace("\\", "\\", "apple")
Copy link
Contributor

@hdwalters hdwalters Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not entirely happy with these tests:

  1. I had in mind that both test files would contain calls to replace and replace_once, with identical parameters, to show the differences in behaviour.
  2. Each function call should test a specific use case; they don't all have to contain backslashes!
  3. Each function call should contain multiple copies of the search text.


main {
echo replace("banana banana", "banana", "apple")
echo replace("\\", "\\", "apple")
Copy link
Contributor

@hdwalters hdwalters Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

import * from "std/text"

// Output
// TWO TWO TWO
// a\\b\\c\\d
// first..second..third..fourth

main {
    echo replace("one one one", "one", "TWO")
    echo replace("a\\b\\c\\d", "\\", "\\\\")
    echo replace("first\nsecond\nthird\nfourth", "\n", "..")
}

main {
echo replace_once("Succinctly", "inctly", "eeded")
echo replace_once("Succinctly\\", "inctly\\", "eeded")
Copy link
Contributor

@hdwalters hdwalters Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

import * from "std/text"

// Output
// TWO one one
// a\\b\c\d
// first..second
// third
// fourth

main {
    echo replace_once("one one one", "one", "TWO")
    echo replace_once("a\\b\\c\\d", "\\", "\\\\")
    echo replace_once("first\nsecond\nthird\nfourth", "\n", "..")
}

@lens0021 lens0021 marked this pull request as draft December 18, 2024 22:56
@Ph0enixKM Ph0enixKM changed the base branch from master to staging December 19, 2024 11:13
@Ph0enixKM Ph0enixKM dismissed their stale review December 19, 2024 11:13

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Standard library replace function fails to replace backslash characters
4 participants