Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Unnecessary Type Conversion Lint Rule #12

Merged
merged 5 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading