From 3c008e6dfe5a1cc2420c2a48209f35f876034b6b Mon Sep 17 00:00:00 2001
From: Eric Cornelissen <ericornelissen@gmail.com>
Date: Tue, 5 Dec 2023 23:09:46 +0100
Subject: [PATCH] Vet and improve source code using NilAway

Add NilAway (<https://github.com/uber-go/nilaway>) as a new vet check in
order to detect and address potential runtime errors due to nil values.

All 4 violations it detected have been addressed. In `analyze.go` this
simply constitutes checking that received pointers aren't nil. In
`main.go` this constites, first, explicitly relying on the `ok` check of
a map lookup when trying to use said key (instead of implicitly relying
on the control flow being such that said key is injected if it isn't
there already), and second, rewriting to avoid using `strings.Split` and
instead using index lookup - in this case the finding by NilAway was a
false positive (`nil` can never be returned by `strings.Split`, even if
the internal function used may return `nil` in some cases) but rewriting
reduces memory usage.

The modifications are partially tested, covering only `analyze.go`
because `main.go` isn't covered by unit tests.

Signed-off-by: Eric Cornelissen <ericornelissen@gmail.com>
---
 Makefile        |  1 +
 analyze.go      |  6 +++++-
 analyze_test.go | 20 ++++++++++++++++++++
 main.go         | 12 +++++++-----
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 714a69a..c17cefc 100644
--- a/Makefile
+++ b/Makefile
@@ -79,6 +79,7 @@ vet: ## Vet the source code
 	@go run github.com/tetafro/godot/cmd/godot@v1.4.15 .
 	@go run github.com/tomarrell/wrapcheck/v2/cmd/wrapcheck@v2.8.1 .
 	@go run golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow@2f9d82f .
+	@go run go.uber.org/nilaway/cmd/nilaway@a267567 .
 	@go run honnef.co/go/tools/cmd/staticcheck@v0.4.6 .
 	@go run mvdan.cc/unparam@3ee2d22 .
 
diff --git a/analyze.go b/analyze.go
index 7efe577..c8dde9b 100644
--- a/analyze.go
+++ b/analyze.go
@@ -38,7 +38,7 @@ type violation struct {
 var r = regexp.MustCompile(`\$\{\{.*?\}\}`)
 
 func analyzeManifest(manifest *Manifest) []violation {
-	if manifest.Runs.Using == "composite" {
+	if manifest != nil && manifest.Runs.Using == "composite" {
 		return analyzeSteps(manifest.Runs.Steps)
 	} else {
 		return make([]violation, 0)
@@ -47,6 +47,10 @@ func analyzeManifest(manifest *Manifest) []violation {
 
 func analyzeWorkflow(workflow *Workflow) []violation {
 	violations := make([]violation, 0)
+	if workflow == nil {
+		return violations
+	}
+
 	for id, job := range workflow.Jobs {
 		job := job
 		violations = append(violations, analyzeJob(id, &job)...)
diff --git a/analyze_test.go b/analyze_test.go
index 2cdfc5f..1eed56f 100644
--- a/analyze_test.go
+++ b/analyze_test.go
@@ -35,6 +35,12 @@ func TestAnalyzeManifest(t *testing.T) {
 			manifest: Manifest{
 				Runs: ManifestRuns{
 					Using: "node16",
+					Steps: []JobStep{
+						{
+							Name: "Example unsafe",
+							Run:  "echo ${{ inputs.value }}",
+						},
+					},
 				},
 			},
 			want: 0,
@@ -123,6 +129,13 @@ func TestAnalyzeManifest(t *testing.T) {
 			}
 		})
 	}
+
+	t.Run("nil pointer", func(t *testing.T) {
+		violations := analyzeManifest(nil)
+		if got, want := len(violations), 0; got != want {
+			t.Fatalf("Unexpected number of violations (got %d, want %d)", got, want)
+		}
+	})
 }
 
 func TestAnalyzeWorkflow(t *testing.T) {
@@ -244,6 +257,13 @@ func TestAnalyzeWorkflow(t *testing.T) {
 			}
 		})
 	}
+
+	t.Run("nil pointer", func(t *testing.T) {
+		violations := analyzeWorkflow(nil)
+		if got, want := len(violations), 0; got != want {
+			t.Fatalf("Unexpected number of violations (got %d, want %d)", got, want)
+		}
+	})
 }
 
 func TestAnalyzeJob(t *testing.T) {
diff --git a/main.go b/main.go
index 166d328..7419ea7 100644
--- a/main.go
+++ b/main.go
@@ -93,11 +93,13 @@ func run() int {
 
 			for file, fileViolations := range targetViolations {
 				if len(fileViolations) > 0 {
-					if _, ok := violations[target]; !ok {
-						violations[target] = make(map[string][]violation)
+					targetViolations, ok := violations[target]
+					if !ok {
+						targetViolations = make(map[string][]violation)
+						violations[target] = targetViolations
 					}
 
-					violations[target][file] = fileViolations
+					targetViolations[file] = fileViolations
 				}
 			}
 		} else {
@@ -310,8 +312,8 @@ func printViolation(v *violation) {
 }
 
 func getVariableNameForExpression(expression string) (name string) {
-	parts := strings.Split(expression, ".")
-	name = strings.TrimRight(parts[len(parts)-1], "}")
+	name = expression[strings.LastIndex(expression, ".")+1:]
+	name = strings.TrimRight(name, "}")
 	name = strings.TrimSpace(name)
 	return strings.ToUpper(name)
 }