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: check argument count of strings.NewReplacer #47994

Closed
frogtile opened this issue Aug 26, 2021 · 9 comments
Closed

proposal: cmd/vet: check argument count of strings.NewReplacer #47994

frogtile opened this issue Aug 26, 2021 · 9 comments
Labels
Projects
Milestone

Comments

@frogtile
Copy link

@frogtile frogtile commented Aug 26, 2021

Docs says,

NewReplacer panics if given an odd number of arguments.

Currently vet doesn't check if NewReplacer has an odd number of arguments

Reason for having NewReplacer's parameters this way was answered here

@gopherbot gopherbot added this to the Proposal milestone Aug 26, 2021
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Aug 26, 2021

If there is a vet check , it would make sense to start by only handling cases when the number of arguments is locally known.

_ = strings.NewReplacer("a", "b", "c")

or

list := []string{"lorem ipsum"}
_ = strings.NewReplacer(list...)

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 30, 2021

Does this really merit a vet warning? strings.NewReplacer already panics at run-time with a pretty clear error message (https://play.golang.org/p/bUQg60lQCJZ), so having any test coverage at all for the call would uncover the bug when the number of arguments is easy to determine.

It seems like this check would be most useful when strings.NewReplacer is called with a non-trivial slice obtained from somewhere else, but those cases are also much more difficult for a vet analyzer to find.

Loading

@guodongli-google
Copy link

@guodongli-google guodongli-google commented Sep 4, 2021

The main check will be on the variadic argument case. My preliminary study of some code base indicates the following common patterns:

var r []string
for i := 0; i <= k; i++ {
    r = append(r, something)
}
strings.NewReplacer(r...)

and

var r []string
for i := range m {
     r = append(r, something)
 }
strings.NewReplacer(r...)

where "k" and "m" usually come from function arguments. To make it work we will need to reason about "k" and the size of "m" through inter-procedural analysis, which is probably out of the scope of vet.

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Sep 8, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

This seems too specialized and with too little benefit.

Loading

@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented Sep 15, 2021

It is also not clear if the pattern described by @guodongli-google satisfies the requirement of high enough frequency.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Incoming to Active in Proposals Oct 13, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Active to Likely Decline in Proposals Oct 20, 2021
@frogtile
Copy link
Author

@frogtile frogtile commented Oct 26, 2021

Can't comment since the primary reason for this issue is the parameter type.

Loading

@frogtile frogtile closed this Oct 26, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals Oct 27, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants