Skip to content

Commit

Permalink
syntax: pipes are left-associative too
Browse files Browse the repository at this point in the history
That is, the program:

	foo | bar | baz

Should parse with recursive pipes at the left, like:

	       |
	    |     baz
	foo   bar

We already did this for && and ||, but not pipes. This makes the syntax
parser more consistent and correct.

We noticed now because we added an optimization where a shell's pipe
would reuse copy buffers. With right associativity, the original shell
performs all pipes, so the same buffer was used by multiple copies at
the same time, resulting in bugs.

In other words, a shell pipe forks the left side into a subshell, so
nested pipes should go to the subshell.
  • Loading branch information
mvdan committed Dec 21, 2018
1 parent e18078e commit 9446074
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 17 deletions.
8 changes: 4 additions & 4 deletions syntax/filetests_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -637,12 +637,12 @@ var fileTests = []testCase{
Strs: []string{"foo | bar | extra"},
common: &BinaryCmd{
Op: Pipe,
X: litStmt("foo"),
Y: stmt(&BinaryCmd{
X: stmt(&BinaryCmd{
Op: Pipe,
X: litStmt("bar"),
Y: litStmt("extra"),
X: litStmt("foo"),
Y: litStmt("bar"),
}),
Y: litStmt("extra"),
},
},
{
Expand Down
29 changes: 16 additions & 13 deletions syntax/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -1679,14 +1679,14 @@ func (p *Parser) getStmt(readEnd, binCmd, fnBody bool) *Stmt {
p.posErr(s.Pos(), `cannot negate a command multiple times`)
}
}
if s = p.gotStmtPipe(s); s == nil || p.err != nil {
if s = p.gotStmtPipe(s, false); s == nil || p.err != nil {
return nil
}
// instead of using recursion, iterate manually
for p.tok == andAnd || p.tok == orOr {
// left associativity: in a list of BinaryCmds, the
// right recursion should only read a single element
if binCmd {
// left associativity: in a list of BinaryCmds, the
// right recursion should only read a single element
return s
}
b := &BinaryCmd{
Expand Down Expand Up @@ -1730,7 +1730,7 @@ func (p *Parser) getStmt(readEnd, binCmd, fnBody bool) *Stmt {
return s
}

func (p *Parser) gotStmtPipe(s *Stmt) *Stmt {
func (p *Parser) gotStmtPipe(s *Stmt, binCmd bool) *Stmt {
s.Comments, p.accComs = p.accComs, nil
switch p.tok {
case _LitWord:
Expand Down Expand Up @@ -1852,19 +1852,22 @@ func (p *Parser) gotStmtPipe(s *Stmt) *Stmt {
for p.peekRedir() {
p.doRedirect(s)
}
switch p.tok {
case orAnd:
if p.lang == LangMirBSDKorn {
// instead of using recursion, iterate manually
for p.tok == or || p.tok == orAnd {
if binCmd {
// left associativity: in a list of BinaryCmds, the
// right recursion should only read a single element
return s
}
if p.tok == orAnd && p.lang == LangMirBSDKorn {
// No need to check for LangPOSIX, as on that language
// we parse |& as two tokens.
break
}
fallthrough
case or:
b := &BinaryCmd{OpPos: p.pos, Op: BinCmdOperator(p.tok), X: s}
p.next()
p.got(_Newl)
if b.Y = p.gotStmtPipe(p.stmt(p.pos)); b.Y == nil || p.err != nil {
if b.Y = p.gotStmtPipe(p.stmt(p.pos), true); b.Y == nil || p.err != nil {
p.followErr(b.OpPos, b.Op.String(), "a statement")
break
}
Expand Down Expand Up @@ -2291,20 +2294,20 @@ func (p *Parser) timeClause(s *Stmt) {
if _, ok := p.gotRsrv("-p"); ok {
tc.PosixFormat = true
}
tc.Stmt = p.gotStmtPipe(p.stmt(p.pos))
tc.Stmt = p.gotStmtPipe(p.stmt(p.pos), false)
s.Cmd = tc
}

func (p *Parser) coprocClause(s *Stmt) {
cc := &CoprocClause{Coproc: p.pos}
if p.next(); isBashCompoundCommand(p.tok, p.val) {
// has no name
cc.Stmt = p.gotStmtPipe(p.stmt(p.pos))
cc.Stmt = p.gotStmtPipe(p.stmt(p.pos), false)
s.Cmd = cc
return
}
cc.Name = p.getLit()
cc.Stmt = p.gotStmtPipe(p.stmt(p.pos))
cc.Stmt = p.gotStmtPipe(p.stmt(p.pos), false)
if cc.Stmt == nil {
if cc.Name == nil {
p.posErr(cc.Coproc, "coproc clause requires a command")
Expand Down

0 comments on commit 9446074

Please sign in to comment.