From 9e8de0b13e0d20c268c91ae216544f0d55eae3f2 Mon Sep 17 00:00:00 2001 From: Lee ByeongJun Date: Thu, 18 Jul 2024 17:53:45 +0900 Subject: [PATCH] Refactor engine (#9) * refactor engine * update README --- README.md | 58 +++--- formatter/fmt.go | 26 +-- internal/doc.go | 48 +++++ internal/engine.go | 135 ++++++++++++ internal/lint.go | 216 +++++++++----------- internal/{rule_set_test.go => lint_test.go} | 0 internal/rule_set.go | 93 --------- internal/types.go | 17 ++ 8 files changed, 341 insertions(+), 252 deletions(-) create mode 100644 internal/doc.go create mode 100644 internal/engine.go rename internal/{rule_set_test.go => lint_test.go} (100%) delete mode 100644 internal/rule_set.go create mode 100644 internal/types.go diff --git a/README.md b/README.md index 7717723..675f7fb 100644 --- a/README.md +++ b/README.md @@ -47,55 +47,54 @@ tlin . tlin allows addition of custom lint rules beyond the default golangci-lint rules. To add a new lint rule, follow these steps: -1. Add a function defining the new rule in the `internal/rule_set.go` file. +> ⚠️ Must update relevant tests if you have added a new rule or formatter. -Example: +1. Implement the `LintRule` interface for your new rule: ```go -func (e *Engine) detectNewRule(filename string) ([]Issue, error) { - // rule implementation +type NewRule struct{} + +func (r *NewRule) Check(filename string) ([]Issue, error) { + // Implement your lint rule logic here + // return a slice of Issues and any error encountered } ``` -2. Add the new rule to the `Run` method in the `internal/lint.go` file. +2. Register your new rule in the `registerDefaultRules` method of the `Engine` struct in `internal/engine.go`: ```go -newRuleIssues, err := e.detectNewRule(tempFile) -if err != nil { - return nil, fmt.Errorf("error detecting new rule: %w", err) +func (e *Engine) registerDefaultRules() { + e.rules = append(e.rules, + &GolangciLintRule{}, + // ... + &NewRule{}, // Add your new rule here + ) } -filtered = append(filtered, newRuleIssues...) ``` -3. (Optional) Create a new formatter for the new rule in the `formatter` pacakge. - a. Create a new file named after your lint rule (e.g., `new_rule.go`) in the `formatter` package. +3. (Optional) if your rule requires special formatting, create a new formatter in the `formatter` package: - b. Implement the `IssueFormatter` interface for your new rule: + a. Create a new file (e.g., `formatter/new_rule.go`). + b. Implement the `IssueFormatter` interface for your new rule: - ```go - type NewRuleFormatter struct{} + ```go + type NewRuleFormatter struct{} - func (f *NewRuleFormatter) Format( - issue internal.Issue, - snippet *internal.SourceCode, + func (f *NewRuleFormatter) Format( + issue internal.Issue, + snippet *internal.SourceCode, ) string { - // Implementation of the formatting logic for the new rule + // Implement formatting logic for new rule here. } - ``` + ``` - c. Add the new formatter to the `GetFormatter` function in `formatter/fmt.go`: - - ```go - // rule set - const ( - // ... - NewRule = "new_rule" // <- define the new rule as constant - ) + c. Add the new formatter to the `GetFormatter` function in `formatter/fmt.go`. + ```go func GetFormatter(rule string) IssueFormatter { switch rule { // ... - case NewRule: + case "new_rule": // Add your new rule here return &NewRuleFormatter{} default: return &DefaultFormatter{} @@ -103,11 +102,8 @@ filtered = append(filtered, newRuleIssues...) } ``` -4. If necessary, update the `FormatIssueWithArrow` function in `formatter/fmt.go` to handle any special formatting requirements for your new rule. - By following these steps, you can add new lint rules and ensure they are properly formatted when displayed in the CLI. - ## Contributing We welcome all forms of contributions, including bug reports, feature requests, and pull requests. Please feel free to open an issue or submit a pull request. diff --git a/formatter/fmt.go b/formatter/fmt.go index 6d88154..eb54152 100644 --- a/formatter/fmt.go +++ b/formatter/fmt.go @@ -17,30 +17,30 @@ type IssueFormatter interface { Format(issue internal.Issue, snippet *internal.SourceCode) string } -// GetFormatter is a factory function that returns the appropriate IssueFormatter -// based on the given rule. -// If no specific formatter is found for the given rule, it returns a GeneralIssueFormatter. -func GetFormatter(rule string) IssueFormatter { - switch rule { - case UnnecessaryElse: - return &UnnecessaryElseFormatter{} - default: - return &GeneralIssueFormatter{} - } -} - // FormatIssuesWithArrows formats a slice of issues into a human-readable string. // It uses the appropriate formatter for each issue based on its rule. func FormatIssuesWithArrows(issues []internal.Issue, snippet *internal.SourceCode) string { var builder strings.Builder for _, issue := range issues { builder.WriteString(formatIssueHeader(issue)) - formatter := GetFormatter(issue.Rule) + formatter := getFormatter(issue.Rule) builder.WriteString(formatter.Format(issue, snippet)) } return builder.String() } +// getFormatter is a factory function that returns the appropriate IssueFormatter +// based on the given rule. +// If no specific formatter is found for the given rule, it returns a GeneralIssueFormatter. +func getFormatter(rule string) IssueFormatter { + switch rule { + case UnnecessaryElse: + return &UnnecessaryElseFormatter{} + default: + return &GeneralIssueFormatter{} + } +} + // formatIssueHeader creates a formatted header string for a given issue. // The header includes the rule and the filename. (e.g. "error: unused-variable\n --> test.go") func formatIssueHeader(issue internal.Issue) string { diff --git a/internal/doc.go b/internal/doc.go new file mode 100644 index 0000000..e114f9d --- /dev/null +++ b/internal/doc.go @@ -0,0 +1,48 @@ +// Package internal provides the core functionality for a Go-compatible linting tool. +// +// This package implements a flexible and extensible linting engine that can be used +// to analyze Go and Gno code for potential issues, style violations, and areas of improvement. +// It is designed to be easily extendable with custom lint rules while providing a set of +// default rules out of the box. +// +// Key components: +// +// Engine: The main linting engine that coordinates the linting process. +// It manages a collection of lint rules and applies them to the given source files. +// +// LintRule: An interface that defines the contract for all lint rules. +// Each lint rule must implement the Check method to analyze the code and return issues. +// +// Issue: Represents a single lint issue found in the code, including its location and description. +// +// SymbolTable: A data structure that keeps track of defined symbols across the codebase, +// helping to reduce false positives in certain lint rules. +// +// SourceCode: A simple structure to represent the content of a source file as a collection of lines. +// +// The package also includes several helper functions for file operations, running external tools, +// and managing temporary files during the linting process. +// +// Usage: +// +// engine, err := internal.NewEngine("path/to/root/dir") +// if err != nil { +// // handle error +// } +// +// // Optionally add custom rules +// engine.AddRule(myCustomRule) +// +// issues, err := engine.Run("path/to/file.go") +// if err != nil { +// // handle error +// } +// +// // Process the found issues +// for _, issue := range issues { +// fmt.Printf("Found issue: %s at %s\n", issue.Message, issue.Start) +// } +// +// This package is intended for internal use within the linting tool and should not be +// imported by external packages. +package internal diff --git a/internal/engine.go b/internal/engine.go new file mode 100644 index 0000000..65feea9 --- /dev/null +++ b/internal/engine.go @@ -0,0 +1,135 @@ +package internal + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +// Engine manages the linting process. +type Engine struct { + SymbolTable *SymbolTable + rules []LintRule +} + +// NewEngine creates a new lint engine. +func NewEngine(rootDir string) (*Engine, error) { + st, err := BuildSymbolTable(rootDir) + if err != nil { + return nil, fmt.Errorf("error building symbol table: %w", err) + } + + engine := &Engine{SymbolTable: st} + engine.registerDefaultRules() + + return engine, nil +} + +// registerDefaultRules adds the default set of lint rules to the engine. +func (e *Engine) registerDefaultRules() { + e.rules = append(e.rules, + &GolangciLintRule{}, + &UnnecessaryElseRule{}, + &UnusedFunctionRule{}, + ) +} + +// AddRule allows adding custom lint rules to the engine. +func (e *Engine) AddRule(rule LintRule) { + e.rules = append(e.rules, rule) +} + +// Run applies all lint rules to the given file and returns a slice of Issues. +func (e *Engine) Run(filename string) ([]Issue, error) { + tempFile, err := e.prepareFile(filename) + if err != nil { + return nil, err + } + defer e.cleanupTemp(tempFile) + + var allIssues []Issue + for _, rule := range e.rules { + issues, err := rule.Check(tempFile) + if err != nil { + return nil, fmt.Errorf("error running lint rule: %w", err) + } + allIssues = append(allIssues, issues...) + } + + filtered := e.filterUndefinedIssues(allIssues) + + // map issues back to .gno file if necessary + if strings.HasSuffix(filename, ".gno") { + for i := range filtered { + filtered[i].Filename = filename + } + } + + return filtered, nil +} + +func (e *Engine) prepareFile(filename string) (string, error) { + if strings.HasSuffix(filename, "gno") { + return createTempGoFile(filename) + } + return filename, nil +} + +func (e *Engine) cleanupTemp(temp string) { + if temp != "" && strings.HasPrefix(filepath.Base(temp), "temp_") { + _ = os.Remove(temp) + } +} + +func (e *Engine) filterUndefinedIssues(issues []Issue) []Issue { + var filtered []Issue + for _, issue := range issues { + if issue.Rule == "typecheck" && strings.Contains(issue.Message, "undefined:") { + symbol := strings.TrimSpace(strings.TrimPrefix(issue.Message, "undefined:")) + if e.SymbolTable.IsDefined(symbol) { + // ignore issues if the symbol is defined in the symbol table + continue + } + } + filtered = append(filtered, issue) + } + return filtered +} + +func createTempGoFile(gnoFile string) (string, error) { + content, err := os.ReadFile(gnoFile) + if err != nil { + return "", fmt.Errorf("error reading .gno file: %w", err) + } + + dir := filepath.Dir(gnoFile) + tempFile, err := os.CreateTemp(dir, "temp_*.go") + if err != nil { + return "", fmt.Errorf("error creating temp file: %w", err) + } + + _, err = tempFile.Write(content) + if err != nil { + os.Remove(tempFile.Name()) + return "", fmt.Errorf("error writing to temp file: %w", err) + } + + err = tempFile.Close() + if err != nil { + os.Remove(tempFile.Name()) + return "", fmt.Errorf("error closing temp file: %w", err) + } + + return tempFile.Name(), nil +} + +// ReadSourceFile reads the content of a file and returns it as a `SourceCode` struct. +func ReadSourceCode(filename string) (*SourceCode, error) { + content, err := os.ReadFile(filename) + if err != nil { + return nil, err + } + lines := strings.Split(string(content), "\n") + return &SourceCode{Lines: lines}, nil +} diff --git a/internal/lint.go b/internal/lint.go index c33fcab..1b3063b 100644 --- a/internal/lint.go +++ b/internal/lint.go @@ -3,113 +3,26 @@ package internal import ( "encoding/json" "fmt" + "go/ast" + "go/parser" "go/token" - "os" "os/exec" - "path/filepath" - "strings" ) -// SourceCode stores the content of a source code file. -type SourceCode struct { - Lines []string -} - -// ReadSourceFile reads the content of a file and returns it as a `SourceCode` struct. -func ReadSourceCode(filename string) (*SourceCode, error) { - content, err := os.ReadFile(filename) - if err != nil { - return nil, err - } - lines := strings.Split(string(content), "\n") - return &SourceCode{Lines: lines}, nil -} +/* +* Implement each lint rule as a separate 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 +// LintRule defines the interface for all lint rules. +type LintRule interface { + // Check runs the lint rule on the given file and returns a slice of Issues. + Check(filename string) ([]Issue, error) } -// Engine manages the linting process. -type Engine struct { - SymbolTable *SymbolTable -} +type GolangciLintRule struct{} -// NewEngine creates a new lint engine. -func NewEngine(rootDir string) (*Engine, error) { - st, err := BuildSymbolTable(rootDir) - if err != nil { - return nil, fmt.Errorf("error building symbol table: %w", err) - } - return &Engine{SymbolTable: st}, nil -} - -// Run applies golangci-lint to the given file and returns a slice of issues. -func (e *Engine) Run(filename string) ([]Issue, error) { - var tempFile string - var err error - - if strings.HasSuffix(filename, ".gno") { - tempFile, err = createTempGoFile(filename) - if err != nil { - return nil, fmt.Errorf("error creating temp file: %w", err) - } - // deferring the removal of the temporary file to the end of the function - // to ensure that the golangci-lint analyze the file before it's removed. - defer func() { - if tempFile != "" { - _ = os.Remove(tempFile) - } - }() - } else { - tempFile = filename - } - - issues, err := runGolangciLint(tempFile) - if err != nil { - return nil, fmt.Errorf("error running golangci-lint: %w", err) - } - filtered := e.filterUndefinedIssues(issues) - - unnecessaryElseIssues, err := e.detectUnnecessaryElse(tempFile) - if err != nil { - return nil, fmt.Errorf("error detecting unnecessary else: %w", err) - } - filtered = append(filtered, unnecessaryElseIssues...) - - unusedFunc, err := e.detectUnusedFunctions(tempFile) - if err != nil { - return nil, fmt.Errorf("error detecting unused functions: %w", err) - } - filtered = append(filtered, unusedFunc...) - - // map issues back to .gno file if necessary - if strings.HasSuffix(filename, ".gno") { - for i := range filtered { - filtered[i].Filename = filename - } - } - - return filtered, nil -} - -func (e *Engine) filterUndefinedIssues(issues []Issue) []Issue { - var filtered []Issue - for _, issue := range issues { - if issue.Rule == "typecheck" && strings.Contains(issue.Message, "undefined:") { - symbol := strings.TrimSpace(strings.TrimPrefix(issue.Message, "undefined:")) - if e.SymbolTable.IsDefined(symbol) { - // ignore issues if the symbol is defined in the symbol table - continue - } - } - filtered = append(filtered, issue) - } - return filtered +func (r *GolangciLintRule) Check(filename string) ([]Issue, error) { + return runGolangciLint(filename) } type golangciOutput struct { @@ -147,29 +60,102 @@ func runGolangciLint(filename string) ([]Issue, error) { return issues, nil } -func createTempGoFile(gnoFile string) (string, error) { - content, err := os.ReadFile(gnoFile) - if err != nil { - return "", fmt.Errorf("error reading .gno file: %w", err) - } +type UnnecessaryElseRule struct{} - dir := filepath.Dir(gnoFile) - tempFile, err := os.CreateTemp(dir, "temp_*.go") +func (r *UnnecessaryElseRule) Check(filename string) ([]Issue, error) { + engine := &Engine{} + return engine.detectUnnecessaryElse(filename) +} + +// detectUnnecessaryElse detects unnecessary else blocks. +// This rule considers an else block unnecessary if the if block ends with a return statement. +// In such cases, the else block can be removed and the code can be flattened to improve readability. +func (e *Engine) detectUnnecessaryElse(filename string) ([]Issue, error) { + fset := token.NewFileSet() + node, err := parser.ParseFile(fset, filename, nil, parser.ParseComments) if err != nil { - return "", fmt.Errorf("error creating temp file: %w", err) + return nil, err } - _, err = tempFile.Write(content) + var issues []Issue + ast.Inspect(node, func(n ast.Node) bool { + ifStmt, ok := n.(*ast.IfStmt) + if !ok { + return true + } + + if ifStmt.Else != nil { + blockStmt := ifStmt.Body + if len(blockStmt.List) > 0 { + lastStmt := blockStmt.List[len(blockStmt.List)-1] + if _, isReturn := lastStmt.(*ast.ReturnStmt); isReturn { + issue := Issue{ + Rule: "unnecessary-else", + Filename: filename, + Start: fset.Position(ifStmt.Else.Pos()), + End: fset.Position(ifStmt.Else.End()), + Message: "unnecessary else block", + } + issues = append(issues, issue) + } + } + } + + return true + }) + + return issues, nil +} + +type UnusedFunctionRule struct{} + +func (r *UnusedFunctionRule) Check(filename string) ([]Issue, error) { + engine := &Engine{} + return engine.detectUnusedFunctions(filename) +} + +// detectUnusedFunctions detects functions that are declared but never used. +// This rule reports all unused functions except for the following cases: +// 1. The main function: It's considered "used" as it's the entry point of the program. +// 2. The init function: It's used for package initialization and runs without explicit calls. +// 3. Exported functions: Functions starting with a capital letter are excluded as they might be used in other packages. +// +// This rule helps in code cleanup and improves maintainability. +func (e *Engine) detectUnusedFunctions(filename string) ([]Issue, error) { + fset := token.NewFileSet() + node, err := parser.ParseFile(fset, filename, nil, parser.ParseComments) if err != nil { - os.Remove(tempFile.Name()) - return "", fmt.Errorf("error writing to temp file: %w", err) + return nil, err } - err = tempFile.Close() - if err != nil { - os.Remove(tempFile.Name()) - return "", fmt.Errorf("error closing temp file: %w", err) + declaredFuncs := make(map[string]*ast.FuncDecl) + calledFuncs := make(map[string]bool) + + ast.Inspect(node, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.FuncDecl: + declaredFuncs[x.Name.Name] = x + case *ast.CallExpr: + if ident, ok := x.Fun.(*ast.Ident); ok { + calledFuncs[ident.Name] = true + } + } + return true + }) + + var issues []Issue + for funcName, funcDecl := range declaredFuncs { + if !calledFuncs[funcName] && funcName != "main" && funcName != "init" && !ast.IsExported(funcName) { + issue := Issue{ + Rule: "unused-function", + Filename: filename, + Start: fset.Position(funcDecl.Pos()), + End: fset.Position(funcDecl.End()), + Message: "function " + funcName + " is declared but not used", + } + issues = append(issues, issue) + } } - return tempFile.Name(), nil + return issues, nil } diff --git a/internal/rule_set_test.go b/internal/lint_test.go similarity index 100% rename from internal/rule_set_test.go rename to internal/lint_test.go diff --git a/internal/rule_set.go b/internal/rule_set.go deleted file mode 100644 index ad6ea2a..0000000 --- a/internal/rule_set.go +++ /dev/null @@ -1,93 +0,0 @@ -package internal - -import ( - "go/ast" - "go/parser" - "go/token" -) - -// detectUnnecessaryElse detects unnecessary else blocks. -// This rule considers an else block unnecessary if the if block ends with a return statement. -// In such cases, the else block can be removed and the code can be flattened to improve readability. -func (e *Engine) detectUnnecessaryElse(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 { - ifStmt, ok := n.(*ast.IfStmt) - if !ok { - return true - } - - if ifStmt.Else != nil { - blockStmt := ifStmt.Body - if len(blockStmt.List) > 0 { - lastStmt := blockStmt.List[len(blockStmt.List)-1] - if _, isReturn := lastStmt.(*ast.ReturnStmt); isReturn { - issue := Issue{ - Rule: "unnecessary-else", - Filename: filename, - Start: fset.Position(ifStmt.Else.Pos()), - End: fset.Position(ifStmt.Else.End()), - Message: "unnecessary else block", - } - issues = append(issues, issue) - } - } - } - - return true - }) - - return issues, nil -} - -// detectUnusedFunctions detects functions that are declared but never used. -// This rule reports all unused functions except for the following cases: -// 1. The main function: It's considered "used" as it's the entry point of the program. -// 2. The init function: It's used for package initialization and runs without explicit calls. -// 3. Exported functions: Functions starting with a capital letter are excluded as they might be used in other packages. -// -// This rule helps in code cleanup and improves maintainability. -func (e *Engine) detectUnusedFunctions(filename string) ([]Issue, error) { - fset := token.NewFileSet() - node, err := parser.ParseFile(fset, filename, nil, parser.ParseComments) - if err != nil { - return nil, err - } - - declaredFuncs := make(map[string]*ast.FuncDecl) - calledFuncs := make(map[string]bool) - - ast.Inspect(node, func(n ast.Node) bool { - switch x := n.(type) { - case *ast.FuncDecl: - declaredFuncs[x.Name.Name] = x - case *ast.CallExpr: - if ident, ok := x.Fun.(*ast.Ident); ok { - calledFuncs[ident.Name] = true - } - } - return true - }) - - var issues []Issue - for funcName, funcDecl := range declaredFuncs { - if !calledFuncs[funcName] && funcName != "main" && funcName != "init" && !ast.IsExported(funcName) { - issue := Issue{ - Rule: "unused-function", - Filename: filename, - Start: fset.Position(funcDecl.Pos()), - End: fset.Position(funcDecl.End()), - Message: "function " + funcName + " is declared but not used", - } - issues = append(issues, issue) - } - } - - return issues, nil -} diff --git a/internal/types.go b/internal/types.go new file mode 100644 index 0000000..66934a8 --- /dev/null +++ b/internal/types.go @@ -0,0 +1,17 @@ +package internal + +import "go/token" + +// SourceCode stores the content of a source code file. +type SourceCode struct { + Lines []string +} + +// 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 +}