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 new rule time equal #584

Merged
merged 6 commits into from
Oct 1, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ var allRules = append([]lint.Rule{
&rule.NestedStructs{},
&rule.IfReturnRule{},
&rule.UselessBreak{},
&rule.TimeEqualRule{},
}, defaultRules...)

var allFormatters = []lint.Formatter{
Expand Down
70 changes: 70 additions & 0 deletions rule/time-equal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package rule

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

"github.com/mgechev/revive/lint"
)

// TimeEqualRule shows where "==" and "!=" used for equality check time.Time
type TimeEqualRule struct{}

// Apply applies the rule to given file.
func (*TimeEqualRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
var failures []lint.Failure

onFailure := func(failure lint.Failure) {
failures = append(failures, failure)
}

w := &lintTimeEqual{file, onFailure}
w.file.Pkg.TypeCheck()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeCheck might return an error, need to check for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the comments, I agree but saw that this error has not been checked in other files too.

Copy link
Collaborator

@chavacava chavacava Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error has not been checked in other files too.

This is something that need to be fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error has not been checked in other files too.

This is something that need to be fixed

well, i thinks return nil is good , because the Apply method not returns any error.


ast.Walk(w, file.AST)
return failures
}

// Name returns the rule name.
func (*TimeEqualRule) Name() string {
return "time-equal"
}

type lintTimeEqual struct {
file *lint.File
onFailure func(lint.Failure)
}

func (l *lintTimeEqual) Visit(node ast.Node) ast.Visitor {
expr, ok := node.(*ast.BinaryExpr)
if !ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we could check if the opperators are == or !=, if is not the case we can return early.

return l
}

xtyp := l.file.Pkg.TypeOf(expr.X)
ytyp := l.file.Pkg.TypeOf(expr.Y)

if !isNamedType(xtyp, "time", "Time") || !isNamedType(ytyp, "time", "Time") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add constants for "time" and "Time"

return l
}

var failure string
switch expr.Op {
case token.EQL:
failure = fmt.Sprintf("use %s.Equal(%s) instead of %q operator", expr.X, expr.Y, expr.Op)
case token.NEQ:
failure = fmt.Sprintf("use !%s.Equal(%s) instead of %q operator", expr.X, expr.Y, expr.Op)
default:
return l
}

l.onFailure(lint.Failure{
Category: "time",
Confidence: 1,
Node: node,
Failure: failure,
})

return l
}
12 changes: 12 additions & 0 deletions test/time-equal_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package test

import (
"testing"

"github.com/mgechev/revive/rule"
)

// TestTimeEqual rule.
func TestTimeEqual(t *testing.T) {
testRule(t, "time-equal", &rule.TimeEqualRule{})
}
27 changes: 27 additions & 0 deletions testdata/time-equal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package pkg
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After configuring revive to only apply time-equal rule, when doing

./revive -config defaults.toml -formatter friendly testdata/time-equal.go

No failure is found.
This might be due to (silent) type checking errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, it works for me 🤔 😬


import "time"

func eqlOp() bool {
t := time.Now()
u := t
return t == u // MATCH /use t.Equal(u) instead of "==" operator/
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests for other operators ( !=, <, ...) can be done in a single function


func eqlFun() bool {
t := time.Now()
u := t
return t.Equal(t)
}

func neqlOp() bool {
t := time.Now()
u := t.Add(2 * time.Second)
return t != u // MATCH /use !t.Equal(u) instead of "!=" operator/
}

func neqlFun() bool {
t := time.Now()
u := t.Sub(2 * time.Second)
return !t.Equal(t)
}