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.

Sign up for GitHub

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

Jump to bottom

proposal: cmd/vet: report len usage with strings #50166

Closed
epels opened this issue Dec 14, 2021 · 7 comments
Closed

proposal: cmd/vet: report len usage with strings #50166

epels opened this issue Dec 14, 2021 · 7 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@epels
Copy link

epels commented Dec 14, 2021

I have more than once seen unexpected behaviour, mostly related to string validation, caused by the len built-in function returning the count of bytes in the string, rather than the number of runes. For example:

func validate(name string) error {
	if len(name) > 64 {
		return errors.New("name cannot be longer than 64 characters")
	}
	return nil
}

In my experience, the author of code similar to the above is nearly always looking to get the number of runes in the string - not the bytes needed to represent when encoded as UTF-8. Of course, the code above is fully legal and therefore this often unexpected behavior does not surface until runtime.

I believe a go vet check that warns about using len in combination with a string argument would be valuable - especially to programmers that are learning Go.
@epels epels added the Proposal label Dec 14, 2021
@seankhliao seankhliao changed the title cmd/vet: report len usage with strings proposal: cmd/vet: report len usage with strings Dec 14, 2021
@seankhliao
Copy link
Member

How would you measure the size of a string if every use of len with string is flagged?
I think there's no way this will reach the accuracy bar needed for a cmd/vet check.

@epels
Copy link
Author

epels commented Dec 14, 2021

@seankhliao Thanks for correcting the title of the issue.

To answer your question regarding measuring the size of a string s: len([]byte(s)) would work.

@seebs
Copy link
Contributor

seebs commented Dec 14, 2021

vet checks are for things which are probably wrong, at the very least, and which wouldn't be the correct and idiomatic way to do a thing. I think that the behavior of len(s) is reasonably clear and usually correct.

I've only been using go for a few years but I don't think I've ever yet actually wanted to measure a string's length in runes.

@seankhliao
Copy link
Member

Have you looked at how much code this would mark as invalid for a very marginal benefit?

@gopherbot gopherbot added this to the Proposal milestone Dec 14, 2021
@ALTree
Copy link
Member

ALTree commented Dec 14, 2021

I think this fails the 3rd criterion for new vet checks:

Precision:

Most of vet's checks are heuristic and can generate both false positives (flagging
correct programs) and false negatives (not flagging incorrect ones). The rate of
both these failures must be very small.

Just skimming over e.g. the strings package in the standard library I saw dozens of completely correct len(string) calls that would be flagged by this vet check.

@timothy-king
Copy link
Contributor

Consider:

  • comparisons against 0: len(s) > 0
  • comparisons against other lengths: len(s) < len(t)
  • comparisons against a variable:len(s) != n

All of these are widely used and correct usages of len on string. Take a look at the correctness criteria for "cmd/vet" https://github.com/golang/go/blob/master/src/cmd/vet/README . I do not see a path forward without making the proposed check narrower than len(s) where s is an expression of underlying type "string".

@epels epels closed this as completed Dec 14, 2021
@henvic
Copy link
Contributor

henvic commented Dec 14, 2021

This is impractical because getting the length of bytes is the most common use case and such change would break backward compatibility with all existing Go code to date.

If you believe something should be done about this, my suggestion to you is to open a proposal for adding to the len() documentation a note explaining that utf8.RuneCountInString can be used if you want to count runes in a string.

@golang golang locked and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
7 participants
@seebs @timothy-king @henvic @ALTree @gopherbot @epels @seankhliao