go: cmd/vet: report strings.Trim/TrimLeft/TrimRight with duplicate runes in cutset
go vet should ideally report likely misuses of standard library functions especially when it comes to providing constant(-ish) string arguments, e.g. (note: the following list is absolutely not exhaustive)
strings.(Trim|TrimLeft|TrimRight)with acutsetcontaining duplicated runesnet/http.NewRequestwith an invalid HTTPmethodor a non-HTTPurlnet/http.(Do|Get|Post)with a non-HTTPurlregexp.(Must)?Compilewith an invalidregexp(text|html)/template.Template.Parsewith an invalid templatetext- (and so on, for all functions/structs where it makes sense; not all of them need to be added right away, coverage will likely improve over time)
About this issue
- Original URL
- State: open
- Created 3 years ago
- Reactions: 9
- Comments: 22 (20 by maintainers)
I believe there’s nothing stopping a server from offering non-standard methods, so this could cause false positives.
With custom transports that could also lead to false positives.
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
In case @rsc’s example is not obvious enough,
strings.TrimLeft(s, "http://")has a cutset with duplicate runes{"t", "/"}. So a duplicate rune detector would catch this. I find this example fairly persuasive.Would this qualify for the “frequency” factor of vet checks? How often do these issues occur?
It’s also worth noting that @dominikh’s staticcheck has had some of these for a while, like https://staticcheck.io/docs/checks#SA1000.
@nightlyone this bug strongly implies a misunderstanding of what
TrimLeft/TrimRightdo, so suggesting a “fix” where we remove the duplicate runes seems like a mistake.I’m not comfortable with this one. It’s a lucky accident that a duplicated rune catches a string where TrimPrefix was the right thing to call. I’d rather put this on hold until a more predictable method is discovered.
According to the vet README, this one fails the “precision” rule.
Based on the discussion, retitled to be only about strings.Trim/TrimLeft/TrimRight, detecting something like
strings.TrimLeft(s, "http://")that really wants strings.TrimPrefix.