Minor cleanup.

Author: sbarzowski

cmd/jsonnet-lint/cmd.go

@@ -5,7 +5,6 @@ import (
 	"io/ioutil"
 	"os"
 
-	"github.com/fatih/color"
 	"github.com/google/go-jsonnet/linter"
 
 	jsonnet "github.com/google/go-jsonnet"
@@ -48,19 +47,13 @@ func main() {
 	if err != nil {
 		die(err)
 	}
-	errWriter := &linter.ErrorWriter{
-		Writer:    os.Stderr,
-		Formatter: jsonnet.LinterFormatter(),
-	}
 
 	vm := jsonnet.MakeVM()
 
-	errWriter.Formatter.SetColorFormatter(color.New(color.FgRed).Fprintf)
-
-	l := linter.NewLinter(vm, errWriter)
+	l := linter.NewLinter(vm, os.Stderr)
 	l.AddFile(p, string(data))
-	l.Check()
-	if errWriter.ErrorsFound {
+	errorsFound := l.Check()
+	if errorsFound {
 		fmt.Fprintf(os.Stderr, "Problems found!\n")
 		ExitProblemsFound()
 	}

linter/internal/common/common.go

@@ -4,14 +4,19 @@ import (
 	"github.com/google/go-jsonnet/ast"
 )
 
+// VariableKind allows distinguishing various kinds of variables.
 type VariableKind int
 
 const (
+	// VarRegular is a "normal" variable with a definition in the code.
 	VarRegular VariableKind = iota
+	// VarParam is a function parameter.
 	VarParam
+	// VarStdlib is a special `std` variable.
 	VarStdlib
 )
 
+// Variable is a representation of a variable somewhere in the code.
 type Variable struct {
 	Name         ast.Identifier
 	BindNode     ast.Node

linter/internal/traversal/traversal.go

@@ -1,5 +1,11 @@
 package traversal
 
+// Package traversal provides relatively lightweight checks
+// which can all fit within one traversal of the AST.
+// Currently available checks:
+// * Loop detection
+// TODO(sbarzowski) add more
+
 import (
 	"github.com/google/go-jsonnet/ast"
 	"github.com/google/go-jsonnet/linter/internal/utils"
@@ -54,6 +60,8 @@ func findLoopingInLocal(node *ast.Local, ec *utils.ErrCollector) {
 	}
 }
 
+// Traverse visits all nodes in the AST and runs appropriate
+// checks.
 func Traverse(node ast.Node, ec *utils.ErrCollector) {
 	switch node := node.(type) {
 	case *ast.Local:

linter/internal/types/build_graph.go

@@ -11,6 +11,10 @@ import (
 	"github.com/google/go-jsonnet/linter/internal/common"
 )
 
+// Maximum number of array elements for which we track
+// the values individually. We do that, because in Jsonnet
+// there is no separate tuple type, so we treat arrays as
+// potential tuples.
 const maxKnownCount = 5
 
 func (g *typeGraph) getExprPlaceholder(node ast.Node) placeholderID {
@@ -85,13 +89,11 @@ func (g *typeGraph) AddRoots(roots map[string]ast.Node, vars map[string]map[ast.
 	}
 }
 
-type ImportFunc func(currentPath, importedPath string) ast.Node
-
 func countRequiredParameters(params []ast.Parameter) int {
 	count := 0
 	for _, p := range params {
 		if p.DefaultArg != nil {
-			count += 1
+			count++
 		}
 	}
 	return count

linter/internal/types/check.go

@@ -14,6 +14,8 @@ func checkSubexpr(node ast.Node, typeOf ExprTypes, ec *utils.ErrCollector) {
 	}
 }
 
+// Check verifies that the types are valid for a given program, given
+// the previously resolved types.
 func Check(node ast.Node, typeOf ExprTypes, ec *utils.ErrCollector) {
 	checkSubexpr(node, typeOf, ec)
 	switch node := node.(type) {

linter/internal/types/desc.go

@@ -114,6 +114,9 @@ func (f *functionDesc) normalize() {
 	f.resultContains = normalizePlaceholders(f.resultContains)
 }
 
+// TypeDesc is a representation of a type.
+// This is (way) richer than the basic Jsonnet type system
+// with seven types.
 type TypeDesc struct {
 	Bool         bool
 	Number       bool
@@ -124,23 +127,28 @@ type TypeDesc struct {
 	ArrayDesc    *arrayDesc
 }
 
+// Any returns whether all values are allowed (i.e. we know nothing about it).
 func (t *TypeDesc) Any() bool {
 	// TODO(sbarzowski) BUG - must check that function, object and array allow any values inside
 	return t.Bool && t.Number && t.String && t.Null && t.Function() && t.Object() && t.Array()
 }
 
+// Void returns whether the type is empty (no values are possible).
 func (t *TypeDesc) Void() bool {
 	return !t.Bool && !t.Number && !t.String && !t.Null && !t.Function() && !t.Object() && !t.Array()
 }
 
+// Function returns whether the types contains a function.
 func (t *TypeDesc) Function() bool {
 	return t.FunctionDesc != nil
 }
 
+// Object returns whether the types contains an object.
 func (t *TypeDesc) Object() bool {
 	return t.ObjectDesc != nil
 }
 
+// Array returns whether the types contains an array.
 func (t *TypeDesc) Array() bool {
 	return t.ArrayDesc != nil
 }
@@ -149,6 +157,8 @@ func voidTypeDesc() TypeDesc {
 	return TypeDesc{}
 }
 
+// Describe provides incomplete, but human-readable
+// representation of a type.
 func Describe(t *TypeDesc) string {
 	if t.Any() {
 		return "any"
@@ -158,25 +168,25 @@ func Describe(t *TypeDesc) string {
 	}
 	parts := []string{}
 	if t.Bool {
-		parts = append(parts, "bool")
+		parts = append(parts, "a bool")
 	}
 	if t.Number {
-		parts = append(parts, "number")
+		parts = append(parts, "a number")
 	}
 	if t.String {
-		parts = append(parts, "string")
+		parts = append(parts, "a string")
 	}
 	if t.Null {
-		parts = append(parts, "null")
+		parts = append(parts, "a null")
 	}
 	if t.Function() {
-		parts = append(parts, "function")
+		parts = append(parts, "a function")
 	}
 	if t.Object() {
-		parts = append(parts, "object")
+		parts = append(parts, "an object")
 	}
 	if t.Array() {
-		parts = append(parts, "array")
+		parts = append(parts, "an array")
 	}
 	return strings.Join(parts, " or ")
 }
@@ -339,6 +349,7 @@ func plusArrays(left, right *arrayDesc) *arrayDesc {
 
 func builtinPlus(concreteArgs []*TypeDesc, pArgs []placeholderID) builtinOpResult {
 	if concreteArgs[0] != nil && concreteArgs[1] != nil {
+		// We have concrete arguments available - we can provide a concrete result.
 		left := concreteArgs[0]
 		right := concreteArgs[1]
 		return builtinOpResult{
@@ -353,9 +364,10 @@ func builtinPlus(concreteArgs []*TypeDesc, pArgs []placeholderID) builtinOpResul
 			},
 			contained: pArgs,
 		}
-	} else {
-		return builtinOpResult{
-			contained: pArgs,
-		}
+	}
+	// We do now know what the arguments are yet, so we cannot provide any concrete
+	// result without more context.
+	return builtinOpResult{
+		contained: pArgs,
 	}
 }

linter/internal/types/graph.go

@@ -73,9 +73,8 @@ func (g *typeGraph) getOrCreateElementType(target placeholderID, index *indexSpe
 			elID := g.newPlaceholder()
 			elementType.stringIndex[index.stringIndex] = elID
 			return created, elID
-		} else {
-			return created, elementType.stringIndex[index.stringIndex]
 		}
+		return created, elementType.stringIndex[index.stringIndex]
 	} else if index.indexType == knownIntIndex {
 		if elementType.intIndex == nil {
 			elementType.intIndex = make([]placeholderID, maxKnownCount)
@@ -85,9 +84,8 @@ func (g *typeGraph) getOrCreateElementType(target placeholderID, index *indexSpe
 			elID := g.newPlaceholder()
 			elementType.intIndex[index.intIndex] = elID
 			return created, elID
-		} else {
-			return created, elementType.intIndex[index.intIndex]
 		}
+		return created, elementType.intIndex[index.intIndex]
 	} else if index.indexType == functionIndex {
 		if elementType.callIndex == noType {
 			created = true
@@ -128,6 +126,10 @@ type typePlaceholder struct {
 	builtinOp *builtinOpDesc
 }
 
+// ImportFunc should provide an AST node from a given location.
+// If a node is not available it should return nil.
+type ImportFunc func(currentPath, importedPath string) ast.Node
+
 type typeGraph struct {
 	_placeholders   []typePlaceholder
 	exprPlaceholder map[ast.Node]placeholderID
@@ -441,6 +443,7 @@ func tpRef(p placeholderID) typePlaceholder {
 	}
 }
 
+// ExprTypes is a map containing a type of each expression.
 type ExprTypes map[ast.Node]TypeDesc
 
 func (g *typeGraph) newSimpleFuncType(returnType placeholderID, argNames ...ast.Identifier) placeholderID {
@@ -639,6 +642,11 @@ func prepareStdlib(g *typeGraph) {
 	})
 }
 
+// NewTypeGraph creates a new type graph, with the basic types and stdlib ready.
+// It does not contain any representation based on user-provided code yet.
+//
+// It requires varAt - the information about the variables in the problem (indexed
+// by the location of used) and importFunc for importing the code from other files.
 func NewTypeGraph(varAt map[ast.Node]*common.Variable, importFunc ImportFunc) *typeGraph {
 	g := typeGraph{
 		exprPlaceholder: make(map[ast.Node]placeholderID),
@@ -718,11 +726,15 @@ func NewTypeGraph(varAt map[ast.Node]*common.Variable, importFunc ImportFunc) *t
 	return &g
 }
 
+// AddToGraph adds the code represented by node and all its available
+// (importable) transitive dependencies to the type graph.
 func (g *typeGraph) AddToGraph(node ast.Node, varAt map[ast.Node]*common.Variable) {
 	g.varAt = varAt //  TODO(sbarzowski) hacky! If it changes, we shouldn't put it in typeGraph at all
 	prepareTP(node, g)
 }
 
+// PrepareTypes produces a final type for each expression in the graph.
+// No further operations on the graph are valid after this is called.
 func (g *typeGraph) PrepareTypes(node ast.Node, typeOf ExprTypes, varAt map[ast.Node]*common.Variable) {
 	g.AddToGraph(node, varAt)
 	g.simplifyReferences()
@@ -734,3 +746,7 @@ func (g *typeGraph) PrepareTypes(node ast.Node, typeOf ExprTypes, varAt map[ast.
 		typeOf[e] = g.upperBound[p]
 	}
 }
+
+// TODO(sbarzowski) consider refactoring this to a safer interface, where a bunch of root nodes are passed
+// at once, with their varAt.
+// Or just unexport this stuff?

linter/linter.go

@@ -23,12 +23,11 @@ import (
 type ErrorWriter struct {
 	ErrorsFound bool
 	Writer      io.Writer
-	Formatter   jsonnet.ErrorFormatter
 }
 
-func (e *ErrorWriter) writeError(err errors.StaticError) {
+func (e *ErrorWriter) writeError(vm *jsonnet.VM, err errors.StaticError) {
 	e.ErrorsFound = true
-	_, writeErr := e.Writer.Write([]byte(e.Formatter.Format(err) + "\n"))
+	_, writeErr := e.Writer.Write([]byte(vm.ErrorFormatter.Format(err) + "\n"))
 	if writeErr != nil {
 		panic(writeErr)
 	}
@@ -69,8 +68,7 @@ func lint(vm *jsonnet.VM, node ast.Node, currentPath string, errWriter *ErrorWri
 
 	for _, v := range variableInfo.Variables {
 		if len(v.Occurences) == 0 && v.VariableKind == common.VarRegular && v.Name != "$" {
-			// TODO(sbarzowski) re-enable
-			errWriter.writeError(errors.MakeStaticError("Unused variable: "+string(v.Name), v.LocRange))
+			errWriter.writeError(vm, errors.MakeStaticError("Unused variable: "+string(v.Name), v.LocRange))
 		}
 	}
 	et := make(types.ExprTypes)
@@ -97,7 +95,7 @@ func lint(vm *jsonnet.VM, node ast.Node, currentPath string, errWriter *ErrorWri
 	traversal.Traverse(node, &ec)
 
 	for _, err := range ec.Errs {
-		errWriter.writeError(err)
+		errWriter.writeError(vm, err)
 	}
 }
 
@@ -111,7 +109,7 @@ func getImports(vm *jsonnet.VM, node ast.Node, roots map[string]ast.Node, curren
 		p := node.File.Value
 		contents, foundAt, err := vm.ImportAST(currentPath, p)
 		if err != nil {
-			errWriter.writeError(errors.MakeStaticError(err.Error(), *node.Loc()))
+			errWriter.writeError(vm, errors.MakeStaticError(err.Error(), *node.Loc()))
 		} else {
 			if _, visited := roots[foundAt]; !visited {
 				roots[foundAt] = contents
@@ -122,7 +120,7 @@ func getImports(vm *jsonnet.VM, node ast.Node, roots map[string]ast.Node, curren
 		p := node.File.Value
 		_, err := vm.ResolveImport(currentPath, p)
 		if err != nil {
-			errWriter.writeError(errors.MakeStaticError(err.Error(), *node.Loc()))
+			errWriter.writeError(vm, errors.MakeStaticError(err.Error(), *node.Loc()))
 		}
 	default:
 		for _, c := range parser.Children(node) {
@@ -135,17 +133,13 @@ type Linter struct {
 	vm *jsonnet.VM
 	// TODO(sbarzowski) implement observer in VM, so that linter cache is automatically flushed when
 	// the sources are invalidated. This is unnecessary if we implement one-go linter.
-	errWriter *ErrorWriter
+	errWriter io.Writer
 
 	// TODO(sbarzowski) allow more files
 	filename string
 	code     string
 }
 
-func (linter *Linter) flushCache() {
-
-}
-
 func (linter *Linter) AddFile(filename, code string) {
 	// TODO(sbarzowski) create addImported - a variant which imports it instead of getting code as string
 	// This allows avoiding processing it twice if it's both imported and accessed directly
@@ -159,16 +153,19 @@ func (linter *Linter) AddFile(filename, code string) {
 
 func (linter *Linter) Check() bool {
 	node, err := jsonnet.SnippetToAST(linter.filename, linter.code)
+	errWriter := ErrorWriter{
+		Writer:      linter.errWriter,
+		ErrorsFound: false,
+	}
 	if err != nil {
-		linter.errWriter.writeError(err.(errors.StaticError)) // ugly but true
+		errWriter.writeError(linter.vm, err.(errors.StaticError)) // ugly but true
 		return true
 	}
-	lint(linter.vm, node, linter.filename, linter.errWriter)
-	// TODO(sbarzowski) There is something fishy about this usage errWriter, since it may be reused multiple times
-	return linter.errWriter.ErrorsFound
+	lint(linter.vm, node, linter.filename, &errWriter)
+	return errWriter.ErrorsFound
 }
 
-func NewLinter(vm *jsonnet.VM, errWriter *ErrorWriter) *Linter {
+func NewLinter(vm *jsonnet.VM, errWriter io.Writer) *Linter {
 	return &Linter{
 		vm:        vm,
 		errWriter: errWriter,