create a sqlcmd style linter (#271)

shueybubbles · web-flow · commit d479b3d6e25d · 2023-03-08T13:05:56.000-06:00
diff --git a/.editorconfig b/.editorconfig
@@ -0,0 +1,9 @@
+# top-most EditorConfig file
+root = true
+
+[*.go]
+indent_style = tab
+# (Please don't specify an indent_size here; that has too many unintended consequences.)
+
+# IDE0073: File header
+file_header_template = Copyright (c) Microsoft Corporation.\nLicensed under the MIT license.
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -5,8 +5,6 @@
     "version": "0.2.0",
     "configurations": [
 
-    
-
         {
 
             "name": "Attach to Process",
@@ -36,5 +34,14 @@
             "program": "${workspaceFolder}/cmd/modern",
             "args" : ["-S", "np:.", "-i", "${workspaceFolder}/cmd/sqlcmd/testdata/select100.sql"],
         },
+        {
+            "name" : "Run sqlcmdlinter",
+            "type" : "go",
+            "request" : "launch",
+            "mode" : "auto",
+            "program": "${workspaceFolder}/cmd/sqlcmd-linter",
+            "args" : ["${workspaceFolder}/..."]
+
+        }
     ]
 }
diff --git a/build/build.cmd b/build/build.cmd
@@ -1,4 +1,19 @@
 @echo off
