Keep parser.Directive internal to parser

Signed-off-by: Daniel Nephin <dnephin@docker.com>

builder/dockerfile/builder.go

@@ -62,7 +62,7 @@ type Builder struct {
 	disableCommit bool
 	cacheBusted   bool
 	buildArgs     *buildArgs
-	directive     *parser.Directive
+	escapeToken   rune
 
 	imageCache builder.ImageCache
 	from       builder.Image
@@ -122,7 +122,7 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
 		runConfig:     new(container.Config),
 		tmpContainers: map[string]struct{}{},
 		buildArgs:     newBuildArgs(config.BuildArgs),
-		directive:     parser.NewDefaultDirective(),
+		escapeToken:   parser.DefaultEscapeToken,
 	}
 	b.imageContexts = &imageContexts{b: b}
 	return b, nil
@@ -190,9 +190,9 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 		return "", err
 	}
 
-	addNodesForLabelOption(dockerfile, b.options.Labels)
+	addNodesForLabelOption(dockerfile.AST, b.options.Labels)
 
-	if err := checkDispatchDockerfile(dockerfile); err != nil {
+	if err := checkDispatchDockerfile(dockerfile.AST); err != nil {
 		return "", err
 	}
 
@@ -220,10 +220,13 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	return b.image, nil
 }
 
