Linter

Author: sbarzowski

.gitignore

@@ -27,4 +27,6 @@ gojsonnet.egg-info/
 /linter/jsonnet-lint/jsonnet-lint
 /tests_path.source
 
+/jsonnet-lint
+
 /builtin-benchmark-results

.golangci.yml

@@ -9,6 +9,7 @@ issues:
   exclude-use-default: false
   exclude:
     - "should have a package comment, unless it's in another file for this package"
+    - "the surrounding loop is unconditionally terminated"
 linters-settings:
   golint:
     min-confidence: 0

builtins.go

@@ -1572,8 +1572,8 @@ var funcBuiltins = buildBuiltinMap([]builtin{
 	&binaryBuiltin{name: "objectFieldsEx", function: builtinObjectFieldsEx, params: ast.Identifiers{"obj", "hidden"}},
 	&ternaryBuiltin{name: "objectHasEx", function: builtinObjectHasEx, params: ast.Identifiers{"obj", "fname", "hidden"}},
 	&unaryBuiltin{name: "type", function: builtinType, params: ast.Identifiers{"x"}},
-	&unaryBuiltin{name: "char", function: builtinChar, params: ast.Identifiers{"x"}},
-	&unaryBuiltin{name: "codepoint", function: builtinCodepoint, params: ast.Identifiers{"x"}},
+	&unaryBuiltin{name: "char", function: builtinChar, params: ast.Identifiers{"n"}},
+	&unaryBuiltin{name: "codepoint", function: builtinCodepoint, params: ast.Identifiers{"str"}},
 	&unaryBuiltin{name: "ceil", function: builtinCeil, params: ast.Identifiers{"x"}},
 	&unaryBuiltin{name: "floor", function: builtinFloor, params: ast.Identifiers{"x"}},
 	&unaryBuiltin{name: "sqrt", function: builtinSqrt, params: ast.Identifiers{"x"}},
@@ -1587,9 +1587,9 @@ var funcBuiltins = buildBuiltinMap([]builtin{
 	&unaryBuiltin{name: "exp", function: builtinExp, params: ast.Identifiers{"x"}},
 	&unaryBuiltin{name: "mantissa", function: builtinMantissa, params: ast.Identifiers{"x"}},
 	&unaryBuiltin{name: "exponent", function: builtinExponent, params: ast.Identifiers{"x"}},
-	&binaryBuiltin{name: "pow", function: builtinPow, params: ast.Identifiers{"base", "exp"}},
+	&binaryBuiltin{name: "pow", function: builtinPow, params: ast.Identifiers{"x", "n"}},
 	&binaryBuiltin{name: "modulo", function: builtinModulo, params: ast.Identifiers{"x", "y"}},
-	&unaryBuiltin{name: "md5", function: builtinMd5, params: ast.Identifiers{"x"}},
+	&unaryBuiltin{name: "md5", function: builtinMd5, params: ast.Identifiers{"s"}},
 	&ternaryBuiltin{name: "substr", function: builtinSubstr, params: ast.Identifiers{"str", "from", "len"}},
 	&ternaryBuiltin{name: "splitLimit", function: builtinSplitLimit, params: ast.Identifiers{"str", "c", "maxsplits"}},
 	&ternaryBuiltin{name: "strReplace", function: builtinStrReplace, params: ast.Identifiers{"str", "from", "to"}},

linter/jsonnet-lint/BUILD.bazel renamed to cmd/jsonnet-lint/BUILD.bazel

@@ -3,11 +3,13 @@ load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
 go_library(
     name = "go_default_library",
     srcs = ["cmd.go"],
-    importpath = "github.com/google/go-jsonnet/linter/jsonnet-lint",
+    importpath = "github.com/google/go-jsonnet/cmd/jsonnet-lint",
     visibility = ["//visibility:private"],
     deps = [
         "//:go_default_library",
+        "//cmd/internal/cmd:go_default_library",
         "//linter:go_default_library",
+        "@com_github_fatih_color//:go_default_library",
     ],
 )
 

cmd/jsonnet-lint/cmd.go

@@ -0,0 +1,192 @@
+package main
+
+import (
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"github.com/fatih/color"
+	"github.com/google/go-jsonnet/cmd/internal/cmd"
+	"github.com/google/go-jsonnet/linter"
+
+	jsonnet "github.com/google/go-jsonnet"
+)
+
+func version(o io.Writer) {
+	fmt.Fprintf(o, "Jsonnet linter %s\n", jsonnet.Version())
+}
+
+func usage(o io.Writer) {
+	version(o)
+	fmt.Fprintln(o)
+	fmt.Fprintln(o, "jsonnet-lint {<option>} { <filename> }")
+	fmt.Fprintln(o)
+	fmt.Fprintln(o, "Available options:")
+	fmt.Fprintln(o, "  -h / --help                This message")
+	fmt.Fprintln(o, "  -J / --jpath <dir>         Specify an additional library search dir")
+	fmt.Fprintln(o, "                             (right-most wins)")
+	fmt.Fprintln(o, "  --version                  Print version")
+	fmt.Fprintln(o)
+	fmt.Fprintln(o, "Environment variables:")
+	fmt.Fprintln(o, "  JSONNET_PATH is a colon (semicolon on Windows) separated list of directories")
+	fmt.Fprintln(o, "  added in reverse order before the paths specified by --jpath (i.e. left-most")
+	fmt.Fprintln(o, "  wins). E.g. these are equivalent:")
+	fmt.Fprintln(o, "    JSONNET_PATH=a:b jsonnet -J c -J d")
+	fmt.Fprintln(o, "    JSONNET_PATH=d:c:a:b jsonnet")
+	fmt.Fprintln(o, "    jsonnet -J b -J a -J c -J d")
+	fmt.Fprintln(o)
+	fmt.Fprintln(o, "In all cases:")
+	fmt.Fprintln(o, "  <filename> can be - (stdin)")
+	fmt.Fprintln(o, "  Multichar options are expanded e.g. -abc becomes -a -b -c.")
+	fmt.Fprintln(o, "  The -- option suppresses option processing for subsequent arguments.")
+	fmt.Fprintln(o, "  Note that since filenames and jsonnet programs can begin with -, it is")
+	fmt.Fprintln(o, "  advised to use -- if the argument is unknown, e.g. jsonnetfmt -- \"$FILENAME\".")
+	fmt.Fprintln(o)
+	fmt.Fprintln(o, "Exit code:")
+	fmt.Fprintln(o, "  0 – If the file was checked no problems were found.")
+	fmt.Fprintln(o, "  1 – If errors occured which prevented checking (e.g. specified file is missing).")
+	fmt.Fprintln(o, "  2 – If problems were found.")
+
+}
+
+type config struct {
+	// TODO(sbarzowski) Allow multiple root files checked at once for greater efficiency
+	inputFile string
+	evalJpath []string
+}
+
+func makeConfig() config {
+	return config{
+		evalJpath: []string{},
+	}
+}
+
+type processArgsStatus int
+
+const (
+	processArgsStatusContinue     = iota
+	processArgsStatusSuccessUsage = iota
+	processArgsStatusFailureUsage = iota
+	processArgsStatusSuccess      = iota
+	processArgsStatusFailure      = iota
+)
+
+func processArgs(givenArgs []string, config *config, vm *jsonnet.VM) (processArgsStatus, error) {
+	args := cmd.SimplifyArgs(givenArgs)
+	remainingArgs := make([]string, 0, len(args))
+	i := 0
+
+	for ; i < len(args); i++ {
+		arg := args[i]
+		if arg == "--" {
+			// All subsequent args are not options.
+			i++
+			for ; i < len(args); i++ {
+				remainingArgs = append(remainingArgs, args[i])
+			}
+			break
+		} else if arg == "-h" || arg == "--help" {
+			return processArgsStatusSuccessUsage, nil
+		} else if arg == "-v" || arg == "--version" {
+			version(os.Stdout)
+			return processArgsStatusSuccess, nil
+		} else if arg == "-J" || arg == "--jpath" {
+			dir := cmd.NextArg(&i, args)
+			if len(dir) == 0 {
+				return processArgsStatusFailure, fmt.Errorf("-J argument was empty string")
+			}
+			if dir[len(dir)-1] != '/' {
+				dir += "/"
+			}
+			config.evalJpath = append(config.evalJpath, dir)
+		} else if len(arg) > 1 && arg[0] == '-' {
+			return processArgsStatusFailure, fmt.Errorf("unrecognized argument: %s", arg)
+		} else {
+			remainingArgs = append(remainingArgs, arg)
+		}
+	}
+
+	if len(remainingArgs) == 0 {
+		return processArgsStatusFailureUsage, fmt.Errorf("file not provided")
+	}
+
+	if len(remainingArgs) > 1 {
+		return processArgsStatusFailure, fmt.Errorf("only one file is allowed")
+	}
+
+	config.inputFile = remainingArgs[0]
+	return processArgsStatusContinue, nil
+}
+
+func die(err error) {
+	fmt.Fprintf(os.Stderr, "ERROR: %s\n", err.Error())
+	os.Exit(1)
+}
+
+func main() {
+	cmd.StartCPUProfile()
+	defer cmd.StopCPUProfile()
+
+	vm := jsonnet.MakeVM()
+	vm.ErrorFormatter.SetColorFormatter(color.New(color.FgRed).Fprintf)
+
+	config := makeConfig()
+	jsonnetPath := filepath.SplitList(os.Getenv("JSONNET_PATH"))
+	for i := len(jsonnetPath) - 1; i >= 0; i-- {
+		config.evalJpath = append(config.evalJpath, jsonnetPath[i])
+	}
+
+	status, err := processArgs(os.Args[1:], &config, vm)
+	if err != nil {
+		fmt.Fprintln(os.Stderr, "ERROR: "+err.Error())
+	}
+	switch status {
+	case processArgsStatusContinue:
+		break
+	case processArgsStatusSuccessUsage:
+		usage(os.Stdout)
+		os.Exit(0)
+	case processArgsStatusFailureUsage:
+		if err != nil {
+			fmt.Fprintln(os.Stderr, "")
+		}
+		usage(os.Stderr)
+		os.Exit(1)
+	case processArgsStatusSuccess:
+		os.Exit(0)
+	case processArgsStatusFailure:
+		os.Exit(1)
+	}
+
+	vm.Importer(&jsonnet.FileImporter{
+		JPaths: config.evalJpath,
+	})
+
+	inputFile, err := os.Open(config.inputFile)
+	if err != nil {
+		die(err)
+	}
+	data, err := ioutil.ReadAll(inputFile)
+	if err != nil {
+		die(err)
+	}
+	err = inputFile.Close()
+	if err != nil {
+		die(err)
+	}
+
+	cmd.MemProfile()
+
+	_, err = jsonnet.SnippetToAST(config.inputFile, string(data))
+	if err != nil {
+		die(err)
+	}
+
+	errorsFound := linter.LintSnippet(vm, os.Stderr, config.inputFile, string(data))
+	if errorsFound {
+		fmt.Fprintf(os.Stderr, "Problems found!\n")
+		os.Exit(2)
+	}
+}

linter/BUILD.bazel

@@ -1,16 +1,30 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
-    srcs = [
-        "find_variables.go",
-        "linter.go",
-    ],
+    srcs = ["linter.go"],
     importpath = "github.com/google/go-jsonnet/linter",
     visibility = ["//visibility:public"],
     deps = [
+        "//:go_default_library",
         "//ast:go_default_library",
         "//internal/errors:go_default_library",
         "//internal/parser:go_default_library",
+        "//linter/internal/common:go_default_library",
+        "//linter/internal/traversal:go_default_library",
+        "//linter/internal/types:go_default_library",
+        "//linter/internal/utils:go_default_library",
+        "//linter/internal/variables:go_default_library",
+    ],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = ["linter_test.go"],
+    data = glob(["testdata/**"]),
+    embed = [":go_default_library"],
+    deps = [
+        "//:go_default_library",
+        "//internal/testutils:go_default_library",
     ],
 )

linter/README.md

@@ -1,10 +1,45 @@
 # jsonnet-lint
 
-Very basic experimental, quick and a bit dirty linter. It's intended to become a full featured linter as it matures. PRs are very welcome!
+This is a linter for Jsonnet. It is alpha stage, but it should be already useful.
 
-Don't expect much from it now.
+## Features
 
-## Notes
+The linter detect the following kinds of issues:
+* "Type" problems, such as:
+    ∗ Accessing nonexistent fields
+    ∗ Calling a function with a wrong number of arguments or named arguments
+    which do not match the parameters
+    ∗ Trying to call a value which is not a function
+    ∗ Trying to index a value which is not an object, array or a string
+* Unused variables
+* Endlessly looping constructs, which are always invalid, but often appear  as a result of confusion about language semantics (e.g. local x = x + 1)
+* Anything that is statically detected during normal execution, such as syntax errors and undeclared variables.
 
-It currently runs on a desugared file. It has its good and bad sides.
-We should figure out what's the right way.
+## Usage
+
+`jsonnet-lint [options] <filename>`
+
+## Design
+
+### Goals
+
+The purpose of the linter is to aid development by providing quick and clear feedback about simple problems. With that in mind I defined the following goals:
+- It should find common problems, especially the kinds resulting from typos, trivial omissions and issues resulting from misunderstanding of the semantics.
+- It should find problems in the parts of code which are not reached by the tests (especially
+important due to the lazy evaluation).
+- It must be practical to use with the existing Jsonnet code, without any need for modification.
+- It must be fast enough so it is practical to always run the linter before execution during development. The overhead required to run the linter prior to running the program in real world conditions should be comparable with parsing and desugaring.
+- It must be conservative regarding the reported problems. False negatives are preferable to false positives. False positives are allowed as long as they relate to code which is going to be confusing for humans to read or if they can be worked around easily while preserving readability.
+- Its results must be stable, i.e. trivial changes such as changing the order of variables in local expressions should not change the result nontrivially.
+- It must preserve the abstractions. Validity of the definitions should not depend on their use. In particular calling functions with specific arguments or accessing fields of objects should not cause errors in their definitions.
+- It should be possible to explicitly silence individual errors, so that occasional acknowledged false positives do not distract the users. This is espcially important if a clean pass is enforced in  Continuous Integration.
+
+### Rules
+
+The above goals naturally lead to the some more specific code-level rules which all analyses must obey:
+
+- All expressions should be checked, even the provably dead code.
+- Always consider both branches of the `if` expression possible (even if the condition is trivially always true or always false).
+- Correctness of a function definition should not depend on how it is used. In particular
+when analyzing the definition assume that function arguments can take arbitrary values.
+- Correctness of an object definition should not depend on how it is used. In particular when analyzing the definition assume that the object may be part of an arbitrary inheritance chain