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

feat: Prevent Constant Error Declaration #94

Merged
merged 4 commits into from
Oct 18, 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
101 changes: 101 additions & 0 deletions docs/rfc/const-error-decl.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# RFC: Constant Error Declaration Rule

## Summary

This rule detects and reports the use of `const` for declaring errors created with `errors.New()`. The rule enforces the usage of `var` for error declarations, preventing compilation issues that arise when using `const` for error values.

## Motivation

The result of `errors.New()` is not a constant value, even though the string passed to it is constant. Using `const` for such declarations can lead to compilation errors. This rule helps developers avoid this common mistake and use `var` for error declarations instead.

## Implementation

The rule will scan all `const` declarations in the code and check if any of them use `errors.New()`. If found, it will report an issue suggesting to use `var` instead.

### Rule Details

- **Rule ID**: const-error-declaration
- **Severity**: error
- **Category**: bug prevention, best practices
- **Auto-fixable**: Yes

### Code Examples

#### Incorrect

**Case 1** Single error in a single const declaration

```go
const err = errors.New("error")
```

Output

```plain
error: const-error-declaration
--> foo.gno
|
5 | const err = errors.New("error")
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
= Constant declaration of errors.New() is not allowed
```

**Case 2** Multiple errors in a single const declaration

```go
const (
err1 = errors.New("error1")
err2 = errors.New("error2")
)
```

Output

```plain
error: const-error-declaration
--> foo.gno
|
5 | const (
6 | err1 = errors.New("error")
7 | err2 = errors.New("error2")
8 | )
| ~~
= Constant declaration of errors.New() is not allowed
```

#### Correct

**Case 1** Single error in a single var declaration

```go
var err = errors.New("error")
```

**Case 2** Multiple errors in a single var declaration

```go
var (
err1 = errors.New("error1")
err2 = errors.New("error2")
)
```

### Exceptions

There are no exceptions to this rule. All `const` declarations using `errors.New()` should be reported.

## Alternatives

An alternative approach could be to allow `const` declarations of errors but wrap them in a function call that returns an error interface. However, this approach is less idiomatic in Go and may lead to confusion.

## Implementation Impact

Positive impacts:

- Prevents compilation errors
- Encourages correct usage of error declarations
- Improves code consistency

Negative impacts:

- Potential migration costs could arise
4 changes: 2 additions & 2 deletions docs/rfc/template.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@

### Code Examples

#### Incorrect:
#### Incorrect

[Example of code that violates the rule]

#### Correct:
#### Correct

[Example of code that does not violate the rule]

Expand Down
1 change: 1 addition & 0 deletions internal/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ var allRuleConstructors = ruleMap{
"defer-issues": NewDeferRule,
"gno-mod-tidy": NewMissingModPackageRule,
"slice-bounds-check": NewSliceBoundCheckRule,
"const-error-declaration": NewConstErrorDeclarationRule,
}

func (e *Engine) applyRules(rules map[string]tt.ConfigRule) {
Expand Down
79 changes: 79 additions & 0 deletions internal/lints/const_error_decl.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package lints

import (
"go/ast"
"go/token"

tt "github.com/gnolang/tlin/internal/types"
)

func DetectConstErrorDeclaration(
filename string,
node *ast.File,
fset *token.FileSet,
severity tt.Severity,
) ([]tt.Issue, error) {
var issues []tt.Issue

ast.Inspect(node, func(n ast.Node) bool {
genDecl, ok := n.(*ast.GenDecl)
if !ok || genDecl.Tok != token.CONST {
return true
}

containsErrorsNew := false
for _, spec := range genDecl.Specs {
valueSpec, ok := spec.(*ast.ValueSpec)
if !ok {
continue
}

for _, value := range valueSpec.Values {
if isErrorNew(value) {
containsErrorsNew = true
break
}
}
if containsErrorsNew {
break
}
}

if containsErrorsNew {
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",
Confidence: 1.0,
Severity: severity,
}
issues = append(issues, issue)
}

return true
})

return issues, nil
}

func isErrorNew(expr ast.Expr) bool {
callExpr, ok := expr.(*ast.CallExpr)
if !ok {
return false
}

selector, ok := callExpr.Fun.(*ast.SelectorExpr)
if !ok {
return false
}

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

return selector.Sel.Name == "New" && ident.Name == "errors"
}
66 changes: 66 additions & 0 deletions internal/lints/lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -627,3 +627,69 @@ outer:
})
}
}

func TestDetectConstErrorDeclaration(t *testing.T) {
t.Parallel()
tests := []struct {
name string
code string
expected int
}{
{
name: "Constant error declaration",
code: `
package main

import "errors"

const err = errors.New("error")
`,
expected: 1,
},
{
name: "Constant error declaration with multiple errors",
code: `
package main

import "errors"

const (
err1 = errors.New("error1")
err2 = errors.New("error2")
)
`,
expected: 1,
},
{
name: "Variable error declaration",
code: `
package main

import "errors"

var err = errors.New("error")
`,
expected: 0,
},
}

for _, tt := range tests {
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)
require.NoError(t, err)

issues, err := DetectConstErrorDeclaration("test.go", 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")
}
})
}
}
26 changes: 26 additions & 0 deletions internal/rule_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,32 @@ func (r *MissingModPackageRule) SetSeverity(severity tt.Severity) {
r.severity = severity
}

type ConstErrorDeclarationRule struct {
severity tt.Severity
}

func NewConstErrorDeclarationRule() LintRule {
return &ConstErrorDeclarationRule{
severity: tt.SeverityError,
}
}

func (r *ConstErrorDeclarationRule) Check(filename string, node *ast.File, fset *token.FileSet) ([]tt.Issue, error) {
return lints.DetectConstErrorDeclaration(filename, node, fset, r.severity)
}

func (r *ConstErrorDeclarationRule) Name() string {
return "const-error-declaration"
}

func (r *ConstErrorDeclarationRule) SetSeverity(severity tt.Severity) {
r.severity = severity
}

func (r *ConstErrorDeclarationRule) Severity() tt.Severity {
return r.severity
}

// -----------------------------------------------------------------------------
// Regex related rules

Expand Down
9 changes: 9 additions & 0 deletions testdata/const-error-decl/const_decl.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package main

import "errors"

const err = errors.New("error")

func main() {
println("foo")
}
12 changes: 12 additions & 0 deletions testdata/const-error-decl/const_decl_multiple.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package main

import "errors"

const (
err = errors.New("error")
err2 = errors.New("error2")
)

func main() {
println("foo")
}
9 changes: 9 additions & 0 deletions testdata/const-error-decl/var_decl.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package main

import "errors"

var err = errors.New("error")

func main() {
println("foo")
}
Loading