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: spec: remove string(int) #3939

Open
robpike opened this issue Aug 10, 2012 · 32 comments
Open

proposal: spec: remove string(int) #3939

robpike opened this issue Aug 10, 2012 · 32 comments

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Aug 10, 2012

The conversion of an integer to a string was put in very early to bootstrap formatted
printing, if I remember correctly. It's odd, though, and no longer necessary, if it ever
was. It also causes inconsistencies, since string(0xD800) cannot produce the UTF-8
encoding for that code point (by definition, surrogates are not legal in UTF-8) so must
produce something else. We chose the "\uFFFD" since that's the only reasonable
option, but that means:

1) the result isn't obvious
2) string(0xD800) and "\uD800" do different things: the former produces the
UTF-8 for U+FFFD while the latter is statically rejected.

I propose that, in some remote future, we eliminate this conversion from the language.
@adg
Copy link
Contributor

@adg adg commented Jan 21, 2013

Comment 1:

I guess we're bound by the Go 1 compatibility promise to keep this until Go 2.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 2:

Labels changed: added go2.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-main.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 3, 2014

Comment 4:

Adding Release=None to all Priority=Someday bugs.

Labels changed: added release-none.

@robpike robpike self-assigned this Mar 3, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@robpike robpike removed their assignment Sep 21, 2015
@dolmen
Copy link

@dolmen dolmen commented Dec 14, 2016

I've been beaten by this "feature" in production code today.
My code was equivalent to this (but of course not so obvious: type, const and cast were in 3 different packages, and the cast result was an argument to a function, so that made the bad use of the cast quite hidden):

   type XType int16
   const x XType = 0x0001
   var y = string(x)

I think that go vet should raise an alert at least for my case (conversion to string of a value of a named integer type).

@robpike
Copy link
Contributor Author

@robpike robpike commented Dec 15, 2016

It is valid code, so vet will not complain about it. Perhaps golint should, but not vet.

@rsc rsc changed the title spec: disallow string(int/rune) spec: remove string(int/rune) Jun 17, 2017
@rsc rsc changed the title spec: remove string(int/rune) proposal: spec: remove string(int/rune) Jun 17, 2017
@urandom
Copy link

@urandom urandom commented Oct 25, 2017

Would it be possible to make string(int), return the string representation of the int. More users will probably expect this behaviour:

a := 42
fmt.Print(string(a)) // "42"

EDIT: got it, bad idea

@dominikh
Copy link
Member

@dominikh dominikh commented Oct 25, 2017

@urandom I would not expect an innocuously looking conversion to invoke an expensive number formatting routine. The solution to fixing a (supposedly) confusing construct isn't to replace it with another, equally confusing one.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 25, 2017

No, that is not possible. It will silently break existing programs in a subtle way, for no real reason. If you want to convert an int (or a float or a bool) to a string, use package strconv. In particular, using strconv allows you more control over the exact formatting.

@qrpnxz
Copy link

@qrpnxz qrpnxz commented Jul 4, 2019

It's not totally clear to me if this proposal also removes string(rune). I hope not since it's very useful for debugging and constants as griesemer noted.

I don't think removing it totally is the solution. If string(rune) using utf8.RuneError replacement isn't obvious to someone, it makes more sense to me that they accept that is the case rather than remove it. It is a stone written fact that strings are valid utf8. It behaves just like string([]rune) and string([]byte), which behave just like utf8.EncodeRune, except that for the latter you have to add an import and write way more code.

It's totally fair to make string(int) have to be string(rune) however.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 4, 2019

I think it's clear that we have to continue to support string(r) where r has type rune. And it's likely that we have to continue to support string(b) where b has type byte or, equivalently, uint8. So what we are talking about removing is the conversions of other integer types to string.

Treating rune and byte/uint8 as a special case is not as bad as it may sound; we already do exactly that for converting values of type []rune and []byte (but not slices of other integer types) to string.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 31, 2019

Change https://golang.org/cl/212919 mentions this issue: go/analysis: add pass to check string(int) conversions

@alanfo
Copy link

@alanfo alanfo commented Dec 31, 2019

I am beginning to wonder whether removing the string(int) conversion is going to be more trouble than it's worth, even for integer types other than rune and byte.

I see from #32479 that the intermediate step of adding a check for this to go vet or other code analyzers presents difficulties if the suggested fix breaks existing code when the integer value being converted overflows rune.

Although one wouldn't expect this to occur in practice, people will no doubt have used these conversions in weird, unexpected but (to them) reasonable ways knowing precisely what the results would be.

Incidentally, just to reinforce the point that the string(byte) conversion should continue to be allowed, my attention was recently drawn (by a link posted on reddit) to this book about writing an interpreter in Go. In the free sample chapter, on page 7, I noticed this function:

func newToken(tokenType token.TokenType, ch byte) token.Token {
    return token.Token{Type: tokenType, Literal: string(ch)}
}

The intended audience for this book is people who know enough Go to be able to make sense of the code and I don't think those people are going to be impressed if the code in the book fails to compile (or vet).

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 31, 2019

It's clearly essential to find how much working code that is correctly using the conversion will break. We don't know that yet.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 19, 2020
Issue golang/go#3939 proposes to remove string(int) from the language on the
grounds that it produces non-obvious results that can't be statically checked.
An intermediate step is to have go vet check for these sorts of conversions.
This change adds an analysis pass to check for string(int) conversions. It
suggests a fix to replace string(int) with string(rune(int)).

Updates golang/go#32479.

Change-Id: Ifafd6d74f9bd4a903ce6b29ac3a3c7a15f8a1ad9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212919
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
andrewslotin added a commit to instana/go-sensor that referenced this issue May 19, 2020
The upcoming change in Go 1.15 `go vet` command disallows type casts
from `int` to `string`. This PR replaces `string(int)` conversions
with explicit formatting.

golang/go#3939
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 2, 2020

In 1.15 we have a vet warning for this case (#32479). Putting this issue on hold until we have more experience with that. Also waiting until we can assume that everyone is using Go modules with a language version.

LENSHOOD added a commit to LENSHOOD/parser that referenced this issue Sep 1, 2020
Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939
kennytm pushed a commit to pingcap/parser that referenced this issue Sep 5, 2020
Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this issue Apr 27, 2021
Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
xhebox pushed a commit to xhebox/parser that referenced this issue Sep 28, 2021
)

Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
xhebox added a commit to xhebox/tidb that referenced this issue Sep 28, 2021
)

Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
xhebox added a commit to xhebox/tidb that referenced this issue Oct 8, 2021
)

Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
ti-chi-bot added a commit to pingcap/tidb that referenced this issue Oct 9, 2021
Make test will fail due to Go1.15 starts to not allow "string(int)" expression.
Fix affected sources by replace such expression to legal expressions.

More info about ban string(int), see: golang/go#3939

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet