Skip to content

Commit

Permalink
Add Unnecessary Type Conversion Lint Rule (#12)
Browse files Browse the repository at this point in the history
* type conv

* fix

* fix linter broke error

* formatting

* update example
  • Loading branch information
notJoon authored Jul 19, 2024
1 parent 32a28a8 commit e57cd00
Show file tree
Hide file tree
Showing 8 changed files with 407 additions and 2 deletions.
7 changes: 5 additions & 2 deletions formatter/fmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import (

// rule set
const (
UnnecessaryElse = "unnecessary-else"
SimplifySliceExpr = "simplify-slice-range"
UnnecessaryElse = "unnecessary-else"
UnnecessaryTypeConv = "unnecessary-type-conversion"
SimplifySliceExpr = "simplify-slice-range"
)

// IssueFormatter is the interface that wraps the Format method.
Expand Down Expand Up @@ -41,6 +42,8 @@ func getFormatter(rule string) IssueFormatter {
return &UnnecessaryElseFormatter{}
case SimplifySliceExpr:
return &SimplifySliceExpressionFormatter{}
case UnnecessaryTypeConv:
return &UnnecessaryTypeConversionFormatter{}
default:
return &GeneralIssueFormatter{}
}
Expand Down
40 changes: 40 additions & 0 deletions formatter/fmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,3 +296,43 @@ func main() {
})
}
}

func TestUnnecessaryTypeConversionFormatter(t *testing.T) {
formatter := &UnnecessaryTypeConversionFormatter{}

issue := internal.Issue{
Rule: "unnecessary-type-conversion",
Filename: "test.go",
Start: token.Position{Line: 5, Column: 10},
End: token.Position{Line: 5, Column: 20},
Message: "unnecessary type conversion",
Suggestion: "Remove the type conversion. Change `int(myInt)` to just `myInt`.",
Note: "Unnecessary type conversions can make the code less readable and may slightly impact performance. They are safe to remove when the expression already has the desired type.",
}

snippet := &internal.SourceCode{
Lines: []string{
"package main",
"",
"func main() {",
" myInt := 42",
" result := int(myInt)",
"}",
},
}

expected := ` |
5 | result := int(myInt)
| ^ unnecessary type conversion
Suggestion:
5 | Remove the type conversion. Change ` + "`int(myInt)`" + ` to just ` + "`myInt`" + `.
Note: Unnecessary type conversions can make the code less readable and may slightly impact performance. They are safe to remove when the expression already has the desired type.
`

result := formatter.Format(issue, snippet)

assert.Equal(t, expected, result, "Formatted output should match expected output")
}
35 changes: 35 additions & 0 deletions formatter/unnecessary_type_conv.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package formatter

import (
"fmt"
"strings"

"github.com/gnoswap-labs/lint/internal"
)

type UnnecessaryTypeConversionFormatter struct{}

func (f *UnnecessaryTypeConversionFormatter) 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()
}
1 change: 1 addition & 0 deletions internal/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func (e *Engine) registerDefaultRules() {
&UnnecessaryElseRule{},
&UnusedFunctionRule{},
&SimplifySliceExprRule{},
&UnnecessaryConversionRule{},
)
}

Expand Down
218 changes: 218 additions & 0 deletions internal/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"encoding/json"
"fmt"
"go/ast"
"go/importer"
"go/parser"
"go/token"
"go/types"
"os/exec"
)

Expand Down Expand Up @@ -226,3 +228,219 @@ func (e *Engine) detectUnnecessarySliceLength(filename string) ([]Issue, error)

return issues, nil
}

type UnnecessaryConversionRule struct{}

func (r *UnnecessaryConversionRule) Check(filename string) ([]Issue, error) {
engine := &Engine{}
return engine.detectUnnecessaryConversions(filename)
}

func (e *Engine) detectUnnecessaryConversions(filename string) ([]Issue, error) {
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, filename, nil, parser.ParseComments)
if err != nil {
return nil, err
}

info := &types.Info{
Types: make(map[ast.Expr]types.TypeAndValue),
Uses: make(map[*ast.Ident]types.Object),
Defs: make(map[*ast.Ident]types.Object),
}

conf := types.Config{Importer: importer.Default()}
//! DO NOT CHECK ERROR HERE.
//! error check may broke the lint formatting process.
conf.Check("", fset, []*ast.File{f}, info)

var issues []Issue
varDecls := make(map[*types.Var]ast.Node)

// First pass: collect variable declarations
ast.Inspect(f, func(n ast.Node) bool {
switch node := n.(type) {
case *ast.ValueSpec:
for _, name := range node.Names {
if obj := info.Defs[name]; obj != nil {
if v, ok := obj.(*types.Var); ok {
varDecls[v] = node
}
}
}
case *ast.AssignStmt:
for _, lhs := range node.Lhs {
if id, ok := lhs.(*ast.Ident); ok {
if obj := info.Defs[id]; obj != nil {
if v, ok := obj.(*types.Var); ok {
varDecls[v] = node
}
}
}
}
}
return true
})

// Second pass: check for unnecessary conversions
ast.Inspect(f, func(n ast.Node) bool {
call, ok := n.(*ast.CallExpr)
if !ok || len(call.Args) != 1 {
return true
}

ft, ok := info.Types[call.Fun]
if !ok || !ft.IsType() {
return true
}

at, ok := info.Types[call.Args[0]]
if !ok {
return true
}

if types.Identical(ft.Type, at.Type) && !isUntypedValue(call.Args[0], info) {
var memo, suggestion string

// find parent node and retrieve the entire assignment statement
var parent ast.Node
ast.Inspect(f, func(node ast.Node) bool {
if node == n {
return false
}
if contains(node, n) {
parent = node
return false
}
return true
})

if assignStmt, ok := parent.(*ast.AssignStmt); ok {
if len(assignStmt.Lhs) > 0 {
lhs := types.ExprString(assignStmt.Lhs[0])
rhs := types.ExprString(call.Args[0])
suggestion = fmt.Sprintf("%s := %s", lhs, rhs)
}
} else {
// not an assignment statement
// keep maintaining the original code
suggestion = types.ExprString(call.Args[0])
}

if id, ok := call.Args[0].(*ast.Ident); ok {
if obj, ok := info.Uses[id].(*types.Var); ok {
if _, exists := varDecls[obj]; exists {
declType := obj.Type().String()
memo = fmt.Sprintf(
"The variable '%s' is declared as type '%s'. This type conversion appears unnecessary.",
id.Name, declType,
)
}
}
}

issues = append(issues, Issue{
Rule: "unnecessary-type-conversion",
Filename: filename,
Start: fset.Position(call.Pos()),
End: fset.Position(call.End()),
Message: "unnecessary type conversion",
Suggestion: suggestion,
Note: memo,
})
}

return true
})

return issues, nil
}

// ref: https://github.com/mdempsky/unconvert/blob/master/unconvert.go#L570
func isUntypedValue(n ast.Expr, info *types.Info) (res bool) {
switch n := n.(type) {
case *ast.BinaryExpr:
switch n.Op {
case token.SHL, token.SHR:
// Shifts yield an untyped value if their LHS is untyped.
return isUntypedValue(n.X, info)
case token.EQL, token.NEQ, token.LSS, token.GTR, token.LEQ, token.GEQ:
// Comparisons yield an untyped boolean value.
return true
case token.ADD, token.SUB, token.MUL, token.QUO, token.REM,
token.AND, token.OR, token.XOR, token.AND_NOT,
token.LAND, token.LOR:
return isUntypedValue(n.X, info) && isUntypedValue(n.Y, info)
}
case *ast.UnaryExpr:
switch n.Op {
case token.ADD, token.SUB, token.NOT, token.XOR:
return isUntypedValue(n.X, info)
}
case *ast.BasicLit:
// Basic literals are always untyped.
return true
case *ast.ParenExpr:
return isUntypedValue(n.X, info)
case *ast.SelectorExpr:
return isUntypedValue(n.Sel, info)
case *ast.Ident:
if obj, ok := info.Uses[n]; ok {
if obj.Pkg() == nil && obj.Name() == "nil" {
// The universal untyped zero value.
return true
}
if b, ok := obj.Type().(*types.Basic); ok && b.Info()&types.IsUntyped != 0 {
// Reference to an untyped constant.
return true
}
}
case *ast.CallExpr:
if b, ok := asBuiltin(n.Fun, info); ok {
switch b.Name() {
case "real", "imag":
return isUntypedValue(n.Args[0], info)
case "complex":
return isUntypedValue(n.Args[0], info) && isUntypedValue(n.Args[1], info)
}
}
}

return false
}

func asBuiltin(n ast.Expr, info *types.Info) (*types.Builtin, bool) {
for {
paren, ok := n.(*ast.ParenExpr)
if !ok {
break
}
n = paren.X
}

ident, ok := n.(*ast.Ident)
if !ok {
return nil, false
}

obj, ok := info.Uses[ident]
if !ok {
return nil, false
}

b, ok := obj.(*types.Builtin)
return b, ok
}

// contains checks if parent contains child node
func contains(parent, child ast.Node) bool {
found := false
ast.Inspect(parent, func(n ast.Node) bool {
if n == child {
found = true
return false
}
return true
})
return found
}
Loading

0 comments on commit e57cd00

Please sign in to comment.