preprocessor: fix error handling for validation of #location

There can be multiple sidebyside nodes in a guide. When validating nodes
in a guide, we validate them sequentially, but want to report all errors
in the file rather than stopping at the first error in the first node
(such behaviour would not be helpful to the author of a guide).

As such, the signature of validatingNode.validate() was not helpful:

    validate() error

This suggested that the callee should return an error, probably the
first error, rather than recording errors against the implementer of
validate().

errorContext exists for just this purpose: to allow the implementer to
record multiple errors, and then have the caller check
$implementer.isInError().

Make all nodes implement errorContext therefore, and update the validate
signature to simply:

    validate()

along with an appropriate comment, to make clear it is the implementer's
job to record errors.

This requires us to fix validate() implementations in a couple of
places, recording errors instead of returning them, and making explicit
where we return early in some situations (because later validation
requires certain preconditions to hold).

The one test change here is the addition of a log message, which
confirms we do now report multiple errors in the same file (across
multiple nodes).

Signed-off-by: Paul Jolly <[email protected]>
Preprocessor-No-Write-Cache: true
Change-Id: Ifdbe91f6648a381cf263c3ececb7bc4a8c8f67c9
Dispatch-Trailer: {"type":"trybot","CL":1169948,"patchset":6,"ref":"refs/changes/48/1169948/6","targetBranch":"alpha"}

internal/cmd/preprocessor/cmd/code_node.go

