Skip to content

Commit

Permalink
fix: more accurate unused import detection (#30)
Browse files Browse the repository at this point in the history
  • Loading branch information
notJoon authored Jul 25, 2024
1 parent 94ff0b0 commit aebfa05
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 151 deletions.
31 changes: 11 additions & 20 deletions formatter/general.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,28 +43,19 @@ func (f *GeneralIssueFormatter) Format(
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))

if len(snippet.Lines) > 0 {
line := expandTabs(snippet.Lines[lineIndex])
result.WriteString(lineStyle.Sprintf("%d | ", issue.Start.Line))
result.WriteString(line + "\n")
line := expandTabs(snippet.Lines[lineIndex])
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))
} else {
result.WriteString(messageStyle.Sprintf("Unable to display line. File might be empty.\n"))
result.WriteString(messageStyle.Sprintf("Issue: %s\n\n", issue.Message))
}
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))
} else {
result.WriteString(messageStyle.Sprintf("Unable to display line. File might be empty.\n"))
result.WriteString(messageStyle.Sprintf("Issue: %s\n\n", issue.Message))
}

return result.String()
}
Expand Down
6 changes: 3 additions & 3 deletions internal/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import (

// Engine manages the linting process.
type Engine struct {
SymbolTable *SymbolTable
rules []LintRule
ignoredRules map[string]bool
SymbolTable *SymbolTable
rules []LintRule
ignoredRules map[string]bool
}

// NewEngine creates a new lint engine.
Expand Down
126 changes: 34 additions & 92 deletions internal/lints/gno_analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"go/parser"
"go/token"
"os"
"path/filepath"
"strings"

tt "github.com/gnoswap-labs/lint/internal/types"
Expand All @@ -17,38 +16,22 @@ const (
GNO_STD_PACKAGE = "std"
)

// Dependency represents an imported package and its usage status.
type Dependency struct {
ImportPath string
IsGno bool
IsUsed bool
IsIgnored bool // alias with `_` should be ignored
IsIgnored bool // aliased as `_`
}

type (
// Dependencies is a map of import paths to their Dependency information.
Dependencies map[string]*Dependency
type Dependencies map[string]*Dependency

// FileMap is a map of filenames to their parsed AST representation.
FileMap map[string]*ast.File
)

// Package represents a Go/Gno package with its name and files.
type Package struct {
Name string
Files FileMap
}

// DetectGnoPackageImports analyzes the given file for Gno package imports and returns any issues found.
func DetectGnoPackageImports(filename string) ([]tt.Issue, error) {
dir := filepath.Dir(filename)

pkg, deps, err := analyzePackage(dir)
file, deps, err := analyzeFile(filename)
if err != nil {
return nil, fmt.Errorf("error analyzing package: %w", err)
return nil, fmt.Errorf("error analyzing file: %w", err)
}

issues := runGnoPackageLinter(pkg, deps)
issues := runGnoPackageLinter(file, deps)

for i := range issues {
issues[i].Filename = filename
Expand All @@ -57,83 +40,52 @@ func DetectGnoPackageImports(filename string) ([]tt.Issue, error) {
return issues, nil
}

// parses all gno files and collect their imports and usage.
func analyzePackage(dir string) (*Package, Dependencies, error) {
pkg := &Package{
Files: make(FileMap),
func analyzeFile(filename string) (*ast.File, Dependencies, error) {
content, err := os.ReadFile(filename)
if err != nil {
return nil, nil, err
}
deps := make(Dependencies)

files, err := filepath.Glob(filepath.Join(dir, "*.gno"))
fset := token.NewFileSet()
file, err := parser.ParseFile(fset, filename, content, parser.ParseComments)
if err != nil {
return nil, nil, err
}

// 1. Parse all file contents and collect dependencies
for _, file := range files {
f, err := parseFile(file)
if err != nil {
return nil, nil, err
}

pkg.Files[file] = f
if pkg.Name == "" {
pkg.Name = f.Name.Name
}

for _, imp := range f.Imports {
impPath := strings.Trim(imp.Path.Value, `"`)
if _, exists := deps[impPath]; !exists {
deps[impPath] = &Dependency{
ImportPath: impPath,
IsGno: isGnoPackage(impPath),
IsUsed: false,
IsIgnored: imp.Name != nil && imp.Name.Name == "_",
}
}
deps := make(Dependencies)
for _, imp := range file.Imports {
impPath := strings.Trim(imp.Path.Value, `"`)
deps[impPath] = &Dependency{
ImportPath: impPath,
IsGno: isGnoPackage(impPath),
IsUsed: false,
IsIgnored: imp.Name != nil && imp.Name.Name == "_",
}
}

// 2. Determine which dependencies are used
for _, file := range pkg.Files {
ast.Inspect(file, func(n ast.Node) bool {
switch x := n.(type) {
case *ast.SelectorExpr:
if ident, ok := x.X.(*ast.Ident); ok {
for _, imp := range file.Imports {
if imp.Name != nil && imp.Name.Name == ident.Name {
deps[strings.Trim(imp.Path.Value, `"`)].IsUsed = true
} else if lastPart := getLastPart(strings.Trim(imp.Path.Value, `"`)); lastPart == ident.Name {
deps[strings.Trim(imp.Path.Value, `"`)].IsUsed = true
}
// Determine which dependencies are used in this file
ast.Inspect(file, func(n ast.Node) bool {
switch x := n.(type) {
case *ast.SelectorExpr:
if ident, ok := x.X.(*ast.Ident); ok {
for _, imp := range file.Imports {
if imp.Name != nil && imp.Name.Name == ident.Name {
deps[strings.Trim(imp.Path.Value, `"`)].IsUsed = true
} else if lastPart := getLastPart(strings.Trim(imp.Path.Value, `"`)); lastPart == ident.Name {
deps[strings.Trim(imp.Path.Value, `"`)].IsUsed = true
}
}
}
return true
})
}
}
return true
})

return pkg, deps, nil
return file, deps, nil
}

func runGnoPackageLinter(pkg *Package, deps Dependencies) []tt.Issue {
func runGnoPackageLinter(_ *ast.File, deps Dependencies) []tt.Issue {
var issues []tt.Issue

for _, file := range pkg.Files {
ast.Inspect(file, func(n ast.Node) bool {
switch x := n.(type) {
case *ast.SelectorExpr:
// check unused imports
if ident, ok := x.X.(*ast.Ident); ok {
if dep, exists := deps[ident.Name]; exists {
dep.IsUsed = true
}
}
}
return true
})
}

for impPath, dep := range deps {
if !dep.IsUsed && !dep.IsIgnored {
issue := tt.Issue{
Expand All @@ -151,16 +103,6 @@ func isGnoPackage(importPath string) bool {
return strings.HasPrefix(importPath, GNO_PKG_PREFIX) || importPath == GNO_STD_PACKAGE
}

func parseFile(filename string) (*ast.File, error) {
content, err := os.ReadFile(filename)
if err != nil {
return nil, err
}

fset := token.NewFileSet()
return parser.ParseFile(fset, filename, content, parser.ParseComments)
}

func getLastPart(path string) string {
parts := strings.Split(path, "/")
return parts[len(parts)-1]
Expand Down
102 changes: 67 additions & 35 deletions internal/lints/gno_analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,46 +15,78 @@ func TestRunLinter(t *testing.T) {

testDir := filepath.Join(filepath.Dir(current), "..", "..", "testdata", "pkg")

pkg, deps, err := analyzePackage(testDir)
require.NoError(t, err)
require.NotNil(t, pkg)
require.NotNil(t, deps)

issues := runGnoPackageLinter(pkg, deps)

expectedIssues := []struct {
rule string
message string
tests := []struct {
filename string
expectedIssues []struct {
rule string
message string
}
expectedDeps map[string]struct {
isGno bool
isUsed bool
isIgnored bool
}
}{
{"unused-import", "unused import: strings"},
{
filename: filepath.Join(testDir, "pkg0.gno"),
expectedIssues: []struct {
rule string
message string
}{
{"unused-import", "unused import: strings"},
},
expectedDeps: map[string]struct {
isGno bool
isUsed bool
isIgnored bool
}{
"fmt": {false, true, false},
"gno.land/p/demo/ufmt": {true, true, false},
"strings": {false, false, false},
"std": {true, true, false},
"gno.land/p/demo/diff": {true, false, true},
},
},
{
filename: filepath.Join(testDir, "pkg1.gno"),
expectedIssues: []struct {
rule string
message string
}{},
expectedDeps: map[string]struct {
isGno bool
isUsed bool
isIgnored bool
}{
"strings": {false, true, false},
},
},
}

assert.Equal(t, len(expectedIssues), len(issues), "Number of issues doesn't match expected")
for _, tc := range tests {
t.Run(filepath.Base(tc.filename), func(t *testing.T) {
file, deps, err := analyzeFile(tc.filename)
require.NoError(t, err)
require.NotNil(t, file)

for i, expected := range expectedIssues {
assert.Equal(t, expected.rule, issues[i].Rule, "Rule doesn't match for issue %d", i)
assert.Contains(t, issues[i].Message, expected.message, "Message doesn't match for issue %d", i)
}
issues := runGnoPackageLinter(file, deps)

expectedDeps := map[string]struct {
isGno bool
isUsed bool
isIgnored bool
}{
"fmt": {false, true, false},
"gno.land/p/demo/ufmt": {true, true, false},
"strings": {false, false, false},
"std": {true, true, false},
"gno.land/p/demo/diff": {true, false, true},
}
assert.Equal(t, len(tc.expectedIssues), len(issues), "Number of issues doesn't match expected for %s", tc.filename)

for importPath, expected := range expectedDeps {
dep, exists := deps[importPath]
assert.True(t, exists, "Dependency %s not found", importPath)
if exists {
assert.Equal(t, expected.isGno, dep.IsGno, "IsGno mismatch for %s", importPath)
assert.Equal(t, expected.isUsed, dep.IsUsed, "IsUsed mismatch for %s", importPath)
assert.Equal(t, expected.isIgnored, dep.IsIgnored, "IsIgnored mismatch for %s", importPath)
}
for i, expected := range tc.expectedIssues {
assert.Equal(t, expected.rule, issues[i].Rule, "Rule doesn't match for issue %d in %s", i, tc.filename)
assert.Contains(t, issues[i].Message, expected.message, "Message doesn't match for issue %d in %s", i, tc.filename)
}

for importPath, expected := range tc.expectedDeps {
dep, exists := deps[importPath]
assert.True(t, exists, "Dependency %s not found in %s", importPath, tc.filename)
if exists {
assert.Equal(t, expected.isGno, dep.IsGno, "IsGno mismatch for %s in %s", importPath, tc.filename)
assert.Equal(t, expected.isUsed, dep.IsUsed, "IsUsed mismatch for %s in %s", importPath, tc.filename)
assert.Equal(t, expected.isIgnored, dep.IsIgnored, "IsIgnored mismatch for %s in %s", importPath, tc.filename)
}
}
})
}
}
2 changes: 1 addition & 1 deletion internal/lints/loop_allocation.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func DetectLoopAllocation(filename string) ([]tt.Issue, error) {
case *ast.CallExpr:
if isAllocationFunction(innerNode) {
issues = append(issues, tt.Issue{
Rule: "loop-allocation",
Rule: "loop-allocation",
Message: "Potential unnecessary allocation inside loop",
Start: fset.Position(innerNode.Pos()),
End: fset.Position(innerNode.End()),
Expand Down
9 changes: 9 additions & 0 deletions testdata/pkg/pkg1.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package main

import (
"strings"
)

func main() {
strings.Contains("foo", "o")
}

0 comments on commit aebfa05

Please sign in to comment.