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

error when variable name starts with 0..9 #12

Closed

Conversation

cre4ture
Copy link
Contributor

@cre4ture cre4ture commented Jan 7, 2024

This change checks if the variable name starts with a number and raises an error in this case.

As already stateted in my first PR (#11), I'm integrating subst into a "shell-words" like funktionality needed by the uutils/coreutils-env app.
I had to do this change for our tests to succeed.

To avoid that we need to do a fork of your code, we want to bring it into your main branch.
Please tell me what you think.

Kind regards

@de-vri-es
Copy link
Contributor

de-vri-es commented Jan 7, 2024

Hey! Thanks for the PR.

But I don't see why you want to error in these cases. $0, $1 etc are valid variables in shell script :o

I don't want to forbid this in general. If there is a good use case for forbidding this, it needs to be configurable somehow.

@cre4ture
Copy link
Contributor Author

cre4ture commented Jan 7, 2024

Thanks for the quick response. You've got a point.
It seems to be a bit more tricky than I thought.

The main idea is to replicate the behaviour of existing tools/shells.

E.g. the test suite for "env" tool expects a variable expansion with this name to fail: ${9B}.
I did a bit manual testing, and it seems that variable with a pure numeric name are accepted for expansion but not for assignment. Also, with curly braces, only the first digit after the $ counts. See terminal output below.

What do you think?

$ echo $4TUR
TUR
$ echo ${4TUR}
bash: ${4TUR}: bad substitution
$ echo ${TUR}

$ echo ${T4UR}

$ echo ${4T4UR}
bash: ${4T4UR}: bad substitution
$ echo ${4TUR}
bash: ${4TUR}: bad substitution
$ echo ${4}

$ echo ${9}

$ echo ${10}

$ echo ${100}

$ echo ${100d}
bash: ${100d}: bad substitution
$ echo ${100D}
bash: ${100D}: bad substitution
$ sh
$ echo $4

$ echo ${4R}
sh: 2: Bad substitution
$ 4R=hello
sh: 3: 4R=hello: not found
$ VAR=hello
$ 4VAR=hello
sh: 5: 4VAR=hello: not found
$ 10=hello
sh: 6: 10=hello: not found
$ 1=hello
sh: 7: 1=hello: not found

@de-vri-es
Copy link
Contributor

de-vri-es commented Jan 7, 2024

Hmm.. I see you point. Another thing I was considering at some point was to expose a function that parses a string template but doesn't perform expansion yet. It would returns something like this:

fn parse_template(input: &[u8]) -> Result<Template, Error>

struct Template<'a> {
  parts: Vec<TemplatePart>,
}

enum TemplatePart<'a> {
  Literal(Literal),
  Variable(Variable),
}

struct Literal<'a> {
  text: &'a [u8],
  ...
}

struct Variable<'a> {
  name: &'a [u8],
  ...
}

Then, anyone who wants can still perform extra validation on the variable names before doing the expansion.

As a bonus, it allows more efficient re-expansion of the same string.

@de-vri-es
Copy link
Contributor

Depending on your needs, I can imagine that a system like that might also solve the underlying issue of #11

@cre4ture
Copy link
Contributor Author

cre4ture commented Jan 7, 2024

I see what you mean. But I assume this would be a bigger change. Right?

@cre4ture
Copy link
Contributor Author

cre4ture commented Jan 7, 2024

I'll try ...

@de-vri-es
Copy link
Contributor

Thanks for the PRs! Closing this one in favor of the other ones.

@de-vri-es de-vri-es closed this Jan 8, 2024
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.

2 participants