@@ -16,7 +16,6 @@ package cmd
 
 import (
 	"bytes"
-	"fmt"
 )
 
 const (
@@ -35,16 +34,16 @@ func (c *codeNode) nodeType() string {
 }
 
 var _ runnableNode = (*codeNode)(nil)
+var _ validatingNode = (*codeNode)(nil)
 
 type codeNodeRunContext struct {
 	*txtarRunContext
 }
 
-func (s *codeNode) validate() error {
+func (s *codeNode) validate() {
 	if l := len(s.analysis.fileNames); l != 1 {
-		return fmt.Errorf("code nodes can only contain one file in the txtar archive")
+		s.errorf("%v: %s directives can only contain a single file", s, fnCode)
 	}
-	return nil
 }
 
 func (s *codeNode) run() runnable {
@@ -63,7 +62,7 @@ func (s *codeNodeRunContext) run() (err error) {
 	defer recoverFatalError(&err)
 
 	if err := s.formatFiles(); err != nil {
-		return errorIfInError(s)
+		return s.errorf("%v: failed to format files: %v", s, err)
 	}
 
 	return nil

internal/cmd/preprocessor/cmd/nodes.go

@@ -24,6 +24,8 @@ import (
 
 // A node is an abstraction around the structures that can appear in a page.
 type node interface {
+	errorContext
+
 	// writeSourceTo writes the source (text/template) form of a node to buf.
 	writeSourceTo(buf *bytes.Buffer)
 
@@ -65,8 +67,9 @@ type validatingNode interface {
 
 	// validate ensures that a node is valid. This type of validation is
 	// self-contained, i.e. can only be a function of the contents of the
-	// node itself.
-	validate() error
+	// node itself. Implementers record errors via errorf(), which has
+	// the effect of marking the node as isInError() == true.
+	validate()
 }
 
 // A runnable is something that can be run. It has a bufferedErrorContext for

internal/cmd/preprocessor/cmd/rootfile.go

@@ -162,6 +162,7 @@ func (rf *rootFile) transform(targetPath string) error {
 		return err
 	}
 
+	// Do not continue if we are already in error
 	if rf.isInError() {
 		return errorIfInError(rf)
 	}
@@ -239,23 +240,23 @@ func (rf *rootFile) validate() error {
 		}
 	}
 
+	// If we are already in error, do not progress to validate
 	if rf.isInError() {
 		return errorIfInError(rf)
 	}
 
 	// Validate those parts which have a validate() method
-	err = rf.walkBody(func(n node) error {
+	rf.walkBody(func(n node) error {
 		bp, ok := n.(validatingNode)
 		if !ok {
 			return nil
 		}
-		if err := bp.validate(); err != nil {
-			rf.errorf("%v: %v", bp, err)
-		}
-		return errorIfInError(rf)
+		bp.validate()
+		rf.updateInError(bp.isInError())
+		return nil
 	})
 
-	return err
+	return errorIfInError(rf)
 }
 
 // run is responsible for updating those nodes which contain inputs and outputs.

internal/cmd/preprocessor/cmd/script_node.go

@@ -31,23 +31,28 @@ type scriptNode struct {
 	stmts []*commandStmt
 }
 
+var _ validatingNode = (*scriptNode)(nil)
+
 func (s *scriptNode) nodeType() string {
 	return "script"
 }
 
 // validate the scriptNode. This also has the side effect of parsing the
 // bash script in the comment o commandStmt's.
-func (s *scriptNode) validate() error {
+func (s *scriptNode) validate() {
 	if l := len(s.analysis.fileNames); l != 0 {
-		return fmt.Errorf("script nodes cannot contain any files in the txtar archive")
+		s.errorf("%v: script nodes cannot contain any files in the txtar archive", s)
 	}
 
 	// Now ensure that we can parse the script
 	//
 	// Per @mvdan, use https://pkg.go.dev/mvdan.cc/sh/v3/syntax#Parser.Stmts
 	file, err := syntax.NewParser(syntax.KeepComments(true)).Parse(bytes.NewReader(s.effectiveArchive.Comment), "")
 	if err != nil {
-		return fmt.Errorf("failed to parse shell script: %v", err)
+		// If we get an error here we cannot proceed to further
+		// validation analysis
+		s.errorf("%v: failed to parse shell script: %v", s, err)
+		return
 	}
 	s.debugf(s.debugGeneral, "parsed %q, gave %v statements", s.effectiveArchive.Comment, len(file.Stmts))
 	for _, stmt := range file.Stmts {
@@ -60,14 +65,12 @@ func (s *scriptNode) validate() error {
 		stmt.Negated = false
 		var sb strings.Builder
 		if err := s.rf.shellPrinter.Print(&sb, stmt); err != nil {
-			return fmt.Errorf("failed to print statement: %v", err)
+			s.errorf("%v: failed to print statement at %v: %v", s, stmt.Position, err)
 		}
 		cmdStmt.cmdStr = sb.String()
 		cmdStmt.negated = negated
 		s.stmts = append(s.stmts, &cmdStmt)
 	}
-
-	return nil
 }
 
 // commandStmt is effectively a local version of the parsed *syntax.Stmt. This

internal/cmd/preprocessor/cmd/sidebyside_node.go

@@ -39,13 +39,14 @@ func (s *sidebysideNode) nodeType() string {
 	return fnSidebyside
 }
 
-var _ runnableNode = (*sidebysideNode)(nil)
-
 type sidebysideNodeRunContext struct {
 	*txtarRunContext
 }
 
-func (s *sidebysideNode) validate() error {
+var _ runnableNode = (*sidebysideNode)(nil)
+var _ validatingNode = (*sidebysideNode)(nil)
+
+func (s *sidebysideNode) validate() {
 	// Is there a script to run? If there is no effective script, i.e. non blank
 	// non comment lines, we might need to fill one in when running. There are
 	// shorthand versions of sidebyside nodes which don't have a script and yet
@@ -62,7 +63,9 @@ func (s *sidebysideNode) validate() error {
 	// defaults, otherwise if there are >3 files the #location tag is required.
 	locations, ok, err := s.tag(tagLocation, "")
 	if err != nil {
-		return s.errorf("%v: failed to extract #%s tag: %v", s, tagLocation, err)
+		s.errorf("%v: failed to extract #%s tag: %v", s, tagLocation, err)
+		// Can't validate further
+		return
 	}
 	if !ok {
 		// This case is only a problem if we have >3 files
@@ -89,19 +92,22 @@ func (s *sidebysideNode) validate() error {
 		}
 	}
 
+	// Return early in case we are already in error
 	if s.isInError() {
-		return errorIfInError(s)
+		return
 	}
 
 	// Return early if we have been told to #norun this archive
 	if _, found, _ := s.tag(tagNorun, ""); found {
-		return nil
+		return
 	}
 
 	if !s.analysis.hasEffectiveComment {
 		effectiveArchive.Comment, ok = s.buildEffectiveScript(s.analysis, s.effectiveArchive)
 		if !ok {
-			return s.errorf("%v: failed to build effective comment", s)
+			s.errorf("%v: failed to build effective comment", s)
+			// Cannot proceed with this error
+			return
 		}
 	}
 
@@ -111,9 +117,8 @@ func (s *sidebysideNode) validate() error {
 	// the command, stripping the leading "exec"
 	s.analysis.cmd = extractCommand(effectiveArchive.Comment)
 	if s.analysis.cmd == "" {
-		s.fatalf("no effective command?")
+		s.errorf("%v: failed to find effective command in script:\n%s", s, tabIndent(effectiveArchive.Comment))
 	}
-	return nil
 }
 
 func (s *sidebysideNode) run() runnable {
@@ -132,7 +137,7 @@ func (s *sidebysideNodeRunContext) run() (err error) {
 	defer recoverFatalError(&err)
 
 	if err := s.formatFiles(); err != nil {
-		return errorIfInError(s)
+		return s.errorf("%v: failed to format files: %v", s, err)
 	}
 
 	// Update the effective files in from the source files

internal/cmd/preprocessor/cmd/testdata/execute_err_bad_location_tag.txtar

@@ -46,5 +46,5 @@ package site
 -- hugo/content/.gitkeep --
 -- stderr.golden --
 ** $WORK/content/dir/en.md:7:8: saw 2 files but only 1 arguments to #location
-** $WORK/content/dir/en.md:7:8: in error
+** $WORK/content/dir/en.md:19:8: unknown locaion "rubbish"
 terminating because of errors

internal/cmd/preprocessor/cmd/txtar_node.go

@@ -332,11 +332,7 @@ func (t *txtarRunContext) formatFiles() error {
 		}
 	}
 
-	if t.isInError() {
-		return errorIfInError(t)
-	}
-
-	return nil
+	return errorIfInError(t)
 }
 
 func (t *txtarRunContext) dockerCmd(dockerArgs []string, cmdArgs ...string) *exec.Cmd {

internal/cmd/preprocessor/cmd/upload_node.go

@@ -16,7 +16,6 @@ package cmd
 
 import (
 	"bytes"
-	"fmt"
 )
 
 const fnUpload = "upload"
@@ -25,15 +24,16 @@ type uploadNode struct {
 	txtarNode
 }
 
+var _ validatingNode = (*uploadNode)(nil)
+
 func (u *uploadNode) nodeType() string {
 	return "upload"
 }
 
-func (u *uploadNode) validate() error {
+func (u *uploadNode) validate() {
 	if l := len(u.analysis.fileNames); l != 1 {
-		return fmt.Errorf("upload nodes can only contain a single file; saw %d", l)
+		u.errorf("%v: upload nodes can only contain a single file; saw %d", u, l)
 	}
-	return nil
 }
 
 func (u *uploadNode) writeTransformTo(b *bytes.Buffer) error {
@@ -88,7 +88,7 @@ func (u *uploadNodeRunContext) run() (err error) {
 	}
 
 	if err := u.formatFiles(); err != nil {
-		return errorIfInError(u)
+		return u.errorf("%v: failed to format files: %v", u, err)
 	}
 
 	return nil