From 32a28a84e44ff86cfaa99a5d4bf19a31713ff700 Mon Sep 17 00:00:00 2001 From: Lee ByeongJun Date: Fri, 19 Jul 2024 14:27:48 +0900 Subject: [PATCH] simplify slice expr (avoid len) (#10) * simplify slice expr (avoid len) * fix lint * case specific suggestion output * disable gosimple check and use our lint * improve message --- .github/workflows/go.yml | 5 +- formatter/fmt.go | 20 +++++++- formatter/simplify_slice_expr.go | 35 +++++++++++++ internal/engine.go | 1 + internal/lint.go | 69 ++++++++++++++++++++++++- internal/lint_test.go | 88 ++++++++++++++++++++++++++++++++ internal/types.go | 12 +++-- testdata/slice0.gno | 16 ++++++ 8 files changed, 235 insertions(+), 11 deletions(-) create mode 100644 formatter/simplify_slice_expr.go create mode 100644 testdata/slice0.gno diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 983fe5f..4050875 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -25,7 +25,4 @@ jobs: run: go build -v ./... - name: Test - run: go test -v ./... - - - name: Golangci-lint - uses: golangci/golangci-lint-action@v6.0.1 \ No newline at end of file + run: go test -v ./... \ No newline at end of file diff --git a/formatter/fmt.go b/formatter/fmt.go index eb54152..4471379 100644 --- a/formatter/fmt.go +++ b/formatter/fmt.go @@ -1,14 +1,17 @@ package formatter import ( + "fmt" "strings" + "github.com/fatih/color" "github.com/gnoswap-labs/lint/internal" ) // rule set const ( - UnnecessaryElse = "unnecessary-else" + UnnecessaryElse = "unnecessary-else" + SimplifySliceExpr = "simplify-slice-range" ) // IssueFormatter is the interface that wraps the Format method. @@ -36,6 +39,8 @@ func getFormatter(rule string) IssueFormatter { switch rule { case UnnecessaryElse: return &UnnecessaryElseFormatter{} + case SimplifySliceExpr: + return &SimplifySliceExpressionFormatter{} default: return &GeneralIssueFormatter{} } @@ -47,3 +52,16 @@ func formatIssueHeader(issue internal.Issue) string { return errorStyle.Sprint("error: ") + ruleStyle.Sprint(issue.Rule) + "\n" + lineStyle.Sprint(" --> ") + fileStyle.Sprint(issue.Filename) + "\n" } + +func buildSuggestion(result *strings.Builder, issue internal.Issue, lineStyle, suggestionStyle *color.Color) { + result.WriteString(suggestionStyle.Sprintf("Suggestion:\n")) + result.WriteString(lineStyle.Sprintf("%d | ", issue.Start.Line)) + result.WriteString(fmt.Sprintf("%s\n", issue.Suggestion)) + result.WriteString("\n") +} + +func buildNote(result *strings.Builder, issue internal.Issue, suggestionStyle *color.Color) { + result.WriteString(suggestionStyle.Sprint("Note: ")) + result.WriteString(fmt.Sprintf("%s\n", issue.Note)) + result.WriteString("\n") +} diff --git a/formatter/simplify_slice_expr.go b/formatter/simplify_slice_expr.go new file mode 100644 index 0000000..c55cafc --- /dev/null +++ b/formatter/simplify_slice_expr.go @@ -0,0 +1,35 @@ +package formatter + +import ( + "fmt" + "strings" + + "github.com/gnoswap-labs/lint/internal" +) + +type SimplifySliceExpressionFormatter struct{} + +func (f *SimplifySliceExpressionFormatter) Format( + issue internal.Issue, + snippet *internal.SourceCode, +) string { + var result strings.Builder + + lineNumberStr := fmt.Sprintf("%d", issue.Start.Line) + padding := strings.Repeat(" ", len(lineNumberStr)-1) + result.WriteString(lineStyle.Sprintf(" %s|\n", padding)) + + line := expandTabs(snippet.Lines[issue.Start.Line-1]) + result.WriteString(lineStyle.Sprintf("%d | ", issue.Start.Line)) + result.WriteString(line + "\n") + + visualColumn := calculateVisualColumn(line, issue.Start.Column) + result.WriteString(lineStyle.Sprintf(" %s| ", padding)) + result.WriteString(strings.Repeat(" ", visualColumn)) + result.WriteString(messageStyle.Sprintf("^ %s\n\n", issue.Message)) + + buildSuggestion(&result, issue, lineStyle, suggestionStyle) + buildNote(&result, issue, suggestionStyle) + + return result.String() +} diff --git a/internal/engine.go b/internal/engine.go index 65feea9..1eab575 100644 --- a/internal/engine.go +++ b/internal/engine.go @@ -32,6 +32,7 @@ func (e *Engine) registerDefaultRules() { &GolangciLintRule{}, &UnnecessaryElseRule{}, &UnusedFunctionRule{}, + &SimplifySliceExprRule{}, ) } diff --git a/internal/lint.go b/internal/lint.go index 1b3063b..a3fc76a 100644 --- a/internal/lint.go +++ b/internal/lint.go @@ -38,7 +38,7 @@ type golangciOutput struct { } func runGolangciLint(filename string) ([]Issue, error) { - cmd := exec.Command("golangci-lint", "run", "--out-format=json", filename) + cmd := exec.Command("golangci-lint", "run", "--disable=gosimple", "--out-format=json", filename) output, _ := cmd.CombinedOutput() var golangciResult golangciOutput @@ -159,3 +159,70 @@ func (e *Engine) detectUnusedFunctions(filename string) ([]Issue, error) { return issues, nil } + +type SimplifySliceExprRule struct{} + +func (r *SimplifySliceExprRule) Check(filename string) ([]Issue, error) { + engine := &Engine{} + return engine.detectUnnecessarySliceLength(filename) +} + +func (e *Engine) detectUnnecessarySliceLength(filename string) ([]Issue, error) { + fset := token.NewFileSet() + node, err := parser.ParseFile(fset, filename, nil, parser.ParseComments) + if err != nil { + return nil, err + } + + var issues []Issue + ast.Inspect(node, func(n ast.Node) bool { + sliceExpr, ok := n.(*ast.SliceExpr) + if !ok { + return true + } + + if callExpr, ok := sliceExpr.High.(*ast.CallExpr); ok { + if ident, ok := callExpr.Fun.(*ast.Ident); ok && ident.Name == "len" { + if arg, ok := callExpr.Args[0].(*ast.Ident); ok { + if sliceExpr.X.(*ast.Ident).Name == arg.Name { + var suggestion, detailedMessage string + baseMessage := "unnecessary use of len() in slice expression, can be simplified" + + if sliceExpr.Low == nil { + suggestion = fmt.Sprintf("%s[:]", arg.Name) + detailedMessage = fmt.Sprintf( + "%s\nIn this case, `%s[:len(%s)]` is equivalent to `%s[:]`. "+ + "The full length of the slice is already implied when omitting both start and end indices.", + baseMessage, arg.Name, arg.Name, arg.Name) + } else if basicLit, ok := sliceExpr.Low.(*ast.BasicLit); ok { + suggestion = fmt.Sprintf("%s[%s:]", arg.Name, basicLit.Value) + detailedMessage = fmt.Sprintf("%s\nHere, `%s[%s:len(%s)]` can be simplified to `%s[%s:]`. "+ + "When slicing to the end of a slice, using len() is unnecessary.", + baseMessage, arg.Name, basicLit.Value, arg.Name, arg.Name, basicLit.Value) + } else if lowIdent, ok := sliceExpr.Low.(*ast.Ident); ok { + suggestion = fmt.Sprintf("%s[%s:]", arg.Name, lowIdent.Name) + detailedMessage = fmt.Sprintf("%s\nIn this instance, `%s[%s:len(%s)]` can be written as `%s[%s:]`. "+ + "The len() function is redundant when slicing to the end, regardless of the start index.", + baseMessage, arg.Name, lowIdent.Name, arg.Name, arg.Name, lowIdent.Name) + } + + issue := Issue{ + Rule: "simplify-slice-range", + Filename: filename, + Start: fset.Position(sliceExpr.Pos()), + End: fset.Position(sliceExpr.End()), + Message: baseMessage, + Suggestion: suggestion, + Note: detailedMessage, + } + issues = append(issues, issue) + } + } + } + } + + return true + }) + + return issues, nil +} diff --git a/internal/lint_test.go b/internal/lint_test.go index 21a4cd8..b87c0f0 100644 --- a/internal/lint_test.go +++ b/internal/lint_test.go @@ -197,3 +197,91 @@ func unused2() { }) } } + +func TestDetectUnnecessarySliceLength(t *testing.T) { + baseMsg := "unnecessary use of len() in slice expression, can be simplified" + tests := []struct { + name string + code string + expected int + message string + }{ + { + name: "suggests to use slice[:]", + code: ` +package main + +func main() { + slice := []int{1, 2, 3} + _ = slice[:len(slice)] +}`, + expected: 1, + message: baseMsg, + }, + { + name: "suggests to use slice[a:]", + code: ` +package main + +func main() { + slice := []int{1, 2, 3} + _ = slice[1:len(slice)] +}`, + expected: 1, + message: baseMsg, + }, + { + name: "Unnecessary slice length", + code: ` +package main + +func main() { + slice := []int{1, 2, 3} + _ = slice[:] +}`, + expected: 0, + }, + { + name: "slice[a:len(slice)] -> slice[a:] (a: variable)", + code: ` +package main + +func main() { + slice := []int{1, 2, 3} + a := 1 + _ = slice[a:len(slice)] +}`, + expected: 1, + message: baseMsg, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "lint-test") + require.NoError(t, err) + defer os.RemoveAll(tmpDir) + + tmpfile := filepath.Join(tmpDir, "test.go") + err = os.WriteFile(tmpfile, []byte(tt.code), 0o644) + require.NoError(t, err) + + engine := &Engine{} + issues, err := engine.detectUnnecessarySliceLength(tmpfile) + require.NoError(t, err) + + assert.Equal(t, tt.expected, len(issues), "Number of detected unnecessary slice length doesn't match expected") + + if len(issues) > 0 { + for _, issue := range issues { + assert.Equal(t, "simplify-slice-range", issue.Rule) + assert.Equal( + t, + tt.message, + issue.Message, + ) + } + } + }) + } +} diff --git a/internal/types.go b/internal/types.go index 66934a8..37352bd 100644 --- a/internal/types.go +++ b/internal/types.go @@ -9,9 +9,11 @@ type SourceCode struct { // Issue represents a lint issue found in the code base. type Issue struct { - Rule string - Filename string - Start token.Position - End token.Position - Message string + Rule string + Filename string + Start token.Position + End token.Position + Message string + Suggestion string + Note string } diff --git a/testdata/slice0.gno b/testdata/slice0.gno new file mode 100644 index 0000000..7008ff4 --- /dev/null +++ b/testdata/slice0.gno @@ -0,0 +1,16 @@ +package slice + +var slice = []int{1, 2, 3} + +func foo() { + _ = slice[1:len(slice)] +} + +func bar() { + _ = slice[:len(slice)] +} + +func baz() { + a := 1 + _ = slice[a:len(slice)] +}