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

proposal: cmd/vet: reject unary min and max calls #66179

Open
dsnet opened this issue Mar 8, 2024 · 6 comments
Open

proposal: cmd/vet: reject unary min and max calls #66179

dsnet opened this issue Mar 8, 2024 · 6 comments
Labels
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Mar 8, 2024

Proposal Details

A unary min and max call is an identity function and its presence suggests a bug.

I had code that was doing the following:

blockSize = min(max(r.minBlockSize, blockSize, r.maxBlockSize))

where the intent is to clamp blockSize within a certain range.

However, due to a single misplaced ), this does not work.
It should have been:

blockSize = min(max(r.minBlockSize, blockSize), r.maxBlockSize)

I propose that go vet flag any unary calls to min or max, which would have caught my bug where I had 3 arguments passed to max, but only 1 argument passed to min.

@dsnet dsnet added the Proposal label Mar 8, 2024
@gopherbot gopherbot added this to the Proposal milestone Mar 8, 2024
@ianlancetaylor
Copy link
Contributor

CC @golang/tools-team

@leaxoy
Copy link

leaxoy commented Mar 8, 2024

Maybe I don't have relevant contextual information, why is the max function allowed to accept a parameter, and what is its actual meaning? Is there any real scenario where only one parameter needs to be passed?

@seankhliao
Copy link
Member

the function signature is max[T cmp.Ordered](x T, y ...T) T

@go101
Copy link

go101 commented Mar 8, 2024

@leaxoy

why is the max function allowed to accept a parameter, and what is its actual meaning? Is there any real scenario where only one parameter needs to be passed?

At least, min/max can be used as poor men's eval of ordered expressions. For example,
in the following code, the foo call may return 3 or 4, but the bar call is guaranteed to return 3.

package main

func foo(k int) int {
	f := func() int {
		k++
		return k
	}
	return k + f()
}

func bar(k int) int {
	f := func() int {
		k++
		return k
	}
	return min(k) + f()
}

func main() {
	println(foo(1), bar(1))
}

:D

Similarly, append can be used poor men's eval of slice expressions: https://go.dev/play/p/9T0KcZaYtuY

@adonovan
Copy link
Member

adonovan commented Mar 8, 2024

This seems closely analogous to #60448, which added a similar vet check for append. (Too bad we called it append and not missingarg or something more general...)

@AlexanderYastrebov
Copy link
Contributor

Maybe I don't have relevant contextual information, why is the max function allowed to accept a parameter, and what is its actual meaning? Is there any real scenario where only one parameter needs to be passed?

This was raised during discussion #59488 (comment) and there was no obvious reason to prohibit it #59488 (comment)
Now there is a reason.

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

No branches or pull requests

8 participants