Shellcheck wrap variables #376

Mte90 · 2024-07-30T14:06:40Z

Taking as example the date_compare.ab test we have this issues:

In /tmp/amber-sc-tests/date_compare.ab.sh line 9:
    if [ $(
         ^-- SC2046 (warning): Quote this to prevent word splitting.


In /tmp/amber-sc-tests/date_compare.ab.sh line 11:
        echo $?
             ^-- SC2319 (warning): This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten.


In /tmp/amber-sc-tests/date_compare.ab.sh line 15:
    if [ $(
         ^-- SC2046 (warning): Quote this to prevent word splitting.


In /tmp/amber-sc-tests/date_compare.ab.sh line 17:
        echo $?
             ^-- SC2319 (warning): This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten.


In /tmp/amber-sc-tests/date_compare.ab.sh line 23:
    if [ ${utc} != 0 ]; then
         ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    if [ "${utc}" != 0 ]; then


In /tmp/amber-sc-tests/date_compare.ab.sh line 49:
    if [ $(
         ^-- SC2046 (warning): Quote this to prevent word splitting.


In /tmp/amber-sc-tests/date_compare.ab.sh line 53:
        date_posix__0_v0 "" "" ${utc}
                               ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
        date_posix__0_v0 "" "" "${utc}"


In /tmp/amber-sc-tests/date_compare.ab.sh line 58:
    date_posix__0_v0 "%s" "${date_a}" ${utc}
                                      ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    date_posix__0_v0 "%s" "${date_a}" "${utc}"


In /tmp/amber-sc-tests/date_compare.ab.sh line 66:
    date_posix__0_v0 "%s" "${date_b}" ${utc}
                                      ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    date_posix__0_v0 "%s" "${date_b}" "${utc}"


In /tmp/amber-sc-tests/date_compare.ab.sh line 74:
    if [ $(echo ${timestamp_a} '>' ${timestamp_b} | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then
         ^-- SC2046 (warning): Quote this to prevent word splitting.
                ^------------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                   ^------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    if [ $(echo "${timestamp_a}" '>' "${timestamp_b}" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then


In /tmp/amber-sc-tests/date_compare.ab.sh line 78:
    if [ $(echo ${timestamp_a} '==' ${timestamp_b} | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then
         ^-- SC2046 (warning): Quote this to prevent word splitting.
                ^------------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                    ^------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    if [ $(echo "${timestamp_a}" '==' "${timestamp_b}" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then


In /tmp/amber-sc-tests/date_compare.ab.sh line 82:
    if [ $(echo ${timestamp_a} '<' ${timestamp_b} | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then
         ^-- SC2046 (warning): Quote this to prevent word splitting.
                ^------------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                   ^------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    if [ $(echo "${timestamp_a}" '<' "${timestamp_b}" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then


In /tmp/amber-sc-tests/date_compare.ab.sh line 94:
if [ $(echo $(echo "$__AF_date_compare3_v0__4_16" '==' $(echo '-' 1 | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') '&&' $(echo "$__AF_date_compare3_v0__4_60" '==' 1 | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//') != 0 ]; then
     ^-- SC2046 (warning): Quote this to prevent word splitting.
            ^-- SC2046 (warning): Quote this to prevent word splitting.
                                                       ^-- SC2046 (warning): Quote this to prevent word splitting.
                                                                                                                                                                 ^-- SC2046 (warning): Quote this to prevent word splitting.

For more information:
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2319 -- This $? refers to a condition, no...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

We fix some of those issues (also in other tests) but I am still working on some of the warning here.

Mte90 · 2024-07-30T14:32:04Z

Now is just:

In ./date_compare.ab.sh line 11:
        echo $?
             ^-- SC2319 (warning): This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten.


In ./date_compare.ab.sh line 17:
        echo $?
             ^-- SC2319 (warning): This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten.


In ./date_compare.ab.sh line 94:
if [ "$(echo "$(echo ""$__AF_date_compare3_v0__4_16"" '==' "$(echo "" '-' "1" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" '&&' "$(echo ""$__AF_date_compare3_v0__4_60"" '==' "1" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" != 0 ]; then
                       ^--------------------------^ SC2027 (warning): The surrounding quotes actually unquote this. Remove or escape them.
                       ^--------------------------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                                                                                                                                                       ^--------------------------^ SC2027 (warning): The surrounding quotes actually unquote this. Remove or escape them.
                                                                                                                                                                                       ^--------------------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
if [ "$(echo "$(echo """$__AF_date_compare3_v0__4_16""" '==' "$(echo "" '-' "1" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" '&&' "$(echo """$__AF_date_compare3_v0__4_60""" '==' "1" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//')" != 0 ]; then

For more information:
  https://www.shellcheck.net/wiki/SC2027 -- The surrounding quotes actually u...
  https://www.shellcheck.net/wiki/SC2319 -- This $? refers to a condition, no...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

We are working for the 2 issues in 2 different tickets, so it will be their turn soon.

Mte90 · 2024-07-30T14:52:01Z

src/modules/condition/ifcond.rs

@@ -91,7 +91,11 @@ impl SyntaxModule<ParserMetadata> for IfCondition {
 impl TranslateModule for IfCondition {
    fn translate(&self, meta: &mut TranslateMetadata) -> String {
        let mut result = vec![];
-        result.push(format!("if [ {} != 0 ]; then", self.expr.translate(meta)));
+        if self.expr.translate(meta).starts_with("\"") {


I am not sure if this is something that we want to handle here.
Probably to fix the various cases in this PR is enough that the translate method automatically wrap a variable so this check is not needed.

@Ph0enixKM I am right?

I added that check because it was generating if [ ""$__AF_file_exist1_v0__28_8"" != 0 ]; thenif [ ""$__AF_file_exist1_v0__28_8"" != 0 ]; then stuff like that with a double "

i think its best to remove this check. if some part of code translates a thing unquoted, it should be fixed. if we add checks like this all over this place, it will be a giant mess

Yeah that's my point but we can generate a bash like this so a check somewhere to not read them is needed

so just do it like this without a check?

result.push(format!("if [ {} != 0 ]; then", self.expr.translate(meta)));

assuming that it should be translated quoted. if it's not translated quoted, then its an issue with the translator

I think that #283 this PR implemented in some cases to wrap the variable but not everywhere so I am fixing in this PR in the wrong place.

Don't do that check. Please find the root cause of what adds the quotes and fix it instead. If you need any help, let me know

I wasn't able to find where it is happening, looking at the rest of changes I did in this PR there is somewhere where we escape but not like in all the cases.

Ph0enixKM · 2024-08-06T09:39:26Z

src/modules/condition/ifchain.rs

@@ -82,11 +82,11 @@ impl TranslateModule for IfChain {
        let mut is_first = true;
        for (cond, block) in self.cond_blocks.iter() {
            if is_first {
-                result.push(format!("if [ {} != 0 ]; then", cond.translate(meta)));
+                result.push(format!("if [ \"{}\" != 0 ]; then", cond.translate(meta)));


This shouldn't look like this too. Unless there is a big reason why all the conditions should be wrapped in quotes

Ph0enixKM · 2024-08-06T09:39:32Z

src/modules/condition/ifchain.rs

                result.push(block.translate(meta));
                is_first = false;
            } else {
-                result.push(format!("elif [ {} != 0 ]; then", cond.translate(meta)));
+                result.push(format!("elif [ \"{}\" != 0 ]; then", cond.translate(meta)));


This one too

Ph0enixKM · 2024-08-06T09:39:46Z

src/modules/condition/ternary.rs

@@ -54,7 +54,7 @@ impl TranslateModule for Ternary {
        let cond = self.cond.translate(meta);
        let true_expr = self.true_expr.translate(meta);
        let false_expr = self.false_expr.translate(meta);
-        meta.gen_subprocess(&format!("if [ {} != 0 ]; then echo {}; else echo {}; fi", cond, true_expr, false_expr))
+        meta.gen_subprocess(&format!("if [ \"{}\" != 0 ]; then echo {}; else echo {}; fi", cond, true_expr, false_expr))


And here...

Ph0enixKM · 2024-08-06T09:41:24Z

src/translate/compute.rs

@@ -50,7 +50,7 @@ pub fn translate_computation(meta: &TranslateMetadata, operation: ArithOp, left:
                ArithOp::Or => "||"
            };
            let math_lib_flag = if math_lib_flag { "-l" } else { "" };
-            meta.gen_subprocess(&format!("echo {left} '{op}' {right} | bc {math_lib_flag} | sed '{sed_regex}'"))
+            meta.gen_subprocess(&format!("echo \"{left}\" '{op}' \"{right}\" | bc {math_lib_flag} | sed '{sed_regex}'"))


I think that we just need to check for the inner variables if they are wrapped in string quotes

yeah I think that can resolve a lot of issues if we do at the top of the flow and not when it is time to print them

Ph0enixKM · 2024-08-06T09:58:53Z

@Mte90 to fix the shell check warnings I propose you to go get the minimal Amber code needed, generate bash from it and then try to understand the root cause of this warning. Perhaps the function calls are not wrapped in double quotes or maybe just the variables?

Mte90 added 2 commits July 30, 2024 16:03

feat(shellcheck): other issue

0f448ad

feat(invocation): wrap vvariable

6e9b66b

Mte90 marked this pull request as ready for review July 30, 2024 14:29

feat(invocation): wrap vvariable

347eae0

Mte90 requested review from Ph0enixKM and KrosFire July 30, 2024 14:32

b1ek approved these changes Jul 30, 2024

View reviewed changes

Mte90 marked this pull request as draft July 30, 2024 14:43

feat(invocation): wrap variable if needed

0a357d4

Mte90 commented Jul 30, 2024

View reviewed changes

Mte90 changed the title ~~feat(shellcheck): other issue~~ Shellcheck wrap variables Jul 30, 2024

Mte90 mentioned this pull request Jul 30, 2024

Remove sed for condition #381

Open

Ph0enixKM reviewed Aug 6, 2024

View reviewed changes

Mte90 closed this Sep 5, 2024

Mte90 deleted the sc-2086-2046 branch September 5, 2024 13:03

Mte90 mentioned this pull request Sep 23, 2024

Implement builtin glob function #488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shellcheck wrap variables #376

Shellcheck wrap variables #376

Mte90 commented Jul 30, 2024

Mte90 commented Jul 30, 2024

Mte90 Jul 30, 2024

Mte90 Jul 30, 2024

b1ek Jul 30, 2024

Mte90 Jul 30, 2024

b1ek Jul 30, 2024

Mte90 Jul 30, 2024

Ph0enixKM Aug 6, 2024

Mte90 Aug 6, 2024

Ph0enixKM Aug 6, 2024

Ph0enixKM Aug 6, 2024

Ph0enixKM Aug 6, 2024

Ph0enixKM Aug 6, 2024

Mte90 Aug 6, 2024

Ph0enixKM commented Aug 6, 2024

Shellcheck wrap variables #376

Shellcheck wrap variables #376

Conversation

Mte90 commented Jul 30, 2024

Mte90 commented Jul 30, 2024

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

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

Ph0enixKM commented Aug 6, 2024