+
+REM We get the value of the escape character by using PROMPT $E
+for /F "tokens=1,2 delims=#" %%a in ('"prompt #$H#$E# & echo on & for %%b in (1) do rem"') do (
+  set "DEL=%%a"
+  set "ESC=%%b"
+)
+setlocal
+SET     RED=%ESC%[1;31m
+echo %RED%
+REM run the custom sqlcmd linter for code style enforcement
+REM using for/do instead of running it directly so the status code isn't checked by the shell.
+REM Once we are prepared to block the build with the linter we will move this step into a pipeline
+for /F  "usebackq"  %%l in (`go run cmd\sqlcmd-linter\main.go -test %~dp0../...`) DO echo %%l
+echo %ESC%[0m
+endlocal
 REM Get Version Tag
 for /f %%i in ('"git describe --tags --abbrev=0"') do set sqlcmdVersion=%%i
 
@@ -9,13 +24,14 @@ REM Generate NOTICE
 if not exist %gopath%\bin\go-licenses.exe (
     go install github.com/google/go-licenses@latest
 )
-go-licenses report github.com/microsoft/go-sqlcmd/cmd/modern --template build\NOTICE.tpl --ignore github.com/microsoft > %~dp0notice.txt
+go-licenses report github.com/microsoft/go-sqlcmd/cmd/modern --template build\NOTICE.tpl --ignore github.com/microsoft > %~dp0notice.txt 2>nul
 copy %~dp0NOTICE.header + %~dp0notice.txt %~dp0..\NOTICE.md
 del %~dp0notice.txt
 
 REM Generates all versions of sqlcmd in platform-specific folder
 setlocal
 
 for /F "tokens=1-3 delims=," %%i in (%~dp0arch.txt) do set GOOS=%%i&set GOARCH=%%j&go build -o %~dp0..\%%i-%%j\%%k -ldflags="-X main.version=%sqlcmdVersion%" %~dp0..\cmd\modern
+endlocal
 
 
diff --git a/cmd/sqlcmd-linter/main.go b/cmd/sqlcmd-linter/main.go
@@ -0,0 +1,10 @@
+package main
+
+import (
+	sqlcmdlinter "github.com/microsoft/go-sqlcmd/pkg/sqlcmd-linter"
+	"golang.org/x/tools/go/analysis/multichecker"
+)
+
+func main() {
+	multichecker.Main(sqlcmdlinter.AssertAnalyzer, sqlcmdlinter.ImportsAnalyzer)
+}
diff --git a/cmd/sqlcmd/sqlcmd_test.go b/cmd/sqlcmd/sqlcmd_test.go
@@ -26,7 +26,7 @@ func newKong(t *testing.T, cli interface{}, options ...kong.Option) *kong.Kong {
 		kong.NoDefaultHelp(),
 		kong.Exit(func(int) {
 			t.Helper()
-			t.Fatalf("unexpected exit()")
+			assert.Fail(t, "unexpected exit()")
 		}),
 	}, options...)
 	parser, err := kong.New(cli, options...)
diff --git a/pkg/sqlcmd-linter/imports.go b/pkg/sqlcmd-linter/imports.go
@@ -0,0 +1,84 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+package sqlcmdlinter
+
+import (
+	"go/ast"
+	"go/token"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var ImportsAnalyzer = &analysis.Analyzer{
+	Name:     "importslint",
+	Doc:      "Require most external packages be imported only by internal packages",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      runImports,
+}
+
+var AllowedImports = map[string][]string{
+	`"github.com/alecthomas/kong`:      {`cmd/sqlcmd`, `pkg/sqlcmd`},
+	`"github.com/golang-sql/sqlexp`:    {`pkg/sqlcmd`},
+	`"github.com/google/uuid`:          {},
+	`"github.com/peterh/liner`:         {`pkg/console`},
+	`"github.com/microsoft/go-mssqldb`: {},
+	`"github.com/microsoft/go-sqlcmd`:  {},
+	`"github.com/spf13/cobra`:          {`cmd/sqlcmd`, `cmd/modern`},
+	`"github.com/spf13/viper`:          {`cmd/sqlcmd`, `cmd/modern`},
+	`"github.com/stretchr/testify`:     {},
+}
+
+func runImports(pass *analysis.Pass) (interface{}, error) {
+	inspectorInstance := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	nodeFilter := []ast.Node{(*ast.File)(nil)}
+	inspectorInstance.Preorder(nodeFilter, func(n ast.Node) {
+
+		f := n.(*ast.File)
+		fileName := pass.Fset.Position(f.Package).Filename
+		isInternal := strings.Contains(fileName, `internal\`) || strings.Contains(fileName, `internal/`)
+		for _, s := range f.Imports {
+			if s.Path.Kind == token.STRING {
+				pkg := s.Path.Value
+				if isInternal {
+					if !isValidInternalImport(pkg) {
+						pass.Reportf(s.Pos(), "Internal packages should not import %s", pkg)
+					}
+				} else if !isValidExternalImport(pkg, fileName) {
+					pass.Reportf(s.Pos(), "Non-internal packages should not import %s", pkg)
+				}
+			}
+		}
+	})
+
+	return nil, nil
+}
+
+func isValidInternalImport(pkg string) bool {
+	return !strings.HasPrefix(pkg, `"github.com/microsoft/go-sqlcmd/pkg`) && !strings.HasPrefix(pkg, `"github.com/microsoft/go-sqlcmd/cmd`)
+}
+
+func isValidExternalImport(pkg string, filename string) bool {
+	if strings.HasPrefix(pkg, `"github.com`) {
+		for key, paths := range AllowedImports {
+			if strings.HasPrefix(pkg, key) {
+				if len(paths) == 0 {
+					// any package can import it
+					return true
+				}
+				for _, p := range paths {
+					// canonicalize path to Linux separator for comparison
+					path := strings.ReplaceAll(filename, `\`, `/`)
+					if strings.Contains(path, p) {
+						return true
+					}
+				}
+			}
+		}
+		return false
+	}
+	return true
+}
diff --git a/pkg/sqlcmd-linter/imports_test.go b/pkg/sqlcmd-linter/imports_test.go
@@ -0,0 +1,20 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+package sqlcmdlinter
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestImports(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Errorf("Failed to get wd: %s", err)
+	}
+	analysistest.Run(t, filepath.Join(wd, `testdata`), ImportsAnalyzer, "imports_linter_tests/...")
+}
diff --git a/pkg/sqlcmd-linter/testdata/src/github.com/README.md b/pkg/sqlcmd-linter/testdata/src/github.com/README.md
@@ -0,0 +1 @@
+This directory structure exists to provide stub package implementations for the linter tests. The `analysistest` package replaces `$GOPATH` with the local file system path. We create these stubs so the linter test files can closely mimic our production code.
diff --git a/pkg/sqlcmd-linter/testdata/src/github.com/alecthomas/chroma/chroma.go b/pkg/sqlcmd-linter/testdata/src/github.com/alecthomas/chroma/chroma.go
@@ -0,0 +1,3 @@
+package chroma
+
+var C = 1
diff --git a/pkg/sqlcmd-linter/testdata/src/github.com/microsoft/go-sqlcmd/pkg/sqlcmd/sqlcmd.go b/pkg/sqlcmd-linter/testdata/src/github.com/microsoft/go-sqlcmd/pkg/sqlcmd/sqlcmd.go
@@ -0,0 +1,3 @@
+package sqlcmd
+
+var S = 1
diff --git a/pkg/sqlcmd-linter/testdata/src/github.com/stretchr/testify/assert/main.go b/pkg/sqlcmd-linter/testdata/src/github.com/stretchr/testify/assert/main.go
@@ -0,0 +1,11 @@
+package assert
+
+import "testing"
+
+func NotNil(t *testing.T, object interface{}, msgAndArgs ...interface{}) bool {
+	return false
+}
+
+func NoError(t *testing.T, err error, msgAndArgs ...interface{}) bool {
+	return false
+}
diff --git a/pkg/sqlcmd-linter/testdata/src/imports_linter_tests/cmd/main/main.go b/pkg/sqlcmd-linter/testdata/src/imports_linter_tests/cmd/main/main.go
@@ -0,0 +1,12 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+package main
+
+import (
+	_ "github.com/alecthomas/chroma" // want "Non-internal packages should not import \"github.com/alecthomas/chroma\""
+	_ "github.com/microsoft/go-sqlcmd/pkg/sqlcmd"
+	_ "github.com/stretchr/testify/assert"
+)
+
+var X = 1
diff --git a/pkg/sqlcmd-linter/testdata/src/imports_linter_tests/internal/main.go b/pkg/sqlcmd-linter/testdata/src/imports_linter_tests/internal/main.go
@@ -0,0 +1,12 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+package main
+
+import (
+	_ "fmt"
+
+	_ "github.com/microsoft/go-sqlcmd/pkg/sqlcmd" // want "Internal packages should not import \"github.com/microsoft/go-sqlcmd/pkg/sqlcmd\""
+)
+
+var X = 1
diff --git a/pkg/sqlcmd-linter/testdata/src/useassert_linter_tests/assert.go b/pkg/sqlcmd-linter/testdata/src/useassert_linter_tests/assert.go
@@ -0,0 +1,21 @@
+package sqlcmd
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDontUseFatal(t *testing.T) {
+	t.Fatal("this should fail")        // want "Use assert package methods instead of Fatal"
+	t.Fatalf("this should %s", "fail") // want "Use assert package methods instead of Fatalf"
+	t.Fail()                           // want "Use assert package methods instead of Fail"
+	assert.NoError(t, fmt.Errorf("what"))
+	t.FailNow() // want "Use assert package methods instead of FailNow"
+
+}
+
+func TestDontUseRecover(t *testing.T) {
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }() // want "Use assert.Panics instead of recover()"
+}
diff --git a/pkg/sqlcmd-linter/useasserts.go b/pkg/sqlcmd-linter/useasserts.go
@@ -0,0 +1,58 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+package sqlcmdlinter
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var AssertAnalyzer = &analysis.Analyzer{
+	Name:     "assertlint",
+	Doc:      "Require use of asserts instead of fmt.Error functions in tests",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      runAsserts,
+}
+
+var blockedTestingMethods = []string{"Error", "ErrorF", "Fail", "FailNow", "Fatal", "Fatalf"}
+
+func runAsserts(pass *analysis.Pass) (interface{}, error) {
+	// pass.ResultOf[inspect.Analyzer] will be set if we've added inspect.Analyzer to Requires.
+	// Analyze code and make an AST from the file:
+	inspectorInstance := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	nodeFilter := []ast.Node{(*ast.CallExpr)(nil)}
+
+	inspectorInstance.Preorder(nodeFilter, func(n ast.Node) {
+		node := n.(*ast.CallExpr)
+		switch fun := node.Fun.(type) {
+		case (*ast.SelectorExpr):
+			switch funX := fun.X.(type) {
+			case (*ast.Ident):
+				if funX.Name == "t" && contains(blockedTestingMethods, fun.Sel.Name) {
+					pass.Reportf(node.Pos(), "Use assert package methods instead of %s", fun.Sel.Name)
+				}
+			default:
+				return
+			}
+		case (*ast.Ident):
+			if fun.Name == "recover" {
+				pass.Reportf(node.Pos(), "Use assert.Panics instead of recover()")
+			}
+		}
+	})
+	return nil, nil
+}
+
+func contains(a []string, v string) bool {
+	for _, val := range a {
+		if val == v {
+			return true
+		}
+	}
+	return false
+}
diff --git a/pkg/sqlcmd-linter/useasserts_test.go b/pkg/sqlcmd-linter/useasserts_test.go
@@ -0,0 +1,17 @@
+package sqlcmdlinter
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestUseAsserts(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Errorf("Failed to get wd: %s", err)
+	}
+	analysistest.Run(t, filepath.Join(wd, `testdata`), AssertAnalyzer, "useassert_linter_tests/...")
+}
diff --git a/pkg/sqlcmd/batch_test.go b/pkg/sqlcmd/batch_test.go
@@ -4,6 +4,7 @@
 package sqlcmd
 
 import (
+	"fmt"
 	"io"
 	"strings"
 	"testing"
@@ -53,7 +54,7 @@ func TestBatchNext(t *testing.T) {
 				}
 				break loop
 			case err != nil:
-				t.Fatalf("test %s did not expect error, got: %v", test.s, err)
+				assert.Fail(t, fmt.Sprintf("test %s did not expect error, got: %v", test.s, err))
 			}
 			if cmd != nil {
 				cmds = append(cmds, cmd.name)
@@ -143,9 +144,7 @@ func TestReadString(t *testing.T) {
 	for _, test := range tests {
 		r := []rune(test.s)
 		c, end := rune(strings.TrimSpace(test.s)[0]), len(r)
-		if c != '\'' && c != '"' {
-			t.Fatalf("test %+v incorrect!", test)
-		}
+		assert.False(t, c != '\'' && c != '"', fmt.Sprintf("test %+v incorrect!", test))
 		pos, ok, err := b.readString(r, test.i+1, end, c, uint(0))
 		assert.NoErrorf(t, err, "should be no error for %s", test)
 		assert.Equal(t, test.ok, ok, "test %+v ok", test)
diff --git a/pkg/sqlcmd/commands_test.go b/pkg/sqlcmd/commands_test.go
@@ -236,7 +236,7 @@ func TestConnectCommand(t *testing.T) {
 		// not using assert to avoid printing passwords in the log
 		assert.NotContains(t, buf.buf.String(), "$(servername)", "ConnectDB should have succeeded")
 		if s.Connect.UserName != c.UserName || c.Password != s.Connect.Password || s.Connect.LoginTimeoutSeconds != 111 {
-			t.Fatalf("After connect, sqlCmd.Connect is not updated %+v", s.Connect)
+			assert.Fail(t, fmt.Sprintf("After connect, sqlCmd.Connect is not updated %+v", s.Connect))
 		}
 	}
 }
diff --git a/pkg/sqlcmd/sqlcmd_test.go b/pkg/sqlcmd/sqlcmd_test.go
@@ -412,9 +412,7 @@ func TestPromptForPasswordPositive(t *testing.T) {
 	err = s.ConnectDb(c, false)
 	assert.True(t, prompted, "ConnectDb with !nopw should prompt for password")
 	assert.NoError(t, err, "ConnectDb with !nopw and valid password returned from prompt")
-	if s.Connect.Password != password {
-		t.Fatal(t, err, "Password not stored in the connection")
-	}
+	assert.Equal(t, password, s.Connect.Password, "Password not stored in the connection")
 }
 
 func TestVerticalLayoutNoColumns(t *testing.T) {

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+This directory structure exists to provide stub package implementations for the linter tests. The `analysistest` package replaces `$GOPATH` with the local file system path. We create these stubs so the linter test files can closely mimic our production code.