-func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Node) (string, error) {
+func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Result) (string, error) {
-	total := len(dockerfile.Children)
+	// TODO: pass this to dispatchRequest instead
+	b.escapeToken = dockerfile.EscapeToken
+
+	total := len(dockerfile.AST.Children)
 	var shortImgID string
-	for i, n := range dockerfile.Children {
+	for i, n := range dockerfile.AST.Children {
 		select {
 		case <-b.clientCtx.Done():
 			logrus.Debug("Builder: build cancelled!")
@@ -320,13 +323,13 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con
 		return nil, err
 	}
 
-	ast, err := parser.Parse(bytes.NewBufferString(strings.Join(changes, "\n")), b.directive)
+	result, err := parser.Parse(bytes.NewBufferString(strings.Join(changes, "\n")))
 	if err != nil {
 		return nil, err
 	}
 
 	// ensure that the commands are valid
-	for _, n := range ast.Children {
+	for _, n := range result.AST.Children {
 		if !validCommitCommands[n.Value] {
 			return nil, fmt.Errorf("%s is not a valid change command", n.Value)
 		}
@@ -337,11 +340,11 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con
 	b.Stderr = ioutil.Discard
 	b.disableCommit = true
 
-	if err := checkDispatchDockerfile(ast); err != nil {
+	if err := checkDispatchDockerfile(result.AST); err != nil {
 		return nil, err
 	}
 
-	if err := dispatchFromDockerfile(b, ast); err != nil {
+	if err := dispatchFromDockerfile(b, result); err != nil {
 		return nil, err
 	}
 	return b.runConfig, nil
@@ -356,8 +359,12 @@ func checkDispatchDockerfile(dockerfile *parser.Node) error {
 	return nil
 }
 
-func dispatchFromDockerfile(b *Builder, ast *parser.Node) error {
+func dispatchFromDockerfile(b *Builder, result *parser.Result) error {
+	// TODO: pass this to dispatchRequest instead
+	b.escapeToken = result.EscapeToken
+	ast := result.AST
 	total := len(ast.Children)
+
 	for i, n := range ast.Children {
 		if err := b.dispatch(i, total, n); err != nil {
 			return err

builder/dockerfile/builder_test.go

@@ -10,8 +10,7 @@ import (
 
 func TestAddNodesForLabelOption(t *testing.T) {
 	dockerfile := "FROM scratch"
-	d := parser.NewDefaultDirective()
+	result, err := parser.Parse(strings.NewReader(dockerfile))
-	nodes, err := parser.Parse(strings.NewReader(dockerfile), d)
 	assert.NilError(t, err)
 
 	labels := map[string]string{
@@ -21,6 +20,7 @@ func TestAddNodesForLabelOption(t *testing.T) {
 		"org.b": "cli-b",
 		"org.a": "cli-a",
 	}
+	nodes := result.AST
 	addNodesForLabelOption(nodes, labels)
 
 	expected := []string{

builder/dockerfile/dispatchers.go

@@ -200,7 +200,7 @@ func from(b *Builder, args []string, attributes map[string]bool, original string
 		substituionArgs = append(substituionArgs, key+"="+value)
 	}
 
-	name, err := ProcessWord(args[0], substituionArgs, b.directive.EscapeToken())
+	name, err := ProcessWord(args[0], substituionArgs, b.escapeToken)
 	if err != nil {
 		return err
 	}

builder/dockerfile/dispatchers_test.go

@@ -10,7 +10,6 @@ import (
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/strslice"
 	"github.com/docker/docker/builder"
-	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/pkg/testutil/assert"
 	"github.com/docker/go-connections/nat"
 )
@@ -205,7 +204,6 @@ func newBuilderWithMockBackend() *Builder {
 		options:   &types.ImageBuildOptions{},
 		docker:    &MockBackend{},
 		buildArgs: newBuildArgs(make(map[string]*string)),
-		directive: parser.NewDefaultDirective(),
 	}
 	b.imageContexts = &imageContexts{b: b}
 	return b

builder/dockerfile/evaluator.go

@@ -180,7 +180,7 @@ func (b *Builder) evaluateEnv(cmd string, str string, envs []string) ([]string,
 			return []string{word}, err
 		}
 	}
-	return processFunc(str, envs, b.directive.EscapeToken())
+	return processFunc(str, envs, b.escapeToken)
 }
 
 // buildArgsWithoutConfigEnv returns a list of key=value pairs for all the build

builder/dockerfile/evaluator_test.go

@@ -171,7 +171,7 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) {
 	}()
 
 	r := strings.NewReader(testCase.dockerfile)
-	n, err := parser.Parse(r, parser.NewDefaultDirective())
+	result, err := parser.Parse(r)
 
 	if err != nil {
 		t.Fatalf("Error when parsing Dockerfile: %s", err)
@@ -188,9 +188,9 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) {
 		Stdout:    ioutil.Discard,
 		context:   context,
 		buildArgs: newBuildArgs(options.BuildArgs),
-		directive: parser.NewDefaultDirective(),
 	}
 
+	n := result.AST
 	err = b.dispatch(0, len(n.Children), n.Children[0])
 
 	if err == nil {

builder/dockerfile/internals.go

@@ -462,12 +462,12 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 
 	// parse the ONBUILD triggers by invoking the parser
 	for _, step := range onBuildTriggers {
-		ast, err := parser.Parse(strings.NewReader(step), b.directive)
+		result, err := parser.Parse(strings.NewReader(step))
 		if err != nil {
 			return err
 		}
 
-		for _, n := range ast.Children {
+		for _, n := range result.AST.Children {
 			if err := checkDispatch(n); err != nil {
 				return err
 			}
@@ -481,7 +481,7 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 			}
 		}
 
-		if err := dispatchFromDockerfile(b, ast); err != nil {
+		if err := dispatchFromDockerfile(b, result); err != nil {
 			return err
 		}
 	}
@@ -650,7 +650,7 @@ func (b *Builder) clearTmp() {
 }
 
 // readAndParseDockerfile reads a Dockerfile from the current context.
-func (b *Builder) readAndParseDockerfile() (*parser.Node, error) {
+func (b *Builder) readAndParseDockerfile() (*parser.Result, error) {
 	// If no -f was specified then look for 'Dockerfile'. If we can't find
 	// that then look for 'dockerfile'.  If neither are found then default
 	// back to 'Dockerfile' and use that in the error message.
@@ -664,9 +664,9 @@ func (b *Builder) readAndParseDockerfile() (*parser.Node, error) {
 		}
 	}
 
-	nodes, err := b.parseDockerfile()
+	result, err := b.parseDockerfile()
 	if err != nil {
-		return nodes, err
+		return nil, err
 	}
 
 	// After the Dockerfile has been parsed, we need to check the .dockerignore
@@ -680,10 +680,10 @@ func (b *Builder) readAndParseDockerfile() (*parser.Node, error) {
 	if dockerIgnore, ok := b.context.(builder.DockerIgnoreContext); ok {
 		dockerIgnore.Process([]string{b.options.Dockerfile})
 	}
-	return nodes, nil
+	return result, nil
 }
 
-func (b *Builder) parseDockerfile() (*parser.Node, error) {
+func (b *Builder) parseDockerfile() (*parser.Result, error) {
 	f, err := b.context.Open(b.options.Dockerfile)
 	if err != nil {
 		if os.IsNotExist(err) {
@@ -702,5 +702,5 @@ func (b *Builder) parseDockerfile() (*parser.Node, error) {
 			return nil, fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
 		}
 	}
-	return parser.Parse(f, b.directive)
+	return parser.Parse(f)
 }

builder/dockerfile/parser/dumper/main.go

@@ -5,6 +5,7 @@ import (
 	"os"
 
 	"github.com/docker/docker/builder/dockerfile/parser"
+	"go/ast"
 )
 
 func main() {
@@ -23,12 +24,10 @@ func main() {
 		}
 		defer f.Close()
 
-		d := parser.NewDefaultDirective()
+		result, err := parser.Parse(f)
-		ast, err := parser.Parse(f, d)
 		if err != nil {
 			panic(err)
-		} else {
-			fmt.Println(ast.Dump())
 		}
+		fmt.Println(result.AST.Dump())
 	}
 }

builder/dockerfile/parser/json_test.go

@@ -28,10 +28,9 @@ var validJSONArraysOfStrings = map[string][]string{
 
 func TestJSONArraysOfStrings(t *testing.T) {
 	for json, expected := range validJSONArraysOfStrings {
-		d := Directive{}
+		d := NewDefaultDirective()
-		d.SetEscapeToken(DefaultEscapeToken)
 
-		if node, _, err := parseJSON(json, &d); err != nil {
+		if node, _, err := parseJSON(json, d); err != nil {
 			t.Fatalf("%q should be a valid JSON array of strings, but wasn't! (err: %q)", json, err)
 		} else {
 			i := 0
@@ -51,10 +50,9 @@ func TestJSONArraysOfStrings(t *testing.T) {
 		}
 	}
 	for _, json := range invalidJSONArraysOfStrings {
-		d := Directive{}
+		d := NewDefaultDirective()
-		d.SetEscapeToken(DefaultEscapeToken)
 
-		if _, _, err := parseJSON(json, &d); err != errDockerfileNotStringArray {
+		if _, _, err := parseJSON(json, d); err != errDockerfileNotStringArray {
 			t.Fatalf("%q should be an invalid JSON array of strings, but wasn't!", json)
 		}
 	}

builder/dockerfile/parser/parser.go

@@ -34,7 +34,7 @@ type Node struct {
 	Original   string          // original line used before parsing
 	Flags      []string        // only top Node should have this set
 	StartLine  int             // the line in the original dockerfile where the node begins
-	EndLine    int             // the line in the original dockerfile where the node ends
+	endLine    int             // the line in the original dockerfile where the node ends
 }
 
 // Dump dumps the AST defined by `node` as a list of sexps.
@@ -70,7 +70,7 @@ var (
 )
 
 // DefaultEscapeToken is the default escape token
-const DefaultEscapeToken = "\\"
+const DefaultEscapeToken = '\\'
 
 // Directive is the structure used during a build run to hold the state of
 // parsing directives.
@@ -81,8 +81,8 @@ type Directive struct {
 	escapeSeen            bool           // Whether the escape directive has been seen
 }
 
-// SetEscapeToken sets the default token for escaping characters in a Dockerfile.
+// setEscapeToken sets the default token for escaping characters in a Dockerfile.
-func (d *Directive) SetEscapeToken(s string) error {
+func (d *Directive) setEscapeToken(s string) error {
 	if s != "`" && s != "\\" {
 		return fmt.Errorf("invalid ESCAPE '%s'. Must be ` or \\", s)
 	}
@@ -91,18 +91,13 @@ func (d *Directive) SetEscapeToken(s string) error {
 	return nil
 }
 
-// EscapeToken returns the escape token
-func (d *Directive) EscapeToken() rune {
-	return d.escapeToken
-}
-
 // NewDefaultDirective returns a new Directive with the default escapeToken token
 func NewDefaultDirective() *Directive {
 	directive := Directive{
 		escapeSeen:           false,
 		lookingForDirectives: true,
 	}
-	directive.SetEscapeToken(DefaultEscapeToken)
+	directive.setEscapeToken(string(DefaultEscapeToken))
 	return &directive
 }
 
@@ -200,7 +195,7 @@ func handleParserDirective(line string, d *Directive) (bool, error) {
 	}
 	for i, n := range tokenEscapeCommand.SubexpNames() {
 		if n == "escapechar" {
-			if err := d.SetEscapeToken(tecMatch[i]); err != nil {
+			if err := d.setEscapeToken(tecMatch[i]); err != nil {
 				return false, err
 			}
 			return true, nil
@@ -209,9 +204,16 @@ func handleParserDirective(line string, d *Directive) (bool, error) {
 	return false, nil
 }
 
-// Parse is the main parse routine.
+// Result is the result of parsing a Dockerfile
-// It handles an io.ReadWriteCloser and returns the root of the AST.
+type Result struct {
-func Parse(rwc io.Reader, d *Directive) (*Node, error) {
+	AST         *Node
+	EscapeToken rune
+}
+
+// Parse reads lines from a Reader, parses the lines into an AST and returns
+// the AST and escape token
+func Parse(rwc io.Reader) (*Result, error) {
+	d := NewDefaultDirective()
 	currentLine := 0
 	root := &Node{}
 	root.StartLine = -1
@@ -267,18 +269,18 @@ func Parse(rwc io.Reader, d *Directive) (*Node, error) {
 		if child != nil {
 			// Update the line information for the current child.
 			child.StartLine = startLine
-			child.EndLine = currentLine
+			child.endLine = currentLine
 			// Update the line information for the root. The starting line of the root is always the
 			// starting line of the first child and the ending line is the ending line of the last child.
 			if root.StartLine < 0 {
 				root.StartLine = currentLine
 			}
-			root.EndLine = currentLine
+			root.endLine = currentLine
 			root.Children = append(root.Children, child)
 		}
 	}
 
-	return root, nil
+	return &Result{AST: root, EscapeToken: d.escapeToken}, nil
 }
 
 // covers comments and empty lines. Lines should be trimmed before passing to

builder/dockerfile/parser/parser_test.go

@@ -8,6 +8,8 @@ import (
 	"path/filepath"
 	"runtime"
 	"testing"
+
+	"github.com/docker/docker/pkg/testutil/assert"
 )
 
 const testDir = "testfiles"
@@ -16,17 +18,11 @@ const testFileLineInfo = "testfile-line/Dockerfile"
 
 func getDirs(t *testing.T, dir string) []string {
 	f, err := os.Open(dir)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
-
 	defer f.Close()
 
 	dirs, err := f.Readdirnames(0)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
-
 	return dirs
 }
 
@@ -35,15 +31,11 @@ func TestTestNegative(t *testing.T) {
 		dockerfile := filepath.Join(negativeTestDir, dir, "Dockerfile")
 
 		df, err := os.Open(dockerfile)
-		if err != nil {
+		assert.NilError(t, err)
-			t.Fatalf("Dockerfile missing for %s: %v", dir, err)
-		}
 		defer df.Close()
 
-		_, err = Parse(df, NewDefaultDirective())
+		_, err = Parse(df)
-		if err == nil {
+		assert.Error(t, err, "")
-			t.Fatalf("No error parsing broken dockerfile for %s", dir)
-		}
 	}
 }
 
@@ -53,31 +45,21 @@ func TestTestData(t *testing.T) {
 		resultfile := filepath.Join(testDir, dir, "result")
 
 		df, err := os.Open(dockerfile)
-		if err != nil {
+		assert.NilError(t, err)
-			t.Fatalf("Dockerfile missing for %s: %v", dir, err)
-		}
 		defer df.Close()
 
-		ast, err := Parse(df, NewDefaultDirective())
+		result, err := Parse(df)
-		if err != nil {
+		assert.NilError(t, err)
-			t.Fatalf("Error parsing %s's dockerfile: %v", dir, err)
-		}
 
 		content, err := ioutil.ReadFile(resultfile)
-		if err != nil {
+		assert.NilError(t, err)
-			t.Fatalf("Error reading %s's result file: %v", dir, err)
-		}
 
 		if runtime.GOOS == "windows" {
 			// CRLF --> CR to match Unix behavior
 			content = bytes.Replace(content, []byte{'\x0d', '\x0a'}, []byte{'\x0a'}, -1)
 		}
 
-		if ast.Dump()+"\n" != string(content) {
+		assert.Equal(t, result.AST.Dump()+"\n", string(content))
-			fmt.Fprintln(os.Stderr, "Result:\n"+ast.Dump())
-			fmt.Fprintln(os.Stderr, "Expected:\n"+string(content))
-			t.Fatalf("%s: AST dump of dockerfile does not match result", dir)
-		}
 	}
 }
 
@@ -119,46 +101,33 @@ func TestParseWords(t *testing.T) {
 
 	for _, test := range tests {
 		words := parseWords(test["input"][0], NewDefaultDirective())
-		if len(words) != len(test["expect"]) {
+		assert.DeepEqual(t, words, test["expect"])
-			t.Fatalf("length check failed. input: %v, expect: %q, output: %q", test["input"][0], test["expect"], words)
-		}
-		for i, word := range words {
-			if word != test["expect"][i] {
-				t.Fatalf("word check failed for word: %q. input: %q, expect: %q, output: %q", word, test["input"][0], test["expect"], words)
-			}
-		}
 	}
 }
 
 func TestLineInformation(t *testing.T) {
 	df, err := os.Open(testFileLineInfo)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatalf("Dockerfile missing for %s: %v", testFileLineInfo, err)
-	}
 	defer df.Close()
 
-	ast, err := Parse(df, NewDefaultDirective())
+	result, err := Parse(df)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatalf("Error parsing dockerfile %s: %v", testFileLineInfo, err)
-	}
 
-	if ast.StartLine != 5 || ast.EndLine != 31 {
+	ast := result.AST
-		fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.EndLine)
+	if ast.StartLine != 5 || ast.endLine != 31 {
+		fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.endLine)
 		t.Fatal("Root line information doesn't match result.")
 	}
-	if len(ast.Children) != 3 {
+	assert.Equal(t, len(ast.Children), 3)
-		fmt.Fprintf(os.Stderr, "Wrong number of child: expected(%d), actual(%d)\n", 3, len(ast.Children))
-		t.Fatalf("Root line information doesn't match result for %s", testFileLineInfo)
-	}
 	expected := [][]int{
 		{5, 5},
 		{11, 12},
 		{17, 31},
 	}
 	for i, child := range ast.Children {
-		if child.StartLine != expected[i][0] || child.EndLine != expected[i][1] {
+		if child.StartLine != expected[i][0] || child.endLine != expected[i][1] {
 			t.Logf("Wrong line information for child %d: expected(%d-%d), actual(%d-%d)\n",
-				i, expected[i][0], expected[i][1], child.StartLine, child.EndLine)
+				i, expected[i][0], expected[i][1], child.StartLine, child.endLine)
 			t.Fatal("Root line information doesn't match result.")
 		}
 	}