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

[BUG] Cannot shadow variables which are already declared readonly #666

Open
karpfediem opened this issue Jan 15, 2025 · 5 comments
Open
Labels
bug Something isn't working stdlib

Comments

@karpfediem
Copy link
Contributor

Describe the bug
The generated printf function tries create a local variable by the name "args". If this variable name was previously declared as readonly, bash exits with the error:

$ amber run name-collision.ab 
environment: line 9: args: readonly variable
environment: line 9: local: args: readonly variable
environment: line 10: args: readonly variable

amber 0.4.0-alpha
rev: d3ceda3

To Reproduce
File: name-collision.ab

import { echo_info } from "std/env"

main(args) {
echo_info("hi")
}
$ amber run name-collision.ab 
environment: line 9: args: readonly variable
environment: line 9: local: args: readonly variable
environment: line 10: args: readonly variable

Expected behavior
The scripts should not crash and be able to assign local variables.
We might need to use a prefix or generate variable names a user likely won't use in their own code. (Which would unfortunately make code more unreadable).

Additional context
Some discussions can be found here
https://lists.gnu.org/archive/html/bug-bash/2011-02/msg00105.html
and here
https://lists.gnu.org/archive/html/bug-bash/2019-03/msg00150.html

In the name-collision.ab example given in this issue, the variable args is already assigned as global read-only variable:

#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.4.0-alpha
# date: 2025-01-15 17:04:11


printf__99_v0() {
    local format=$1
    local args=("${!2}")
     args=("${format}" "${args[@]}") ;
    __AS=$?
     printf "${args[@]}" ;
    __AS=$?
}
echo_info__106_v0() {
    local message=$1
    __AMBER_ARRAY_0=("${message}");
    printf__99_v0 "\x1b[1;3;97;44m%s\x1b[0m
" __AMBER_ARRAY_0[@];
    __AF_printf99_v0__147_5="$__AF_printf99_v0";
    echo "$__AF_printf99_v0__147_5" > /dev/null 2>&1
}
declare -r args=("$0" "$@")
    echo_info__106_v0 "hi";
    __AF_echo_info106_v0__4_1="$__AF_echo_info106_v0";
    echo "$__AF_echo_info106_v0__4_1" > /dev/null 2>&1

Relevant Parts:

declare -r args=("$0" "$@")

and later a reassignment even if shadowed is rejected:

    local args=("${!2}")
@karpfediem karpfediem added the bug Something isn't working label Jan 15, 2025
@github-project-automation github-project-automation bot moved this to 🆕 New in Amber Project Jan 15, 2025
@Mte90 Mte90 added the stdlib label Jan 15, 2025
@Mte90
Copy link
Member

Mte90 commented Jan 15, 2025

So we need to improve this function code https://github.com/amber-lang/amber/blob/main/src/std/env.ab#L110

Maybe we can use a variable name less common like __args?

@hdwalters
Copy link
Contributor

hdwalters commented Jan 15, 2025

So we need to improve this function code https://github.com/amber-lang/amber/blob/main/src/std/env.ab#L110

Maybe we can use a variable name less common like __args?

That would only fix the problem in this instance. I think a better solution would be to name-mangle local as well as global variables, by using local __0_args instead of local args.

@hdwalters
Copy link
Contributor

Specifically, we would need to change https://github.com/amber-lang/amber/blob/main/src/modules/variable/init.rs#L72, and other lines like it. This is one reason I've been wanting to centralise all Bash variable name generation, so we don't have to go searching for all instances... but that may be a larger task than is called for at this point.

@hdwalters
Copy link
Contributor

Even better would be to just always assign a global ID. I'm trying a fix.

@hdwalters
Copy link
Contributor

However, since this affects the translation layer, it would be sensible to wait until @Ph0enixKM has merged his forthcoming PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stdlib
Projects
None yet
Development

No branches or pull requests

3 participants