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.

Sign up for GitHub

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

Jump to bottom

check clear/max/min in redefines-builtin-id rule on go1.21+ #1024

Merged
merged 2 commits into from
Aug 15, 2024
Merged

check clear/max/min in redefines-builtin-id rule on go1.21+ #1024

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81854e5
check clear/max/min in redefines-builtin-id rule on go1.21+
skaji Aug 14, 2024
67857ce
Update rule/redefines-builtin-id.go
skaji Aug 14, 2024
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
6 changes: 6 additions & 0 deletions lint/package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ var (
falseValue = 2
notSet = 3

go121 = goversion.Must(goversion.NewVersion("1.21"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering: isn't go 1.21 already EOL? Because if it is, we should better use the existing go122 constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't go 1.21 already EOL?

Yes, go 1.21 is EOL.

Because if it is, we should better use the existing go122 constant.

Does this mean "revive does not care about EOL go"?
Then, I think we should just add clear/max/min to builtFunctions unconditionaly.

However, I'm not unwilling to use go122 / IsAtLeastGo122 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, I think we should just add clear/max/min to builtFunctions unconditionaly.

👍

Suggesting to avoid using function named max/min is not really a problem, even for version under go 1.21. They could be renamed even if max/min is not available

Copy link
Collaborator

Choose a reason for hiding this comment

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

revive itself requires Go 1.21 and it will eventually require 1.22 because 1.21 is EOL but it should be possible to use revive to analyze Go code bases of any version.

go122 = goversion.Must(goversion.NewVersion("1.22"))
)

Expand Down Expand Up @@ -194,6 +195,11 @@ func (p *Package) lint(rules []Rule, config Config, failures chan Failure) {
wg.Wait()
}

// IsAtLeastGo121 returns true if the Go version for this package is 1.21 or higher, false otherwise
func (p *Package) IsAtLeastGo121() bool {
return p.goVersion.GreaterThanOrEqual(go121)
}

// IsAtLeastGo122 returns true if the Go version for this package is 1.22 or higher, false otherwise
func (p *Package) IsAtLeastGo122() bool {
return p.goVersion.GreaterThanOrEqual(go122)
Expand Down
32 changes: 26 additions & 6 deletions rule/redefines-builtin-id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"go/ast"
"go/token"
"maps"

"github.com/mgechev/revive/lint"
)
Expand Down Expand Up @@ -33,6 +34,12 @@ var builtFunctions = map[string]bool{
"recover": true,
}

var builtFunctionsAfterGo121 = map[string]bool{
"clear": true,
"max": true,
"min": true,
}

var builtInTypes = map[string]bool{
"bool": true,
"byte": true,
Expand Down Expand Up @@ -69,7 +76,17 @@ func (*RedefinesBuiltinIDRule) Apply(file *lint.File, _ lint.Arguments) []lint.F
}

astFile := file.AST
w := &lintRedefinesBuiltinID{onFailure}

builtFuncs := maps.Clone(builtFunctions)
if file.Pkg.IsAtLeastGo121() {
maps.Copy(builtFuncs, builtFunctionsAfterGo121)
}
w := &lintRedefinesBuiltinID{
onFailure: onFailure,
builtInConstAndVars: builtInConstAndVars,
builtFunctions: builtFuncs,
builtInTypes: builtInTypes,
}
ast.Walk(w, astFile)

return failures
Expand All @@ -81,7 +98,10 @@ func (*RedefinesBuiltinIDRule) Name() string {
}

type lintRedefinesBuiltinID struct {
onFailure func(lint.Failure)
onFailure func(lint.Failure)
builtInConstAndVars map[string]bool
builtFunctions map[string]bool
builtInTypes map[string]bool
}

func (w *lintRedefinesBuiltinID) Visit(node ast.Node) ast.Visitor {
Expand Down Expand Up @@ -162,16 +182,16 @@ func (w lintRedefinesBuiltinID) addFailure(node ast.Node, msg string) {
})
}

func (lintRedefinesBuiltinID) isBuiltIn(id string) (r bool, builtInKind string) {
if builtFunctions[id] {
func (w lintRedefinesBuiltinID) isBuiltIn(id string) (r bool, builtInKind string) {
skaji marked this conversation as resolved.
Show resolved Hide resolved
if w.builtFunctions[id] {
return true, "function"
}

if builtInConstAndVars[id] {
if w.builtInConstAndVars[id] {
return true, "constant or variable"
}

if builtInTypes[id] {
if w.builtInTypes[id] {
return true, "type"
}

Expand Down
9 changes: 9 additions & 0 deletions testdata/redefines-builtin-id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,12 @@ var i, copy int // MATCH /redefinition of the built-in function copy/

// issue #792
type ()

func foo() {
clear := 0 // MATCH /redefinition of the built-in function clear/
Copy link
Contributor Author

@skaji skaji Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

revive itself requires go1.21. So I don't add any condition for go version here.
https://github.com/mgechev/revive/blob/master/go.mod#L3

I manually confirmed that the following code does not report any errors:

go.mod:

module playground

go 1.20

main.go

// package comment
package main

func main() {
	clear := 0
	_ = clear
}
❯ revive -set_exit_status

❯ echo $?
0

max := 0 // MATCH /redefinition of the built-in function max/
min := 0 // MATCH /redefinition of the built-in function min/
_ = clear
_ = max
_ = min
}