syntax: rethink IfClause for v3

Following a TODO from a while ago, we can remove three fields from
IfClause entirely. The node becomes a bit easier to work with, mainly
because of the reduction of redundant fields.

The most important change is that Else is now an *IfClause, representing
either an "elif" or an "else" if non-nil. The other notable changes is
that FiPos is always valid, and ThenPos is no longer valid in the "else"
case.

Finally, stop the printer from removing empty else branches. It's not
its place to remove pieces of nodes. We might teach syntax.Simplify to
do this in the future.

While at it, add a v3 TODO to update Simplify's signature.

interp/interp.go

@@ -801,7 +801,9 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) {
 			break
 		}
 		r.exit = 0
-		r.stmts(ctx, x.Else)
+		if x.Else != nil {
+			r.cmd(ctx, x.Else)
+		}
 	case *syntax.WhileClause:
 		for !r.stop(ctx) {
 			r.stmts(ctx, x.Cond)

syntax/filetests_test.go

@@ -213,7 +213,9 @@ var fileTests = []testCase{
 		common: &IfClause{
 			Cond: litStmts("a"),
 			Then: litStmts("b"),
-			Else: litStmts("c"),
+			Else: &IfClause{
+				Then: litStmts("c"),
+			},
 		},
 	},
 	{
@@ -223,12 +225,13 @@ var fileTests = []testCase{
 		common: &IfClause{
 			Cond: litStmts("a"),
 			Then: litStmts("a"),
-			Else: stmtList(stmt(&IfClause{
-				Elif: true,
+			Else: &IfClause{
 				Cond: litStmts("b"),
 				Then: litStmts("b"),
-				Else: litStmts("c"),
-			})),
+				Else: &IfClause{
+					Then: litStmts("c"),
+				},
+			},
 		},
 	},
 	{
@@ -239,17 +242,17 @@ var fileTests = []testCase{
 		common: &IfClause{
 			Cond: litStmts("a"),
 			Then: litStmts("a"),
-			Else: stmtList(stmt(&IfClause{
-				Elif: true,
+			Else: &IfClause{
 				Cond: litStmts("b"),
 				Then: litStmts("b"),
-				Else: stmtList(stmt(&IfClause{
-					Elif: true,
+				Else: &IfClause{
 					Cond: litStmts("c"),
 					Then: litStmts("c"),
-					Else: litStmts("d"),
-				})),
-			})),
+					Else: &IfClause{
+						Then: litStmts("d"),
+					},
+				},
+			},
 		},
 	},
 	{
@@ -4351,15 +4354,16 @@ func clearPosRecurse(tb testing.TB, src string, v interface{}) {
 		setPos(&x.Rbrace, "}")
 		recurse(x.StmtList)
 	case *IfClause:
-		setPos(&x.IfPos, "if", "elif")
-		setPos(&x.ThenPos, "then")
-		if x.FiPos.IsValid() {
-			setPos(&x.FiPos, "fi", "elif")
+		if x.ThenPos.IsValid() {
+			setPos(&x.Position, "if", "elif")
+			setPos(&x.ThenPos, "then")
+		} else {
+			setPos(&x.Position, "else")
 		}
+		setPos(&x.FiPos, "fi")
 		recurse(x.Cond)
 		recurse(x.Then)
-		if !x.Else.empty() {
-			setPos(&x.ElsePos, "else", "elif")
+		if x.Else != nil {
 			recurse(x.Else)
 		}
 	case *WhileClause:

syntax/nodes.go

@@ -294,51 +294,21 @@ type Block struct {
 func (b *Block) Pos() Pos { return b.Lbrace }
 func (b *Block) End() Pos { return posAddCol(b.Rbrace, 1) }
 
-// TODO(v3): Refactor and simplify elif/else. For example, we could likely make
-// Else an *IfClause, remove ElsePos, make IfPos also do opening "else"
-// positions, and join the comment slices as Last []Comment.
-
 // IfClause represents an if statement.
 type IfClause struct {
-	Elif    bool // whether this IfClause begins with "elif"
-	IfPos   Pos  // position of the starting "if" or "elif" token
-	ThenPos Pos
-	ElsePos Pos // position of a following "else" or "elif", if any
-	FiPos   Pos // position of "fi", empty if Elif == true
+	Position Pos // position of the starting "if", "elif", or "else" token
+	ThenPos  Pos // position of "then", empty if this is an "else"
+	FiPos    Pos // position of "fi", shared with .Else if non-nil
 
 	Cond StmtList
 	Then StmtList
-	Else StmtList
+	Else *IfClause // if non-nil, an "elif" or an "else"
 
-	ElseComments []Comment // comments on the "else"
-	FiComments   []Comment // comments on the "fi"
+	Last []Comment // comments on the first "elif", "else", or "fi"
 }
 
-func (c *IfClause) Pos() Pos { return c.IfPos }
-func (c *IfClause) End() Pos {
-	if !c.FiPos.IsValid() {
-		return posAddCol(c.ElsePos, 4)
-	}
-	return posAddCol(c.FiPos, 2)
-}
-
-// FollowedByElif reports whether this IfClause is followed by an "elif"
-// IfClause in its Else branch. This is true if Else.Stmts has exactly one
-// statement with an IfClause whose Elif field is true.
-func (c *IfClause) FollowedByElif() bool {
-	if len(c.Else.Stmts) != 1 {
-		return false
-	}
-	ic, _ := c.Else.Stmts[0].Cmd.(*IfClause)
-	return ic != nil && ic.Elif
-}
-
-func (c *IfClause) bodyEndPos() Pos {
-	if c.ElsePos.IsValid() {
-		return c.ElsePos
-	}
-	return c.FiPos
-}
+func (c *IfClause) Pos() Pos { return c.Position }
+func (c *IfClause) End() Pos { return posAddCol(c.FiPos, 2) }
 
 // WhileClause represents a while or an until clause.
 type WhileClause struct {

syntax/parser.go

@@ -1924,37 +1924,39 @@ func (p *Parser) block(s *Stmt) {
 }
 
 func (p *Parser) ifClause(s *Stmt) {
-	rif := &IfClause{IfPos: p.pos}
+	rootIf := &IfClause{Position: p.pos}
 	p.next()
-	rif.Cond = p.followStmts("if", rif.IfPos, "then")
-	rif.ThenPos = p.followRsrv(rif.IfPos, "if <cond>", "then")
-	rif.Then = p.followStmts("then", rif.ThenPos, "fi", "elif", "else")
-	curIf := rif
+	rootIf.Cond = p.followStmts("if", rootIf.Position, "then")
+	rootIf.ThenPos = p.followRsrv(rootIf.Position, "if <cond>", "then")
+	rootIf.Then = p.followStmts("then", rootIf.ThenPos, "fi", "elif", "else")
+	curIf := rootIf
 	for p.tok == _LitWord && p.val == "elif" {
-		elf := &IfClause{IfPos: p.pos, Elif: true}
-		s := p.stmt(elf.IfPos)
-		s.Cmd = elf
-		s.Comments = p.accComs
+		elf := &IfClause{Position: p.pos}
+		curIf.Last = p.accComs
 		p.accComs = nil
 		p.next()
-		elf.Cond = p.followStmts("elif", elf.IfPos, "then")
-		elf.ThenPos = p.followRsrv(elf.IfPos, "elif <cond>", "then")
+		elf.Cond = p.followStmts("elif", elf.Position, "then")
+		elf.ThenPos = p.followRsrv(elf.Position, "elif <cond>", "then")
 		elf.Then = p.followStmts("then", elf.ThenPos, "fi", "elif", "else")
-		curIf.ElsePos = elf.IfPos
-		curIf.Else.Stmts = []*Stmt{s}
+		curIf.Else = elf
 		curIf = elf
 	}
 	if elsePos, ok := p.gotRsrv("else"); ok {
-		curIf.ElseComments = p.accComs
+		curIf.Last = p.accComs
 		p.accComs = nil
-		curIf.ElsePos = elsePos
-		curIf.Else = p.followStmts("else", curIf.ElsePos, "fi")
+		els := &IfClause{Position: elsePos}
+		els.Then = p.followStmts("else", els.Position, "fi")
+		curIf.Else = els
+		curIf = els
 	}
-	curIf.FiComments = p.accComs
+	curIf.Last = p.accComs
 	p.accComs = nil
-	rif.FiPos = p.stmtEnd(rif, "if", "fi")
-	curIf.FiPos = rif.FiPos
-	s.Cmd = rif
+	rootIf.FiPos = p.stmtEnd(rootIf, "if", "fi")
+	for els := rootIf.Else; els != nil; els = els.Else {
+		// All the nested IfClauses share the same FiPos.
+		els.FiPos = rootIf.FiPos
+	}
+	s.Cmd = rootIf
 }
 
 func (p *Parser) whileClause(s *Stmt, until bool) {

syntax/printer.go

@@ -1058,31 +1058,34 @@ func (p *Printer) ifClause(ic *IfClause, elif bool) {
 	}
 	p.nestedStmts(ic.Cond, Pos{})
 	p.semiOrNewl("then", ic.ThenPos)
-	p.nestedStmts(ic.Then, ic.bodyEndPos())
-
-	var left []Comment
-	for _, c := range ic.ElseComments {
-		if c.Pos().After(ic.ElsePos) {
-			left = append(left, c)
-			break
-		}
-		p.comments(c)
+	thenEnd := ic.FiPos
+	if ic.Else != nil {
+		thenEnd = ic.Else.Position
 	}
-	if ic.FollowedByElif() {
-		s := ic.Else.Stmts[0]
-		p.comments(s.Comments...)
-		p.semiRsrv("elif", ic.ElsePos)
-		p.ifClause(s.Cmd.(*IfClause), true)
+	p.nestedStmts(ic.Then, thenEnd)
+
+	if ic.Else != nil && ic.Else.ThenPos.IsValid() {
+		p.comments(ic.Last...)
+		p.semiRsrv("elif", ic.Else.Position)
+		p.ifClause(ic.Else, true)
 		return
 	}
-	if !ic.Else.empty() {
-		p.semiRsrv("else", ic.ElsePos)
+	if ic.Else == nil {
+		p.comments(ic.Last...)
+	} else {
+		var left []Comment
+		for _, c := range ic.Last {
+			if c.Pos().After(ic.Else.Position) {
+				left = append(left, c)
+				break
+			}
+			p.comments(c)
+		}
+		p.semiRsrv("else", ic.Else.Position)
 		p.comments(left...)
-		p.nestedStmts(ic.Else, ic.FiPos)
-	} else if ic.ElsePos.IsValid() {
-		p.line = ic.ElsePos.Line()
+		p.nestedStmts(ic.Else.Then, ic.FiPos)
+		p.comments(ic.Else.Last...)
 	}
-	p.comments(ic.FiComments...)
 	p.semiRsrv("fi", ic.FiPos)
 }
 

syntax/printer_test.go

@@ -81,7 +81,7 @@ var printTests = []printCase{
 	samePrint("foo # inline\n# after"),
 	samePrint("$(a) $(b)"),
 	{"if a\nthen\n\tb\nfi", "if a; then\n\tb\nfi"},
-	{"if a; then\nb\nelse\nfi", "if a; then\n\tb\nfi"},
+	samePrint("if a; then\n\tb\nelse\nfi"),
 	{"if a; then b\nelse c\nfi", "if a; then\n\tb\nelse\n\tc\nfi"},
 	samePrint("foo >&2 <f bar"),
 	samePrint("foo >&2 bar <f"),

syntax/simplify.go

@@ -5,6 +5,8 @@ package syntax
 
 import "bytes"
 
+// TODO(v3): replace bool with a Node return, like in SplitBraces.
+
 // Simplify simplifies a given program and returns whether any changes
 // were made.
 //

syntax/walk.go

@@ -84,7 +84,9 @@ func Walk(node Node, f func(Node) bool) {
 	case *IfClause:
 		walkStmts(x.Cond, f)
 		walkStmts(x.Then, f)
-		walkStmts(x.Else, f)
+		if x.Else != nil {
+			Walk(x.Else, f)
+		}
 	case *WhileClause:
 		walkStmts(x.Cond, f)
 		walkStmts(x.Do, f)