From 8d6042f46ac0de309ac55db561c08d6a8583623b Mon Sep 17 00:00:00 2001 From: Lee ByeongJun Date: Tue, 22 Oct 2024 14:25:57 +0900 Subject: [PATCH] feat: Add `const-error-declaration` Suggestion (#97) # Description Added a suggestion to the `const-error-declaration` rule. Additionally, I modified most rules to apply `GeneralFormatter type when formatting issues. --- formatter/builder.go | 107 ++++++++++++----------------- formatter/cyclomatic_complexity.go | 4 +- formatter/defers.go | 18 ----- formatter/deprecated.go | 18 ----- formatter/early_return.go | 19 ----- formatter/format_emit.go | 19 ----- formatter/formatter_test.go | 2 +- formatter/general.go | 4 +- formatter/missing_mod_pacakge.go | 2 +- formatter/simplify_slice_expr.go | 19 ----- formatter/slice_bound.go | 4 +- formatter/unnecessary_type_conv.go | 19 ----- internal/lints/const_error_decl.go | 17 ++++- internal/lints/lint_test.go | 52 +++++++++++--- 14 files changed, 111 insertions(+), 193 deletions(-) delete mode 100644 formatter/defers.go delete mode 100644 formatter/deprecated.go delete mode 100644 formatter/early_return.go delete mode 100644 formatter/format_emit.go delete mode 100644 formatter/simplify_slice_expr.go delete mode 100644 formatter/unnecessary_type_conv.go diff --git a/formatter/builder.go b/formatter/builder.go index e8e8ca5..30191e7 100644 --- a/formatter/builder.go +++ b/formatter/builder.go @@ -10,21 +10,16 @@ import ( tt "github.com/gnolang/tlin/internal/types" ) +const tabWidth = 8 + // rule set const ( - EarlyReturn = "early-return" - UnnecessaryTypeConv = "unnecessary-type-conversion" - SimplifySliceExpr = "simplify-slice-range" - CycloComplexity = "high-cyclomatic-complexity" - EmitFormat = "emit-format" - SliceBound = "slice-bounds-check" - Defers = "defer-issues" - MissingModPackage = "gno-mod-tidy" - DeprecatedFunc = "deprecated" + CycloComplexity = "high-cyclomatic-complexity" + SliceBound = "slice-bounds-check" + MissingModPackage = "gno-mod-tidy" + DeprecatedFunc = "deprecated" ) -const tabWidth = 8 - var ( errorStyle = color.New(color.FgRed, color.Bold) warningStyle = color.New(color.FgHiYellow, color.Bold) @@ -35,11 +30,11 @@ var ( suggestionStyle = color.New(color.FgGreen, color.Bold) ) -// IssueFormatter is the interface that wraps the Format method. +// issueFormatter is the interface that wraps the Format method. // Implementations of this interface are responsible for formatting specific types of lint issues. // // ! TODO: Use template to format issue -type IssueFormatter interface { +type issueFormatter interface { Format(issue tt.Issue, snippet *internal.SourceCode) string } @@ -48,7 +43,6 @@ type IssueFormatter interface { func GenerateFormattedIssue(issues []tt.Issue, snippet *internal.SourceCode) string { var builder strings.Builder for _, issue := range issues { - // builder.WriteString(formatIssueHeader(issue)) formatter := getFormatter(issue.Rule) builder.WriteString(formatter.Format(issue, snippet)) } @@ -58,24 +52,12 @@ func GenerateFormattedIssue(issues []tt.Issue, snippet *internal.SourceCode) str // 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 { +func getFormatter(rule string) issueFormatter { switch rule { - case DeprecatedFunc: - return &DeprecatedFuncFormatter{} - case EarlyReturn: - return &EarlyReturnOpportunityFormatter{} - case SimplifySliceExpr: - return &SimplifySliceExpressionFormatter{} - case UnnecessaryTypeConv: - return &UnnecessaryTypeConversionFormatter{} case CycloComplexity: return &CyclomaticComplexityFormatter{} - case EmitFormat: - return &EmitFormatFormatter{} case SliceBound: return &SliceBoundsCheckFormatter{} - case Defers: - return &DefersFormatter{} case MissingModPackage: return &MissingModPackageFormatter{} default: @@ -85,7 +67,7 @@ func getFormatter(rule string) IssueFormatter { /***** Issue Formatter Builder *****/ -type IssueFormatterBuilder struct { +type issueFormatterBuilder struct { snippet *internal.SourceCode padding string commonIndent string @@ -96,7 +78,7 @@ type IssueFormatterBuilder struct { maxLineNumWidth int } -func NewIssueFormatterBuilder(issue tt.Issue, snippet *internal.SourceCode) *IssueFormatterBuilder { +func newIssueFormatterBuilder(issue tt.Issue, snippet *internal.SourceCode) *issueFormatterBuilder { startLine := issue.Start.Line endLine := issue.End.Line maxLineNumWidth := calculateMaxLineNumWidth(endLine) @@ -109,7 +91,7 @@ func NewIssueFormatterBuilder(issue tt.Issue, snippet *internal.SourceCode) *Iss commonIndent = findCommonIndent(snippet.Lines[startLine-1 : endLine]) } - return &IssueFormatterBuilder{ + return &issueFormatterBuilder{ issue: issue, snippet: snippet, startLine: startLine, @@ -120,30 +102,30 @@ func NewIssueFormatterBuilder(issue tt.Issue, snippet *internal.SourceCode) *Iss } } -func (b *IssueFormatterBuilder) AddHeader() *IssueFormatterBuilder { +func (b *issueFormatterBuilder) AddHeader() *issueFormatterBuilder { // add header type and rule name switch b.issue.Severity { case tt.SeverityError: - b.result.WriteString(errorStyle.Sprint("error: ")) + b.writeStyledLine(errorStyle, "error: ") case tt.SeverityWarning: - b.result.WriteString(warningStyle.Sprint("warning: ")) + b.writeStyledLine(warningStyle, "warning: ") case tt.SeverityInfo: - b.result.WriteString(messageStyle.Sprint("info: ")) + b.writeStyledLine(messageStyle, "info: ") } - b.result.WriteString(ruleStyle.Sprintln(b.issue.Rule)) + b.writeStyledLine(ruleStyle, "%s\n", b.issue.Rule) // add file name padding := strings.Repeat(" ", b.maxLineNumWidth) - b.result.WriteString(lineStyle.Sprint(fmt.Sprintf("%s--> ", padding))) - b.result.WriteString(fileStyle.Sprintf("%s:%d:%d\n", b.issue.Filename, b.issue.Start.Line, b.issue.Start.Column)) + b.writeStyledLine(lineStyle, "%s--> ", padding) + b.writeStyledLine(fileStyle, "%s:%d:%d\n", b.issue.Filename, b.issue.Start.Line, b.issue.Start.Column) return b } -func (b *IssueFormatterBuilder) AddCodeSnippet() *IssueFormatterBuilder { +func (b *issueFormatterBuilder) AddCodeSnippet() *issueFormatterBuilder { // add separator - b.result.WriteString(lineStyle.Sprintf("%s|\n", b.padding)) + b.writeStyledLine(lineStyle, "%s|\n", b.padding) for i := b.startLine; i <= b.endLine; i++ { if i-1 < 0 || i-1 >= len(b.snippet.Lines) { @@ -154,18 +136,17 @@ func (b *IssueFormatterBuilder) AddCodeSnippet() *IssueFormatterBuilder { line = strings.TrimPrefix(line, b.commonIndent) lineNum := fmt.Sprintf("%*d", b.maxLineNumWidth, i) - b.result.WriteString(lineStyle.Sprintf("%s | ", lineNum)) - b.result.WriteString(line + "\n") + b.writeStyledLine(lineStyle, "%s | %s\n", lineNum, line) } return b } -func (b *IssueFormatterBuilder) AddUnderlineAndMessage() *IssueFormatterBuilder { - b.result.WriteString(lineStyle.Sprintf("%s| ", b.padding)) +func (b *issueFormatterBuilder) AddUnderlineAndMessage() *issueFormatterBuilder { + b.writeStyledLine(lineStyle, "%s| ", b.padding) if !b.isValidLineRange() { - b.result.WriteString(messageStyle.Sprintf("%s\n\n", b.issue.Message)) + b.writeStyledLine(messageStyle, "%s\n\n", b.issue.Message) return b } @@ -182,61 +163,61 @@ func (b *IssueFormatterBuilder) AddUnderlineAndMessage() *IssueFormatterBuilder underlineLength := underlineEnd - underlineStart + 1 b.result.WriteString(strings.Repeat(" ", underlineStart)) - b.result.WriteString(messageStyle.Sprintf("%s\n", strings.Repeat("~", underlineLength))) + b.writeStyledLine(messageStyle, "%s\n", strings.Repeat("~", underlineLength)) - b.result.WriteString(lineStyle.Sprintf("%s= ", b.padding)) - b.result.WriteString(messageStyle.Sprintf("%s\n\n", b.issue.Message)) + b.writeStyledLine(lineStyle, "%s= ", b.padding) + b.writeStyledLine(messageStyle, "%s\n\n", b.issue.Message) return b } -func (b *IssueFormatterBuilder) AddMessage() *IssueFormatterBuilder { - b.result.WriteString(messageStyle.Sprint(b.issue.Message)) - b.result.WriteString("\n\n") +func (b *issueFormatterBuilder) AddMessage() *issueFormatterBuilder { + b.writeStyledLine(messageStyle, "%s\n\n", b.issue.Message) return b } -func (b *IssueFormatterBuilder) AddSuggestion() *IssueFormatterBuilder { +func (b *issueFormatterBuilder) AddSuggestion() *issueFormatterBuilder { if b.issue.Suggestion == "" { return b } - b.result.WriteString(suggestionStyle.Sprint("Suggestion:\n")) - b.result.WriteString(lineStyle.Sprintf("%s|\n", b.padding)) + b.writeStyledLine(suggestionStyle, "Suggestion:\n") + b.writeStyledLine(lineStyle, "%s|\n", b.padding) suggestionLines := strings.Split(b.issue.Suggestion, "\n") for i, line := range suggestionLines { lineNum := fmt.Sprintf("%*d", b.maxLineNumWidth, b.issue.Start.Line+i) - b.result.WriteString(lineStyle.Sprintf("%s | ", lineNum)) - b.result.WriteString(line + "\n") + b.writeStyledLine(lineStyle, "%s | %s\n", lineNum, line) } - b.result.WriteString(lineStyle.Sprintf("%s|\n", b.padding)) - b.result.WriteString("\n") + b.writeStyledLine(lineStyle, "%s|\n\n", b.padding) return b } -func (b *IssueFormatterBuilder) AddNote() *IssueFormatterBuilder { +func (b *issueFormatterBuilder) AddNote() *issueFormatterBuilder { if b.issue.Note == "" { return b } b.result.WriteString(suggestionStyle.Sprint("Note: ")) - b.result.WriteString(b.issue.Note) - b.result.WriteString("\n\n") + b.writeStyledLine(lineStyle, "%s\n\n", b.issue.Note) return b } +func (b *issueFormatterBuilder) writeStyledLine(style *color.Color, format string, a ...interface{}) { + b.result.WriteString(style.Sprintf(format, a...)) +} + type BaseFormatter struct{} -func (b *IssueFormatterBuilder) Build() string { +func (b *issueFormatterBuilder) Build() string { return b.result.String() } -func (b *IssueFormatterBuilder) isValidLineRange() bool { +func (b *issueFormatterBuilder) isValidLineRange() bool { return b.startLine > 0 && b.endLine > 0 && b.startLine <= b.endLine && @@ -275,7 +256,7 @@ func findCommonIndent(lines []string) string { } // find first non-empty line's indent - var firstIndent []rune + firstIndent := make([]rune, 0) for _, line := range lines { trimmed := strings.TrimLeftFunc(line, unicode.IsSpace) if trimmed != "" { diff --git a/formatter/cyclomatic_complexity.go b/formatter/cyclomatic_complexity.go index 05b3a5e..e516329 100644 --- a/formatter/cyclomatic_complexity.go +++ b/formatter/cyclomatic_complexity.go @@ -11,7 +11,7 @@ import ( type CyclomaticComplexityFormatter struct{} func (f *CyclomaticComplexityFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) + builder := newIssueFormatterBuilder(issue, snippet) return builder. AddHeader(). AddCodeSnippet(). @@ -21,7 +21,7 @@ func (f *CyclomaticComplexityFormatter) Format(issue tt.Issue, snippet *internal Build() } -func (b *IssueFormatterBuilder) AddComplexityInfo() *IssueFormatterBuilder { +func (b *issueFormatterBuilder) AddComplexityInfo() *issueFormatterBuilder { maxLineNumWidth := calculateMaxLineNumWidth(b.issue.End.Line) padding := strings.Repeat(" ", maxLineNumWidth+1) diff --git a/formatter/defers.go b/formatter/defers.go deleted file mode 100644 index 3c08fd8..0000000 --- a/formatter/defers.go +++ /dev/null @@ -1,18 +0,0 @@ -package formatter - -import ( - "github.com/gnolang/tlin/internal" - tt "github.com/gnolang/tlin/internal/types" -) - -type DefersFormatter struct{} - -func (f *DefersFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) - return builder. - AddHeader(). - AddCodeSnippet(). - AddUnderlineAndMessage(). - AddNote(). - Build() -} diff --git a/formatter/deprecated.go b/formatter/deprecated.go deleted file mode 100644 index 3d5493c..0000000 --- a/formatter/deprecated.go +++ /dev/null @@ -1,18 +0,0 @@ -package formatter - -import ( - "github.com/gnolang/tlin/internal" - tt "github.com/gnolang/tlin/internal/types" -) - -type DeprecatedFuncFormatter struct{} - -func (f *DeprecatedFuncFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) - return builder. - AddHeader(). - AddCodeSnippet(). - AddUnderlineAndMessage(). - AddNote(). - Build() -} diff --git a/formatter/early_return.go b/formatter/early_return.go deleted file mode 100644 index 5204535..0000000 --- a/formatter/early_return.go +++ /dev/null @@ -1,19 +0,0 @@ -package formatter - -import ( - "github.com/gnolang/tlin/internal" - tt "github.com/gnolang/tlin/internal/types" -) - -type EarlyReturnOpportunityFormatter struct{} - -func (f *EarlyReturnOpportunityFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) - return builder. - AddHeader(). - AddCodeSnippet(). - AddUnderlineAndMessage(). - AddSuggestion(). - AddNote(). - Build() -} diff --git a/formatter/format_emit.go b/formatter/format_emit.go deleted file mode 100644 index 28d8889..0000000 --- a/formatter/format_emit.go +++ /dev/null @@ -1,19 +0,0 @@ -package formatter - -import ( - "github.com/gnolang/tlin/internal" - tt "github.com/gnolang/tlin/internal/types" -) - -type EmitFormatFormatter struct{} - -func (f *EmitFormatFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) - return builder. - AddHeader(). - AddCodeSnippet(). - AddUnderlineAndMessage(). - AddSuggestion(). - AddNote(). - Build() -} diff --git a/formatter/formatter_test.go b/formatter/formatter_test.go index 523034c..8639077 100644 --- a/formatter/formatter_test.go +++ b/formatter/formatter_test.go @@ -163,7 +163,7 @@ error: example func TestUnnecessaryTypeConversionFormatter(t *testing.T) { t.Parallel() - formatter := &UnnecessaryTypeConversionFormatter{} + formatter := &GeneralIssueFormatter{} issue := tt.Issue{ Rule: "unnecessary-type-conversion", diff --git a/formatter/general.go b/formatter/general.go index bb4b263..7de030d 100644 --- a/formatter/general.go +++ b/formatter/general.go @@ -14,10 +14,12 @@ func (f *GeneralIssueFormatter) Format( issue tt.Issue, snippet *internal.SourceCode, ) string { - builder := NewIssueFormatterBuilder(issue, snippet) + builder := newIssueFormatterBuilder(issue, snippet) return builder. AddHeader(). AddCodeSnippet(). AddUnderlineAndMessage(). + AddSuggestion(). + AddNote(). Build() } diff --git a/formatter/missing_mod_pacakge.go b/formatter/missing_mod_pacakge.go index 3a29eba..77d8982 100644 --- a/formatter/missing_mod_pacakge.go +++ b/formatter/missing_mod_pacakge.go @@ -8,7 +8,7 @@ import ( type MissingModPackageFormatter struct{} func (f *MissingModPackageFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) + builder := newIssueFormatterBuilder(issue, snippet) return builder. AddHeader(). AddMessage(). diff --git a/formatter/simplify_slice_expr.go b/formatter/simplify_slice_expr.go deleted file mode 100644 index f7942cd..0000000 --- a/formatter/simplify_slice_expr.go +++ /dev/null @@ -1,19 +0,0 @@ -package formatter - -import ( - "github.com/gnolang/tlin/internal" - tt "github.com/gnolang/tlin/internal/types" -) - -type SimplifySliceExpressionFormatter struct{} - -func (f *SimplifySliceExpressionFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) - return builder. - AddHeader(). - AddCodeSnippet(). - AddUnderlineAndMessage(). - AddSuggestion(). - AddNote(). - Build() -} diff --git a/formatter/slice_bound.go b/formatter/slice_bound.go index 14b69a8..2ff1e76 100644 --- a/formatter/slice_bound.go +++ b/formatter/slice_bound.go @@ -11,7 +11,7 @@ func (f *SliceBoundsCheckFormatter) Format( issue tt.Issue, snippet *internal.SourceCode, ) string { - builder := NewIssueFormatterBuilder(issue, snippet) + builder := newIssueFormatterBuilder(issue, snippet) return builder. AddHeader(). AddCodeSnippet(). @@ -20,7 +20,7 @@ func (f *SliceBoundsCheckFormatter) Format( Build() } -func (b *IssueFormatterBuilder) AddWarning() *IssueFormatterBuilder { +func (b *issueFormatterBuilder) AddWarning() *issueFormatterBuilder { b.result.WriteString(warningStyle.Sprint("warning: ")) if b.issue.Category == "index-access" { b.result.WriteString("Index access without bounds checking can lead to runtime panics.\n") diff --git a/formatter/unnecessary_type_conv.go b/formatter/unnecessary_type_conv.go deleted file mode 100644 index aa5020d..0000000 --- a/formatter/unnecessary_type_conv.go +++ /dev/null @@ -1,19 +0,0 @@ -package formatter - -import ( - "github.com/gnolang/tlin/internal" - tt "github.com/gnolang/tlin/internal/types" -) - -type UnnecessaryTypeConversionFormatter struct{} - -func (f *UnnecessaryTypeConversionFormatter) Format(issue tt.Issue, snippet *internal.SourceCode) string { - builder := NewIssueFormatterBuilder(issue, snippet) - return builder. - AddHeader(). - AddCodeSnippet(). - AddUnderlineAndMessage(). - AddSuggestion(). - AddNote(). - Build() -} diff --git a/internal/lints/const_error_decl.go b/internal/lints/const_error_decl.go index 7e59b06..9ecca65 100644 --- a/internal/lints/const_error_decl.go +++ b/internal/lints/const_error_decl.go @@ -3,6 +3,8 @@ package lints import ( "go/ast" "go/token" + "os" + "strings" tt "github.com/gnolang/tlin/internal/types" ) @@ -15,6 +17,11 @@ func DetectConstErrorDeclaration( ) ([]tt.Issue, error) { var issues []tt.Issue + src, err := os.ReadFile(filename) + if err != nil { + return nil, err + } + ast.Inspect(node, func(n ast.Node) bool { genDecl, ok := n.(*ast.GenDecl) if !ok || genDecl.Tok != token.CONST { @@ -40,13 +47,19 @@ func DetectConstErrorDeclaration( } if containsErrorsNew { + startPos := fset.Position(genDecl.Pos()).Offset + endPos := fset.Position(genDecl.End()).Offset + origSnippet := src[startPos:endPos] + + suggestion := strings.Replace(string(origSnippet), "const", "var", 1) + issue := tt.Issue{ Rule: "const-error-declaration", Filename: filename, Start: fset.Position(genDecl.Pos()), End: fset.Position(genDecl.End()), - Message: "Constant declaration of errors.New() is not allowed", - Suggestion: "Use var instead of const for error declarations", + Message: "Avoid declaring constant errors", + Suggestion: suggestion, Confidence: 1.0, Severity: severity, } diff --git a/internal/lints/lint_test.go b/internal/lints/lint_test.go index 53df957..8fdfc7c 100644 --- a/internal/lints/lint_test.go +++ b/internal/lints/lint_test.go @@ -92,7 +92,12 @@ func main() { issues, err := DetectUnnecessarySliceLength(tmpfile, node, fset, types.SeverityError) require.NoError(t, err) - assert.Equal(t, tt.expected, len(issues), "Number of detected unnecessary slice length doesn't match expected") + assert.Equal( + t, + tt.expected, + len(issues), + "Number of detected unnecessary slice length doesn't match expected", + ) if len(issues) > 0 { for _, issue := range issues { @@ -196,7 +201,12 @@ func example() { issues, err := DetectUnnecessaryConversions(tmpfile, node, fset, types.SeverityError) require.NoError(t, err) - assert.Equal(t, tt.expected, len(issues), "Number of detected unnecessary type conversions doesn't match expected") + assert.Equal( + t, + tt.expected, + len(issues), + "Number of detected unnecessary type conversions doesn't match expected", + ) if len(issues) > 0 { for _, issue := range issues { @@ -364,7 +374,12 @@ func TestDetectEmitFormat(t *testing.T) { issues, err := DetectEmitFormat(tmpfile, node, fset, types.SeverityError) require.NoError(t, err) - assert.Equal(t, tt.expected, len(issues), fmt.Sprintf("Number of detected issues doesn't match expected for %s. %v", tt.filename, issues)) + assert.Equal( + t, + tt.expected, + len(issues), + fmt.Sprintf("Number of detected issues doesn't match expected for %s. %v", tt.filename, issues), + ) if len(issues) > 0 { assert.Equal(t, "emit-format", issues[0].Rule) @@ -616,7 +631,12 @@ outer: issues, err := DetectUselessBreak("test.go", node, fset, types.SeverityError) require.NoError(t, err) - assert.Equal(t, tt.expected, len(issues), "Number of detected useless break statements doesn't match expected") + assert.Equal( + t, + tt.expected, + len(issues), + "Number of detected useless break statements doesn't match expected", + ) if len(issues) > 0 { for _, issue := range issues { @@ -677,18 +697,32 @@ var err = errors.New("error") tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - fset := token.NewFileSet() - node, err := parser.ParseFile(fset, "test.go", tt.code, parser.ParseComments) + + tmpDir, err := os.MkdirTemp("", "lint-test") require.NoError(t, err) + defer os.RemoveAll(tmpDir) - issues, err := DetectConstErrorDeclaration("test.go", node, fset, types.SeverityError) + tmpfile := filepath.Join(tmpDir, "test.go") + err = os.WriteFile(tmpfile, []byte(tt.code), 0o644) require.NoError(t, err) - assert.Equal(t, tt.expected, len(issues), "Number of detected constant error declarations doesn't match expected") + node, fset, err := ParseFile(tmpfile, nil) + require.NoError(t, err) + + issues, err := DetectConstErrorDeclaration(tmpfile, node, fset, types.SeverityError) + require.NoError(t, err) + + assert.Equal( + t, + tt.expected, + len(issues), + "Number of detected constant error declarations doesn't match expected", + ) for _, issue := range issues { assert.Equal(t, "const-error-declaration", issue.Rule) - assert.Contains(t, issue.Message, "Constant declaration of errors.New() is not allowed") + assert.Contains(t, issue.Message, "Avoid declaring constant errors") + assert.Contains(t, issue.Suggestion, "var") } }) }