fizyr / subst

Apache License 2.0
12 stars 5 forks source link

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

Closed cre4ture closed 8 months ago

cre4ture commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

cre4ture commented 9 months ago

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

cre4ture commented 9 months ago

I'll try ...

de-vri-es commented 8 months ago

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