Amber icon indicating copy to clipboard operation
Amber copied to clipboard

[Bug] Variable inside a single apix string

Open Mte90 opened this issue 1 year ago • 4 comments

Taking this example:

position = 1
$jq -r '.[0].assets.[{position}].browser_download_url'$

Generate this bash script:

jq -r '.[0].assets.[{position}].browser_download_url'

But inside a single apix string the variable doesn't get substituted, so Amber should report an error or compile it with double apix.

Mte90 avatar Jun 12 '24 14:06 Mte90

Looking at the issues with Shellcheck we have issues with variables that aren't wrapped with " are reported. So I think that can be handy to:

  • Remap all the strings with a single apix to a double one if there is a variable inside
  • All the variables printed should be double apix wrapped

Mte90 avatar Jun 12 '24 15:06 Mte90

It seems a bit wrong since this would modify bash commands under the hood. The Bash that is written by user should verbatim be the same. Instead - this could be solved with the ORM you mentioned in another issue.

Ph0enixKM avatar Jun 13 '24 08:06 Ph0enixKM

It is something that is language design, like the user can do whatever you want but if you use the std exposed by Amber automatically write Bash that is valid for shellcheck.

Mte90 avatar Jun 13 '24 12:06 Mte90

@Mte90 I've read this issue one more time and I take back what I said previously. This is a good issue. We should make the variables to be interpolated correctly even if they are in the verbatim string.

Ph0enixKM avatar Jun 19 '24 08:06 Ph0enixKM

For what it's worth, here is a test.ab file I wrote.

// Output
// Succeeded
// Succeeded
// Succeeded
const txt = "Succeeded"

main {
    trust $ echo {txt} $
    trust $ echo "{txt}" $
    trust $ echo '{txt}' $
}

Run result:

Succeeded
Succeeded
${__0_txt}

Built result:

__0_txt="Succeeded"
echo ${__0_txt}
echo "${__0_txt}"
echo '${__0_txt}'

lens0021 avatar Jun 21 '25 15:06 lens0021

I am thinking of this. I suggest that translating txt in $ echo '{txt}' $ to '${__0_txt}'. The first ' is for closing the outer, left quotation, and the second ' is for reopening the outer, right quotation. So the whole command should be echo ''${__0_txt}''. More examples here:

  • $ echo {txt} $ → echo ${__0_txt}
  • $ echo "{txt}" $ → echo "${__0_txt}"
  • $ echo '{txt}' $ → echo ''${__0_txt}'', because the var_expr is in a single quoted text.
  • $ echo "'{txt}'" $ → echo "'${__0_txt}'"
  • $ echo '"{txt}"' $ → echo '"'${__0_txt}'"', same here
  • $ echo '"'{txt}'"' $ → echo '"'${__0_txt}'"' no, expr_var is not quoted.
  • $ echo "\"{txt}"\" $ → echo "\"${__0_txt}"\"

In this way, we can minimize the modification of the original bash code the user wrote. However, to do this, we should make the render_bash_value() function of var_expr know whether it is in a single quoted text. I don't know the compiler parses bash code. Does it, and could it be extended for this?

lens0021 avatar Jun 23 '25 15:06 lens0021

My above suggestion has a bug.

const txt = "Lorem ipsum" // contains a space 
trust $ for v in '{txt}'; do echo \$v; done $

→ for v in '${__0_txt}'; do echo $v; done (bad, not interpolated at all) → for v in ''${__0_txt}''; do echo $v; done (bad, printing two lines)

It this case, the var_expr should be double-quoted between the empty single quotations. → for v in ''"${__0_txt}"''; do echo $v; done (good)

lens0021 avatar Jun 24 '25 00:06 lens0021

@lens0021 after my testings I found that the compiler parses the snippet correctly:

let position = 1
trust $jq -r '.[0].assets.[{position}].browser_download_url'$

Translates to:

__0_position=1
jq -r '.[0].assets.[${__0_position}].browser_download_url';
__AS=$?

What happened

Heraclitus parses this command region to the following form:

["$", "jq -r '.[0].assets.[", "{", "position", "}", "].browser_download_url'", "$"]

In order to solve the problem we must end the single quote string before the interpolation. This is how to do it:

$ cmd 'this is a string {var} something' $
# Should be translated to
$ cmd 'this is a string '{var}' something' $

This means that we need to check if according to the chunk of command before and after we are in an single_quote_context or not, and if so then we should end the region by wrapping the value with closing and ending just like in the example above. This is quite tricky though because the single quote does not always mean that it's a single quote string region. Here are some edge cases:

# These works properly:
echo "'$a'"
echo 'He said: '\''$a'\''!' # we need to handle escape symbols too
echo "`echo 'hi'`"
echo "$(echo 'hi')"
# This line has a ' but it does nothing
cat <<'END'
This is inside a single-quoted here-doc.
Single 'quotes' here won't start a new region.
END

Generally speaking to solve this properly we must accept that here anything could be written and if we start to support other shell targets, then the list of edge cases is going to grow. In my opinion we should close this issue and let the user close the single quote brackets. When they use a command syntax, then it's up to them to understand the bash code lying underneath.

This issue could be solved soon when we have nothing more important to do.

Ph0enixKM avatar Jun 27 '25 08:06 Ph0enixKM

I understand your concerns for possibly growing edge cases.

Then, I have another idea. It is, without further parsing, wrapping the whole command with eval after escaping ".

  • trust $ echo "'$a'" $ → eval "echo \"'${__0_a}'\""
  • trust $ echo 'He said: '\''$a'\''!' $ - I don't know this exclamation mark is used for the special meaning in Bash or just a mistake. Skipping now.
  • trust $ echo "`echo 'hi'`" $ → eval "echo "`echo 'hi'`""
  • trust $ echo \"$(echo 'hi')\" $ → eval "echo \"$(echo 'hi')\""
  • trust $ # This line has a ' but it does nothing $ → eval "# this line has a ' but it does nothing"
  • trust $ cat <<'END'
    This is inside a single-quoted here-doc.
    Single 'quotes' here won't start a new region.
    END $
    
    This is not compiled at all. I don't remember Amber supports multi-line command.

Could you consider this too? @Ph0enixKM

lens0021 avatar Jun 27 '25 09:06 lens0021

I've tried my InterpolableRenderType::GlobalContext => format!("eval {quote}{result}{quote}") suggestion and discovered another edge case.

const s = "single quotes"
const s2 = "'{s}'"
trust $ echo '{s2}' $
$ amber build test.ab -
s="single quotes"
s2="'${s}'"
eval " echo '${s2}' "
$ amber run test.ab
single quotes

So I agree with this

In my opinion we should close this issue and let the user close the single quote brackets. When they use a command syntax, then it's up to them to understand the bash code lying underneath.

now completely.

Then, would we add a warning when single quote is used in command?

Hm, the above is maybe expected?

lens0021 avatar Jun 27 '25 12:06 lens0021

Sorry @lens0021 I didn't reply immediately but I wanted to tell you that I don't like this idea of wrapping every command into an eval context. This would further slow down the language and make the code even less readable.

Ph0enixKM avatar Jun 29 '25 18:06 Ph0enixKM

@Mte90 This is an out of context question: Where the word apix is coined from? I couldn't find it anywhere.

  • https://en.wikipedia.org/wiki/Apostrophe
  • https://www.wikidata.org/wiki/Q436048
  • Google search results

lens0021 avatar Oct 02 '25 15:10 lens0021

My bad I wrote wrong the word.

Mte90 avatar Oct 02 '25 15:10 Mte90