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

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

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

skaji
Copy link
Contributor

@skaji skaji commented Aug 14, 2024

Close #1021

go1.21 has introduced new functions clear, max and min.
It would be nice revive checks these functions in redefines-builtin-id rule on go1.21+.

@@ -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

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

@@ -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.

@chavacava chavacava merged commit e54773e into mgechev:master Aug 15, 2024
4 checks passed
@skaji
Copy link
Contributor Author

skaji commented Aug 15, 2024

@denisvmedia @ccoVeille @chavacava
Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: treat min, max and clear as built-in functions
4 participants