diff --git a/internal/engine.go b/internal/engine.go index 7a5be67..927ad84 100644 --- a/internal/engine.go +++ b/internal/engine.go @@ -38,18 +38,15 @@ type ruleMap map[string]ruleConstructor // Create a map to hold the mappings of rule names to their constructors var allRuleConstructors = ruleMap{ "golangci-lint": NewGolangciLintRule, - "deprecated-function": NewDeprecateFuncRule, "early-return-opportunity": NewEarlyReturnOpportunityRule, "simplify-slice-range": NewSimplifySliceExprRule, "unnecessary-type-conversion": NewUnnecessaryConversionRule, - "loop-allocation": NewLoopAllocationRule, "emit-format": NewEmitFormatRule, "cycle-detection": NewDetectCycleRule, "unused-package": NewGnoSpecificRule, "repeated-regex-compilation": NewRepeatedRegexCompilationRule, "useless-break": NewUselessBreakRule, "defer-issues": NewDeferRule, - "slice-bounds-check": NewSliceBoundCheckRule, "const-error-declaration": NewConstErrorDeclarationRule, } diff --git a/internal/lints/lint_test.go b/internal/lints/lint_test.go index 3635339..29010f7 100644 --- a/internal/lints/lint_test.go +++ b/internal/lints/lint_test.go @@ -218,107 +218,6 @@ func example() { } } -func TestDetectLoopAllocation(t *testing.T) { - t.Parallel() - tests := []struct { - name string - code string - expected int - }{ - { - name: "Allocation in for loop", - code: ` -package main - -func main() { - for i := 0; i < 10; i++ { - _ = make([]int, 10) - } -}`, - expected: 1, - }, - { - name: "Allocation in range loop", - code: ` -package main - -func main() { - slice := []int{1, 2, 3} - for _, v := range slice { - _ = new(int) - _ = v - } -}`, - expected: 1, - }, - { - name: "Multiple allocations in loop", - code: ` -package main - -func main() { - for i := 0; i < 10; i++ { - _ = make([]int, 10) - _ = new(string) - } -}`, - expected: 2, - }, - { - name: "No allocation in loop", - code: ` -package main - -func main() { - slice := make([]int, 10) - for i := 0; i < 10; i++ { - slice[i] = i - } -}`, - expected: 0, - }, - { - name: "Allocation outside loop", - code: ` -package main - -func main() { - _ = make([]int, 10) - for i := 0; i < 10; i++ { - // Do something - } -}`, - expected: 0, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - tmpDir, err := os.MkdirTemp("", "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) - - node, fset, err := ParseFile(tmpfile, nil) - require.NoError(t, err) - - issues, err := DetectLoopAllocation(tmpfile, node, fset, types.SeverityError) - require.NoError(t, err) - - assert.Equal(t, tt.expected, len(issues), "Number of issues does not match expected") - - for _, issue := range issues { - assert.Contains(t, issue.Message, "potential unnecessary allocation inside loop") - } - }) - } -} - func TestDetectEmitFormat(t *testing.T) { t.Parallel() _, current, _, ok := runtime.Caller(0) @@ -441,105 +340,6 @@ func TestFormatEmitCall(t *testing.T) { } } -func TestDetectSliceBoundCheck(t *testing.T) { - t.Parallel() - tests := []struct { - name string - code string - expected int - }{ - { - name: "simple bound check", - code: ` -package main -func main() { - arr := []int{1, 2, 3} - if i < len(arr) { - _ = arr[i] - } -} - `, - expected: 0, - }, - { - name: "missing bound check", - code: ` -package main -func main() { - arr := []int{1, 2, 3} - _ = arr[i] -} - `, - expected: 1, - }, - { - name: "complex condition 2", - code: ` -package main - -type Item struct { - Name string - Value int -} - -func main() { - sourceItems := []*Item{ - {"item1", 10}, - {"item2", 20}, - {"item3", 30}, - } - - destinationItems := make([]*Item, 0, len(sourceItems)) - - i := 0 - for _, item := range sourceItems { - destinationItems[i] = item - i++ - } -} -`, - expected: 1, - }, - { - name: "accessing in range loop", - code: ` -package main - -func removeStringFromStringArr(arr []string, str string) []string { - for i, a := range arr { - if a == str { - return append(arr[:i], arr[i+1:]...) - } - } - return arr -} -`, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - fset := token.NewFileSet() - node, err := parser.ParseFile(fset, "", tt.code, 0) - if err != nil { - t.Fatalf("Failed to parse code: %v", err) - } - - issues, err := DetectSliceBoundCheck("test.go", node, fset, types.SeverityError) - for i, issue := range issues { - t.Logf("Issue %d: %v", i, issue) - } - assert.NoError(t, err) - assert.Equal( - t, tt.expected, len(issues), - "Number of detected slice bound check issues doesn't match expected", - ) - }) - } -} - func TestDetectUselessBreak(t *testing.T) { tests := []struct { name string diff --git a/internal/lints/loop_allocation.go b/internal/lints/loop_allocation.go deleted file mode 100644 index 07a7324..0000000 --- a/internal/lints/loop_allocation.go +++ /dev/null @@ -1,50 +0,0 @@ -package lints - -import ( - "go/ast" - "go/token" - - tt "github.com/gnolang/tlin/internal/types" -) - -func DetectLoopAllocation(filename string, node *ast.File, fset *token.FileSet, severity tt.Severity) ([]tt.Issue, error) { - var issues []tt.Issue - - ast.Inspect(node, func(n ast.Node) bool { - switch node := n.(type) { - case *ast.RangeStmt, *ast.ForStmt: - ast.Inspect(node, func(inner ast.Node) bool { - switch innerNode := inner.(type) { - case *ast.CallExpr: - if isAllocationFunction(innerNode) { - issues = append(issues, tt.Issue{ - Rule: "loop-allocation", - Message: "potential unnecessary allocation inside loop", - Start: fset.Position(innerNode.Pos()), - End: fset.Position(innerNode.End()), - Severity: severity, - }) - } - // case *ast.AssignStmt: - // if innerNode.Tok == token.DEFINE { - // issues = append(issues, tt.Issue{ - // Message: "Variable declaration inside loop may cause unnecessary allocation", - // Start: fset.Position(innerNode.Pos()), - // End: fset.Position(innerNode.End()), - // }) - // } - } - return true - }) - } - return true - }) - return issues, nil -} - -func isAllocationFunction(call *ast.CallExpr) bool { - if ident, ok := call.Fun.(*ast.Ident); ok { - return ident.Name == "make" || ident.Name == "new" - } - return false -} diff --git a/internal/lints/slice_bound.go b/internal/lints/slice_bound.go deleted file mode 100644 index f0dd062..0000000 --- a/internal/lints/slice_bound.go +++ /dev/null @@ -1,309 +0,0 @@ -package lints - -import ( - "fmt" - "go/ast" - "go/token" - - tt "github.com/gnolang/tlin/internal/types" -) - -// TODO: Make more precisely. -func DetectSliceBoundCheck(filename string, node *ast.File, fset *token.FileSet, severity tt.Severity) ([]tt.Issue, error) { - var issues []tt.Issue - ast.Inspect(node, func(n ast.Node) bool { - switch x := n.(type) { - case *ast.IndexExpr, *ast.SliceExpr: - ident, ok := getIdentForSliceOrArr(x) - if !ok { - return true - } - - if assignStmt, ok := findAssignmentForIdent(node, ident); ok { - if callExpr, ok := assignStmt.Rhs[0].(*ast.CallExpr); ok { - if fun, ok := callExpr.Fun.(*ast.Ident); ok && fun.Name == "make" { - // Slice created with make, it can be dangerous if the initial length is 0 - // because the slice will be nil and accessing an index will panic - if len(callExpr.Args) >= 2 { - if lit, ok := callExpr.Args[1].(*ast.BasicLit); ok && lit.Value == "0" { - issue := createIssue(x, ident, filename, fset, severity) - issues = append(issues, issue) - return true - } - } - } - } - } - - if !isWithinSafeContext(node, x) && !isWithinBoundsCheck(node, x, ident) { - issue := createIssue(x, ident, filename, fset, severity) - issues = append(issues, issue) - } - } - return true - }) - - return issues, nil -} - -func createIssue(node ast.Node, ident *ast.Ident, filename string, fset *token.FileSet, severity tt.Severity) tt.Issue { - var category, message, suggestion, note string - - switch x := node.(type) { - case *ast.IndexExpr: - if isConstantIndex(x.Index) { - return tt.Issue{} - } - category = "index-access" - message = "potential out of bounds array/slice index access" - suggestion = fmt.Sprintf("if i < len(%s) { value := %s[i] }", ident.Name, ident.Name) - note = "always check the length of the array/slice before accessing an index to prevent runtime panics." - case *ast.SliceExpr: - category = "slice-expression" - message = "potential out of bounds slice expression" - suggestion = fmt.Sprintf("%s = append(%s, newElement)", ident.Name, ident.Name) - note = "consider using append() for slices to automatically handle capacity and prevent out of bounds errors." - } - - return tt.Issue{ - Rule: "slice-bounds-check", - Category: category, - Filename: filename, - Start: fset.Position(node.Pos()), - End: fset.Position(node.End()), - Message: message, - Suggestion: suggestion, - Note: note, - Confidence: 0.8, - Severity: severity, - } -} - -// getIdentForSliceOrArr checks if the node is within an if statement -// that performs a bounds check. -func getIdentForSliceOrArr(node ast.Node) (*ast.Ident, bool) { - switch n := node.(type) { - case *ast.IndexExpr: - if ident, ok := n.X.(*ast.Ident); ok { - return ident, true - } - case *ast.SliceExpr: - if ident, ok := n.X.(*ast.Ident); ok { - return ident, true - } - } - return nil, false -} - -// isWithinBoundsCheck checks if the node is within an if statement that performs a bounds check. -func isWithinBoundsCheck(file *ast.File, node ast.Node, ident *ast.Ident) bool { - var ifStmt *ast.IfStmt - var found bool - - ast.Inspect(file, func(n ast.Node) bool { - if found { - return false - } - if x, ok := n.(*ast.IfStmt); ok && containsNode(x, node) { - ifStmt = x - found = true - } - return true - }) - - if ifStmt == nil { - return false - } - return isIfCapCheck(ifStmt, ident) -} - -// isIfCapCheck checks if the if statement condition is a capacity or length check. -func isIfCapCheck(ifStmt *ast.IfStmt, ident *ast.Ident) bool { - if binaryExpr, ok := ifStmt.Cond.(*ast.BinaryExpr); ok { - return binExprHasLenCapCall(binaryExpr, ident) - } - return false -} - -// binExprHasLenCapCall checks if a binary expression contains a length or capacity call. -func binExprHasLenCapCall(bin *ast.BinaryExpr, ident *ast.Ident) bool { - if call, ok := bin.X.(*ast.CallExpr); ok { - return isCapOrLenCallWithIdent(call, ident) - } - if call, ok := bin.Y.(*ast.CallExpr); ok { - return isCapOrLenCallWithIdent(call, ident) - } - return false -} - -// isCapOrLenCallWithIdent checks if a call expression is a len or cap call with the identifier. -func isCapOrLenCallWithIdent(call *ast.CallExpr, ident *ast.Ident) bool { - if fun, ok := call.Fun.(*ast.Ident); ok { - if (fun.Name == "len" || fun.Name == "cap") && len(call.Args) == 1 { - arg, ok := call.Args[0].(*ast.Ident) - return ok && arg.Name == ident.Name - } - } - return false -} - -// isWithinSafeContext checks if the given node is within a "safe" context. -// This function considers the following cases as "safe": -// 1. Inside a for loop with appropriate bounds checking -// 2. Inside a range loop where the loop variable is used as an index -// -// Behavior: -// 1. Traverses the entire AST to find the parent statements containing the given node. -// 2. If a range statement is found, it verifies if the node correctly uses the range variable. -// 3. If a for statement is found, it checks for appropriate length checks. -// 4. If a safe context is found, it immediately stops traversal and returns true. -// -// Notes: -// - This function may not cover all possible safe usage scenarios. -// - Complex nested structures or indirect access through function calls may be difficult to analyze accurately. -func isWithinSafeContext(file *ast.File, node ast.Node) bool { - var safeContext bool - var rangeIndex *ast.Ident - - ast.Inspect(file, func(n ast.Node) bool { - if n == node { - return false - } - switch x := n.(type) { - case *ast.RangeStmt: - if containsNode(x.Body, node) { - // Store the range index variable - if ident, ok := x.Key.(*ast.Ident); ok { - rangeIndex = ident - } - // Check if the node is a safe slice operation using the range index - if sliceExpr, ok := node.(*ast.SliceExpr); ok { - if isRangeIndexSlice(sliceExpr, rangeIndex) { - safeContext = true - return false - } - } - // inside a range statement, but check if the index expression is the range variable - if indexExpr, ok := node.(*ast.IndexExpr); ok { - if ident, ok := indexExpr.X.(*ast.Ident); ok { - // accessing a different slice/array than the range variable is not safe - safeContext = (ident.Name == rangeIndex.Name) - } - } - return false - } - case *ast.ForStmt: - if isForWithLenCheck(x) && containsNode(x.Body, node) { - safeContext = true - return false - } - } - return true - }) - return safeContext -} - -// isForWithLenCheck checks if a for statement has a length check condition. -func isForWithLenCheck(forStmt *ast.ForStmt) bool { - if cond, ok := forStmt.Cond.(*ast.BinaryExpr); ok { - if isBinaryExprLenCheck(cond) { - return true - } - } - return false -} - -// isConstantIndex determines if an expression is a constant index. -func isConstantIndex(expr ast.Expr) bool { - switch x := expr.(type) { - case *ast.BasicLit: - return x.Kind == token.INT - case *ast.Ident: - return x.Obj != nil && x.Obj.Kind == ast.Con - } - return false -} - -// isBinaryExprLenCheck checks if a binary expression is a length check. -func isBinaryExprLenCheck(expr *ast.BinaryExpr) bool { - if expr.Op == token.LSS || expr.Op == token.LEQ { - if call, ok := expr.Y.(*ast.CallExpr); ok { - if ident, ok := call.Fun.(*ast.Ident); ok { - return ident.Name == "len" - } - } - } - return false -} - -// findAssignmentForIdent finds the assignment statement for a given identifier. -func findAssignmentForIdent(file *ast.File, ident *ast.Ident) (*ast.AssignStmt, bool) { - var assignStmt *ast.AssignStmt - var found bool - - ast.Inspect(file, func(n ast.Node) bool { - if assign, ok := n.(*ast.AssignStmt); ok { - for _, lhs := range assign.Lhs { - if id, ok := lhs.(*ast.Ident); ok && id.Name == ident.Name { - assignStmt = assign - found = true - return false - } - } - } - return true - }) - - return assignStmt, found -} - -// isRangeIndexSlice checks if a node is a safe slice operation within a range loop. -func isRangeIndexSlice(node ast.Node, rangeIndex *ast.Ident) bool { - switch expr := node.(type) { - case *ast.SliceExpr: - return isRangeIndexInSlice(expr, rangeIndex) - case *ast.CallExpr: - // Check for append(arr[:i], arr[i+1:]...) pattern - if fun, ok := expr.Fun.(*ast.Ident); ok && fun.Name == "append" { - for _, arg := range expr.Args { - if slice, ok := arg.(*ast.SliceExpr); ok { - if isRangeIndexInSlice(slice, rangeIndex) { - return true - } - } - } - } - } - return false -} - -// isRangeIndexInSlice checks if a slice expression uses the range loop index. -func isRangeIndexInSlice(sliceExpr *ast.SliceExpr, rangeIndex *ast.Ident) bool { - // Check low bound - if isRangeIndexExpr(sliceExpr.Low, rangeIndex) { - return true - } - - // Check high bound - if isRangeIndexExpr(sliceExpr.High, rangeIndex) { - return true - } - - // Check for Ellipsis - if sliceExpr.Slice3 { - return isRangeIndexExpr(sliceExpr.Max, rangeIndex) - } - - return false -} - -// isRangeIndexExpr recursively checks if an expression uses the range loop index. -func isRangeIndexExpr(expr ast.Expr, rangeIndex *ast.Ident) bool { - switch e := expr.(type) { - case *ast.Ident: - return e.Name == rangeIndex.Name - case *ast.BinaryExpr: - return isRangeIndexExpr(e.X, rangeIndex) || isRangeIndexExpr(e.Y, rangeIndex) - } - return false -} diff --git a/internal/rule_set.go b/internal/rule_set.go index 2f3156d..1e3e299 100644 --- a/internal/rule_set.go +++ b/internal/rule_set.go @@ -53,32 +53,6 @@ func (r *GolangciLintRule) SetSeverity(severity tt.Severity) { r.severity = severity } -type DeprecateFuncRule struct { - severity tt.Severity -} - -func NewDeprecateFuncRule() LintRule { - return &DeprecateFuncRule{ - severity: tt.SeverityError, - } -} - -func (r *DeprecateFuncRule) Check(filename string, node *ast.File, fset *token.FileSet) ([]tt.Issue, error) { - return lints.DetectDeprecatedFunctions(filename, node, fset, r.severity) -} - -func (r *DeprecateFuncRule) Name() string { - return "deprecated-function" -} - -func (r *DeprecateFuncRule) Severity() tt.Severity { - return r.severity -} - -func (r *DeprecateFuncRule) SetSeverity(severity tt.Severity) { - r.severity = severity -} - type SimplifySliceExprRule struct { severity tt.Severity } @@ -131,32 +105,6 @@ func (r *UnnecessaryConversionRule) SetSeverity(severity tt.Severity) { r.severity = severity } -type LoopAllocationRule struct { - severity tt.Severity -} - -func NewLoopAllocationRule() LintRule { - return &LoopAllocationRule{ - severity: tt.SeverityWarning, - } -} - -func (r *LoopAllocationRule) Check(filename string, node *ast.File, fset *token.FileSet) ([]tt.Issue, error) { - return lints.DetectLoopAllocation(filename, node, fset, r.severity) -} - -func (r *LoopAllocationRule) Name() string { - return "loop-allocation" -} - -func (r *LoopAllocationRule) Severity() tt.Severity { - return r.severity -} - -func (r *LoopAllocationRule) SetSeverity(severity tt.Severity) { - r.severity = severity -} - type DetectCycleRule struct { severity tt.Severity } @@ -209,32 +157,6 @@ func (r *EmitFormatRule) SetSeverity(severity tt.Severity) { r.severity = severity } -type SliceBoundCheckRule struct { - severity tt.Severity -} - -func NewSliceBoundCheckRule() LintRule { - return &SliceBoundCheckRule{ - severity: tt.SeverityError, - } -} - -func (r *SliceBoundCheckRule) Check(filename string, node *ast.File, fset *token.FileSet) ([]tt.Issue, error) { - return lints.DetectSliceBoundCheck(filename, node, fset, r.severity) -} - -func (r *SliceBoundCheckRule) Name() string { - return "slice-bounds-check" -} - -func (r *SliceBoundCheckRule) Severity() tt.Severity { - return r.severity -} - -func (r *SliceBoundCheckRule) SetSeverity(severity tt.Severity) { - r.severity = severity -} - type UselessBreakRule struct { severity tt.Severity }