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

os: Expand eats $s in the middle of the string (but not at the end) #24345

Closed
opennota opened this issue Mar 11, 2018 · 22 comments
Closed

os: Expand eats $s in the middle of the string (but not at the end) #24345

opennota opened this issue Mar 11, 2018 · 22 comments

Comments

@opennota
Copy link

@opennota opennota commented Mar 11, 2018

What version of Go are you using (go version)?

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/opennota/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/opennota/gocode"
GORACE=""
GOROOT="/home/opennota/go"
GOTMPDIR=""
GOTOOLDIR="/home/opennota/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build224817525=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/ulxC4AgmVNF

What did you expect to see?

The program outputs abc$|def$

What did you see instead?

The program outputs abc|def$

This is unfortunate, because go generate uses os.Expand to expand environment variables in the command line and if one needs to pass a regexp to the executed command, the regexp gets garbled.

@peczenyj

This comment has been minimized.

Copy link

@peczenyj peczenyj commented Mar 11, 2018

Hello

This seems an side effect os this https://golang.org/pkg/os/#ExpandEnv

References to undefined variables are replaced by the empty string.

a variable is defined as$var or ${var} where var is alphanum

so $PATH is a variable, $| is not - and the code

https://golang.org/src/os/env.go#L26

will always remove variables even if it is not a valid variable ( like remove ${|} ) because everything who starts with an $ is a potential variable

in fact the documentation omit few things:

  • there is a validation of the variable name before expand it
  • there is no validation of the variable name to replace it by empty string

It seems pretty KISS: if you try to expand $$, there is no variable $$ (bash uses the value of $PPID , etc) so the code remove it. the only exception is the trailing $

and a single $ is removed with no warning

this code can review part of the inner logic:
https://play.golang.org/p/D4fFKTFoD0N

IMHO we may describe this behavior a little bit better on the documentation.

To fix your issue: maybe you can use two templates:

  • one to be expanded
  • another to be converted on a regular expression (by using fmt.Sprintf to add the metacharacters)

this give you the ability to validate or escape the expanded string if needed

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Mar 11, 2018

To be consistent, I think the trailing $ needs to be removed too. The first $ is correctly gobbled because | is neither a shell special variable nor an alphanumeric character. The expected output, IMO, should be abc|def.

Moreover abc$|def${} is expanded to abc|def, whereas abc$|def$ is expanded to abc|def$.

It's a relatively small change. /cc @ianlancetaylor for decision.

@opennota

This comment has been minimized.

Copy link
Author

@opennota opennota commented Mar 11, 2018

@agnivade

Correct me if I'm wrong, but with your proposed change nobody would be able to use a literal $ in the arguments to a go:generate command, like

//go:generate esc -o static.go -ignore "\\.map$" build/
@opennota

This comment has been minimized.

Copy link
Author

@opennota opennota commented Mar 11, 2018

I believe that the correct way to fix it is to leave $ alone in case it can't be recognized as a variable, as bash does:

$ echo -e "$$\n$?\n$+\n$-\n$|\n$"
4292
0
$+
himBH
$|
$
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Mar 11, 2018

My proposal does not change that behavior. It is already the existing behavior. My proposal was to make it consistent. Theoretically, I agree with your proposal, but I am not sure how much of a breaking change is this considered to be.

I will leave it to others to make the call.

@mvdan mvdan added the NeedsDecision label Mar 11, 2018
@as

This comment has been minimized.

Copy link
Contributor

@as as commented Mar 11, 2018

I believe that the correct way to fix it is to leave $ alone in case it can't be recognized as a variable, as bash does:

Thats sounds incorrect.

The bug is that $ doesn't consider the a zero length string/eof to be a valid follow token, but it works when ${} is at the end of the string. This logic is on the domain of the function during the lexing stage. You're suggesting instead, that the function should look at the range after the evaluation of the mapping function has happened, and treating its output (the empty string) as a special case.

The documentation says that os.Expand replaces the ${var} or $var in the string based on the mapping function. It doesn't matter if the mapping function returns an empty string, your bug happens because of the lexical scanning never even makes it call the mapping function in the first place.

@opennota

This comment has been minimized.

Copy link
Author

@opennota opennota commented Mar 12, 2018

@as
You're speaking in terms of implementation simplicity. I speak in terms of the expected behavior.

@as

This comment has been minimized.

Copy link
Contributor

@as as commented Mar 12, 2018

@opennota I don't expect this behavior either. I expect all tokens denoting a potential variable name to be excised after one pass of os.Expand

Besides, your desired behavior is trivial to implement: just make your own func (s string) string that returns a literal $ on empty input to it.

@opennota

This comment has been minimized.

Copy link
Author

@opennota opennota commented Mar 12, 2018

For the reference, in both Python 2 and 3 os.path.expandvars doesn't even touch variables with non-alphanumeric names:

>>> import os
>>> os.path.expandvars('$$ $? $+ $- $| $')
'$$ $? $+ $- $| $'

That's the most sane behaviour, IMO.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 12, 2018

This issue seems to be about two different things.

The first is that os.Expand ignores a $ at the end of the input string. That does seem peculiar though it's also true that there is no obvious way to handle it. It's not obvious to that we should just remove it. It seems more consistent to me to call the function with the empty string, but that could break existing code. Perhaps we should just document the behavior and carry on. Most people who call os.Expand have some control over the input string, and so need not worry about a trailing $.

The second problem in this issue is that go generate uses os.Expand, and that makes it hard to pass a regular expression as an argument in a go generate command. That is true. I don't see any way backward compatible way to fix that. That problem doesn't really have anything to do with the way that os.Expand handles a trailing $, it's a general problem with any use of $. After all, a regular expression can contain $ at places other than at the end, such as within []. Actually, I take it back: we could address this with only a minor break in compatibility by expanding $$ to $; currently I believe that $$ always expands to the empty string.

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 12, 2018
@peczenyj

This comment has been minimized.

Copy link

@peczenyj peczenyj commented Mar 12, 2018

How about allow escape $ as literal $ ?
If it is too bad ( backward compatibility, etc ), we can add an option/flag to escape or add an new explicit method for this (maybe in go 2)

@peczenyj

This comment has been minimized.

Copy link

@peczenyj peczenyj commented Mar 12, 2018

I try to wrote \$ sorry

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 12, 2018

@peczenyj First let me repeat that I think there there are two separate problems here. Which one are you trying to address?

That said, I don't agree with adding backslash quoting to either case. It's not necessary, because we can use $$. It introduces more complexity, since now we need to use backslash quoting for backslash itself. It naturally leads to people wondering about using backslash quoting for other characters, such as \n. Is that two characters \ n, or one character n, or one character ASCII LF?

@opennota

This comment has been minimized.

Copy link
Author

@opennota opennota commented Mar 12, 2018

@ianlancetaylor
How about ignoring $ altogether unless it is followed by NAME or {NAME}? I fail to see how the existing code might reasonably depend on os.Expand removing single $s, and it's not that there were no breaking changes in the past (the cgo pointer passing rules introduced in Go 1.6 come to mind).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 12, 2018

The cgo pointer passing rules were different, as use of cgo is not covered by the Go 1 compatibility guarantee.

The current os.Expand implementation has special code to support $*, etc., and it doesn't seem right to drop all of that.

@opennota

This comment has been minimized.

Copy link
Author

@opennota opennota commented Mar 14, 2018

The current os.Expand implementation has special code to support $*, etc., and it doesn't seem right to drop all of that.

Why not? It was never officially supported (Expand replaces ${var} or $var, unquote), and if someone uses this (is there anyone who does? Go itself doesn't.), they do it on their own risk.

Alternatively, allow only a limited subset of characters to be treated as a variable, otherwise leave $ alone.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 14, 2018

Let's not change existing behavior unless we are solving a clear problem.

We already check for a list of special characters; is isShellSpecialVar in os/expand.go. But changing os.Expand so that $| is left as $| but $? disappears seems to me to increase confusion, not decrease it.

@rsc rsc changed the title os: Expand eats $$s in the middle of the string (but not at the end) os: Expand eats $\s in the middle of the string (but not at the end) Mar 26, 2018
@rsc rsc changed the title os: Expand eats $\s in the middle of the string (but not at the end) os: Expand eats $s in the middle of the string (but not at the end) Mar 26, 2018
@spf13

This comment has been minimized.

Copy link
Contributor

@spf13 spf13 commented Mar 26, 2018

We should be consistent. It seems that keeping the $ in both cases is ideal rather than eliminating it only at the end.

@spf13 spf13 added the NeedsFix label Mar 26, 2018
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Mar 27, 2018

@spf13 - So just to be clear, you want $ to remain in the output in case the following character is neither a shell special variable nor alphanumeric . So, abc$|def$ would result in abc$|def$.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 27, 2018

Yes.

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Mar 27, 2018

Cool. Will send a CL.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 28, 2018

Change https://golang.org/cl/103055 mentions this issue: os: keep the $ if a variable is not detected

@golang golang locked and limited conversation to collaborators Mar 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.