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

Request: report "redefinition of the built-in" errors for function related variables #1022

Closed
skaji opened this issue Aug 11, 2024 · 4 comments · Fixed by #1023
Closed

Request: report "redefinition of the built-in" errors for function related variables #1022

skaji opened this issue Aug 11, 2024 · 4 comments · Fixed by #1023

Comments

@skaji
Copy link
Contributor

skaji commented Aug 11, 2024

Is your feature request related to a problem? Please describe.

Currently, function related variables are not checked by redefinition of the built-in ....
It would be nice if revive reports redefinition of the built-in ... errors for function related variables.

For example, revive does not report any errors for the following main.go file:

// This is a package comment
package main

func main() {
}

// 1 param
func foo1(new int) {
	_ = new
}

// 2 result
func foo2() (new int) {
	return
}

// 3 type param
func foo3[new any]() {
}
❯ revive -set_exit_status main.go

❯ echo $?
0

Describe the solution you'd like

revive reports redefinition of the built-in ... errors for function related variables, so that:

❯ revive -set_exit_status main.go
main.go:8:11: redefinition of the built-in function new
main.go:13:14: redefinition of the built-in function new
main.go:18:11: redefinition of the built-in function new

❯ echo $?
1

Describe alternatives you've considered

NA

Additional context

I'm not completely certain, but I believe that by making the following changes, this feature can be implemented.

diff --git rule/redefines-builtin-id.go rule/redefines-builtin-id.go
index b3ff084..d0870fd 100644
--- rule/redefines-builtin-id.go
+++ rule/redefines-builtin-id.go
@@ -125,6 +125,29 @@ func (w *lintRedefinesBuiltinID) Visit(node ast.Node) ast.Visitor {
 		if ok, bt := w.isBuiltIn(id); ok {
 			w.addFailure(n, fmt.Sprintf("redefinition of the built-in %s %s", bt, id))
 		}
+	case *ast.FuncType:
+		var fields []*ast.Field
+		if n.TypeParams != nil {
+			fields = append(fields, n.TypeParams.List...)
+		}
+		if n.Params != nil {
+			fields = append(fields, n.Params.List...)
+		}
+		if n.Results != nil {
+			fields = append(fields, n.Results.List...)
+		}
+		for _, field := range fields {
+			for _, name := range field.Names {
+				if obj := name.Obj; obj != nil {
+					if obj.Kind == ast.Var || obj.Kind == ast.Typ {
+						id := obj.Name
+						if ok, bt := w.isBuiltIn(id); ok {
+							w.addFailure(name, fmt.Sprintf("redefinition of the built-in %s %s", bt, id))
+						}
+					}
+				}
+			}
+		}
 	case *ast.AssignStmt:
 		for _, e := range n.Lhs {
 			id, ok := e.(*ast.Ident)
@chavacava
Copy link
Collaborator

Hi @skaji thank you for the proposal. Feel free to make a PR.

@ccoVeille
Copy link
Contributor

ccoVeille commented Aug 12, 2024

It would be a good thing, because many projects overloaded min and max, they could be reported now they are available

@ccoVeille
Copy link
Contributor

ccoVeille commented Aug 12, 2024

Isn't #1021 linked with #1022 then ?

Not because of my previous comment, but because the fix you suggest would also identify min/max function?

@ccoVeille
Copy link
Contributor

Oh,now I get it. One is about overloading function with a function, one is about overloading a function with a variable (via function argument)

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 a pull request may close this issue.

3 participants