go 🚀 - cmd/vet: warn about string(int) type conversions

@alandonovan @mvdan

agnivade on 8 Jun 2019

Pls also permit string(b) where b is a byte or uint8.

networkimprov on 8 Jun 2019

👍6 👎3

This seems fine to me. The warning should probably point the user in the right direction.

mvdan on 25 Jun 2019

Pls also permit string(b) where b is a byte or uint8.

I think this is essential if this proposal (and eventually #3939) is to be adopted.

The string(byte) conversion, when you're dealing with ASCII characters only, is probably used at least as often as string(rune)in my experience.

alanfo on 27 Jun 2019

👍1

@networkimprov @alanfo You will still be able to use string([]byte{b}).

dolmen on 2 Jul 2019

👎10

That would be a surprising requirement. And allocates a byte slice for a single statement?

networkimprov on 2 Jul 2019

👍4

@dolmen

You will still be able to use string([]byte{b}).

Well string(rune(b)) would be a better alternative because you wouldn't then have to allocate a byte slice but it would still be a nuisance to have to update all my code to use this workaround :(

alanfo on 3 Jul 2019

👍4

I will take a crack at adding an analysis pass and vet check for this.

smasher164 on 6 Nov 2019

string([]byte{b}) is not equivalent to string(b) for a byte value b: the former yields the string whose bytes are just b, the latter returns the string that UTF-8-encodes the single rune b. If b is greater than 127, then the former will be a single-byte string but the latter will be a two-byte encoding.

it would still be a nuisance to have to update all my code to use this workaround :(

Michael Matloob recently presented at a conference the "suggested fixes" feature of the analysis Diagnostics API. The diagnostic for the new analyzer could replace (or offer to replace) the deprecated expressions by the preferred version.

alandonovan on 6 Nov 2019

👍4

string([]byte{b}) ... yields the string whose bytes are just b

That could be invalid UTF-8. So string(buf[x:y]) may be a bug.

To extract strings from buffers, should we generally use string(bytes.ToValidUTF8(buf, nil))?

networkimprov on 6 Nov 2019

That could be invalid UTF-8. So string(buf[x:y]) may be a bug.

It could be, but it very much depends on the situation.

To extract strings from buffers, should we generally use string(bytes.ToValidUTF8(buf, nil))?

I had never even heard of that function until now, and I'm more deliberate than most about string encodings. Slicing a string or byte slice may be perfectly fine, again depending on the situation.

alandonovan on 6 Nov 2019

The diagnostic for the new analyzer could replace (or offer to replace) the deprecated expressions by the preferred version.

Be that as it may, it still seems anomalous to allow string(rune) but deprecate string(byte).

After all, string([]rune] and string([]byte) are both allowed but not conversions from slices of other integer types. It may therefore seem strange to people that both corresponding 'scalar' conversions are not allowed as well.

Indeed, @ianlancetaylor made a similar point in #3939 itself.

alanfo on 7 Nov 2019

👍3

The diagnostic for the new analyzer could replace (or offer to replace) the deprecated expressions by the preferred version.

What would be the suggested fix for an integer that overflows the size of the rune? Although this shouldn't occur in practice, simply changing it to rune(i) would break pre-existing code.

package main

import "fmt"

func main() {
    i := int64(1 << 33)
    fmt.Printf("%q %q\n", string(i), string(rune(i))) // prints "�" "\x00"
}

smasher164 on 30 Dec 2019

👍1

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

gopherbot on 31 Dec 2019

@smasher164 Do you have any examples of correct code that has this problem?

The correct fix is going to be to add a condition comparing the value to utf8.MaxRune, and when it is larger using utf8.RuneError.

ianlancetaylor on 31 Dec 2019

@ianlancetaylor Running the tool over my GOPATH revealed 16 cases where an int, uint, int64, or uint64 were converted to a string, and did not do bounds checking to ensure that the converted value doesn't overflow MaxRune. However, most of these cases depend on these functions' call sites, which could potentially be passing only valid input.

github.com/antlr/antlr4/runtime/Go/antlr/dfa_serializer.go:115:15: conversion from int to string
func (l *LexerDFASerializer) getEdgeLabel(i int) string {
    return "'" + string(i) + "'"
}

github.com/antlr/antlr4/runtime/Go/antlr/interval_set.go:235:34: conversion from int to string
github.com/antlr/antlr4/runtime/Go/antlr/interval_set.go:238:30: conversion from int to string
github.com/antlr/antlr4/runtime/Go/antlr/interval_set.go:238:53: conversion from int to string
type Interval struct {
    start int
    stop  int
}
type IntervalSet struct {
    intervals []*Interval
    readOnly  bool
}
for j := 0; j < len(i.intervals); j++ {
    v := i.intervals[j]
    if v.stop == v.start+1 {
        if v.start == TokenEOF {
            names = append(names, "<EOF>")
        } else {
            names = append(names, ("'" + string(v.start) + "'"))
        }
    } else {
        names = append(names, "'"+string(v.start)+"'..'"+string(v.stop-1)+"'")
    }
}



md5-90fde719051b5eedc9434d4c70b142e3



github.com/antlr/antlr4/runtime/Go/antlr/lexer_atn_simulator.go:633:15: conversion from int to string
func (l *LexerATNSimulator) GetTokenName(tt int) string {
    if tt == -1 {
        return "EOF"
    }

    return "'" + string(tt) + "'"
}



md5-90fde719051b5eedc9434d4c70b142e3



github.com/antlr/antlr4/runtime/Go/antlr/transition.go:239:15: conversion from int to string
github.com/antlr/antlr4/runtime/Go/antlr/transition.go:239:42: conversion from int to string
type RangeTransition struct {
    *BaseTransition

    start, stop int
}
func (t *RangeTransition) String() string {
    return "'" + string(t.start) + "'..'" + string(t.stop) + "'"
}



md5-90fde719051b5eedc9434d4c70b142e3



github.com/cznic/ql/etc.go:464:10: conversion from int to string
switch i := int(o); i {
case andand:
    return "&&"
case andnot:
    return "&^"
case lsh:
    return "<<"
case le:
    return "<="
case eq:
    return "=="
case ge:
    return ">="
case neq:
    return "!="
case oror:
    return "||"
case rsh:
    return ">>"
default:
    return string(i)
}



md5-90fde719051b5eedc9434d4c70b142e3



github.com/cznic/ql/etc.go:901:11: conversion from idealInt to string
github.com/cznic/ql/etc.go:905:11: conversion from idealUint to string
github.com/cznic/ql/etc.go:918:11: conversion from int64 to string
github.com/cznic/ql/etc.go:928:11: conversion from uint64 to string
case qString:
switch x := val.(type) {
//case nil:
//case idealComplex:
//case idealFloat:
case idealInt:
    return string(x), nil
case idealRune:
    return string(x), nil
case idealUint:
    return string(x), nil
//case bool:
//case complex64:
//case complex128:
//case float32:
//case float64:
case int8:
    return string(x), nil
case int16:
    return string(x), nil
case int32:
    return string(x), nil
case int64:
    return string(x), nil
case string:
    return x, nil
case uint8:
    return string(x), nil
case uint16:
    return string(x), nil
case uint32:
    return string(x), nil
case uint64:
    return string(x), nil
case []byte:
    return string(x), nil
case *big.Int:
    return x.String(), nil
case time.Time:
    return x.String(), nil
case time.Duration:
    return x.String(), nil
default:
    return invConv(val, typ)
}



md5-90fde719051b5eedc9434d4c70b142e3



github.com/cznic/ql/etc.go:1698:15: conversion from uint64 to string
rec[i] = string(y)



md5-90fde719051b5eedc9434d4c70b142e3



github.com/cznic/ql/ql.go:820:17: conversion from int to string
for _, f := range f {
    a = append(a, string(f.typ)+f.name)
}



md5-90fde719051b5eedc9434d4c70b142e3



github.com/go-delve/delve/pkg/proc/eval.go:535:9: conversion from int64 to string
b, _ := constant.Int64Val(argv.Value)
s := string(b)



md5-90fde719051b5eedc9434d4c70b142e3



github.com/unidoc/unidoc/pdf/internal/cmap/cmap.go:396:36: conversion from uint64 to string
target := hexToUint64(v)
i := uint64(0)
for sc := srcCodeFrom; sc <= srcCodeTo; sc++ {
    r := target + i
    cmap.codeMap[numBytes-1][sc] = string(r)
    i++
}

smasher164 on 1 Jan 2020

Thanks for the list. The antlr and ql cases all look like incorrect uses of string(int), where they really meant to use strconv.Itoa(int). If I'm right, the warning is serving its purpose for those cases. The unicode case probably really does mean to use string(r), so that code would have to change. I'm not sure about the go-delve case.

ianlancetaylor on 1 Jan 2020

I'm not sure about the go-delve case.

The go-delve case is implementing eval for type conversion, so I would ignore that case.

FWIW, if the goal of backwards compatibility in the suggested fixes is to ensure that the fix results in the same string as string(int), then comparing against utf8.MaxRune isn't the only option:

func conv(v uint64) uint64 {
    if v > utf8.MaxRune {
        return utf8.RuneError
    }
    return v
}

When a rune is converted to a string, if bit 22 is set, then the string will print "�". Using this fact, the conversion can be made branchless:

func conv(v uint64) uint64 {
    return v | (1-((v>>22|v>>21&1)-1)>>63)<<21
}

This checks if any of the top 43 bits are nonzero, and uses that truth to set bit 22.

smasher164 on 1 Jan 2020

The goal of the warning is to tell people when their code is not acting as they expect. As far as I can see the fix for cases like antlr is not to add a condition; it's to change the code to use strconv.Itoa instead of string(int), so that the right thing happens.

ianlancetaylor on 1 Jan 2020

That's fair. But we'd have to detect and distinguish those cases from cases where the conversion is correctly made and stays within the bounds of a rune. For instance:

/Users/akhil/work/src/golang.org/x/net/webdav/internal/xml/xml.go:1043:14: conversion from uint64 to string
if err == nil && n <= unicode.MaxRune {
    text = string(n)
    haveText = true
}

/Users/akhil/work/src/golang.org/x/net/webdav/internal/xml/xml.go:1066:15: conversion from int to string
var entity = map[string]int{
    "lt":   '<',
    "gt":   '>',
    "amp":  '&',
    "apos": '\'',
    "quot": '"',
}
if r, ok := entity[s]; ok {
    text = string(r)
    haveText = true
} else if d.Entity != nil {
    text, haveText = d.Entity[s]
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/golang.org/x/text/unicode/cldr/base.go:101:10: conversion from int64 to string
if s[1] == '#' {
    r, _ := strconv.ParseInt(s[3:len(s)-1], 16, 32)
    return string(r)
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/golang.org/x/text/message/fmt_test.go:1454:9: conversion from int to string
for i := 0; i < 128; i++ {
    if f.Flag(i) {
        s += string(i)
    }
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/golang.org/x/exp/errors/fmt/scan.go:607:50: conversion from int64 to string
r := int64(s.getRune())
n := uint(bitSize)
x := (r << (64 - n)) >> (64 - n)
if x != r {
    s.errorString("overflow on character value " + string(r))
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/github.com/docker/docker/vendor/github.com/golang/protobuf/proto/text_parser.go:315:10: conversion from uint64 to string
if i > utf8.MaxRune {
    return "", "", fmt.Errorf(`\%c%s is not a valid Unicode code point`, r, ss)
}
return string(i), s, nil



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/github.com/docker/docker/pkg/chrootarchive/archive_test.go:103:32: conversion from int to string
for i := 0; i < 65534; i++ {
    excludes[i] = strings.Repeat(string(i), 64)
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/github.com/rogpeppe/godef/vendor/9fans.net/go/plan9/dir.go:173:9: conversion from int to string
/Users/akhil/work/src/github.com/rogpeppe/godef/vendor/9fans.net/go/plan9/dir.go:177:10: conversion from int to string
var permChars = []permChar{
    permChar{DMDIR, 'd'},
    permChar{DMAPPEND, 'a'},
    permChar{DMAUTH, 'A'},
    permChar{DMDEVICE, 'D'},
    permChar{DMSOCKET, 'S'},
    permChar{DMNAMEDPIPE, 'P'},
    permChar{0, '-'},
    permChar{DMEXCL, 'l'},
    permChar{DMSYMLINK, 'L'},
    permChar{0, '-'},
    permChar{0400, 'r'},
    permChar{0, '-'},
    permChar{0200, 'w'},
    permChar{0, '-'},
    permChar{0100, 'x'},
    permChar{0, '-'},
    permChar{0040, 'r'},
    permChar{0, '-'},
    permChar{0020, 'w'},
    permChar{0, '-'},
    permChar{0010, 'x'},
    permChar{0, '-'},
    permChar{0004, 'r'},
    permChar{0, '-'},
    permChar{0002, 'w'},
    permChar{0, '-'},
    permChar{0001, 'x'},
    permChar{0, '-'},
}
for _, pc := range permChars {
    if p&pc.bit != 0 {
        did = true
        s += string(pc.c)
    }
    if pc.bit == 0 {
        if !did {
            s += string(pc.c)
        }
        did = false
    }
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/github.com/miekg/dns/msg_test.go:70:13: conversion from int64 to string
n, _ := strconv.ParseInt(escape[1:], 10, 8)
return string(n)



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/github.com/google/skylark/library.go:406:16: conversion from int to string
i, err := AsInt32(args[0])
if err != nil {
    return nil, fmt.Errorf("chr: got %s, want int", args[0].Type())
}
if i < 0 {
    return nil, fmt.Errorf("chr: Unicode code point %d out of range (<0)", i)
}
if i > unicode.MaxRune {
    return nil, fmt.Errorf("chr: Unicode code point U+%X out of range (>0x10FFFF)", i)
}
return String(string(i)), nil



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/github.com/cznic/ql/all_test.go:2949:9: conversion from int to string
for _, v := range rand.Perm(32) {
    s += string('0' + v)
}



md5-90fde719051b5eedc9434d4c70b142e3



/Users/akhil/work/src/upspin.io/client/clientutil/all_test.go:129:40: conversion from int to string
for i := 0; i < 10; i++ {
    loc := upspin.Location{
        Endpoint:  inProcess,
        Reference: upspin.Reference("ref" + string(i)),
    }
    tLocs = append(tLocs, loc)
}

smasher164 on 1 Jan 2020

I don't think I understand what you are suggesting. I don't understand why we, that is to say, cmd/vet, have to detect these cases at all. We are adding a warning in the hopes of making a language change. If there are too many warnings about correct code, we can't make the language change. If we make the language change, then we expect programs to rewrite the correct uses of string(int) to add an explicit conversion to rune; at that time the programmer can decide whether to add an explicit test for a value that does not fit in 32 bits.

ianlancetaylor on 1 Jan 2020

I guess what I meant was that the suggested fix would vary base on the case, i.e. would cmd/vet suggest a replacement of string(i) -> string(rune(i)) or string(i) -> strconv.Itoa(i)? But in practice, I suspect there is no way to knowing what is required, so the suggestion doesn't have to work in 100% of cases.

smasher164 on 1 Jan 2020

As the fix depends on the circumstances, I wonder whether the best solution here would be to wait until we (hopefully) get generics and then add a function to the strconv package which works for all integer types and does exactly what string(i) does today.

That way go vet could simply suggest a straight replacement and no one could complain that their code doesn't work the same as it did before.

Initially, the function could simply return string(i) until such time as this conversion was removed altogether after which the same underlying code could be used.

The documentation for the function could explain that it was being provided for compatibility with the earlier string(i) conversion and should be avoided otherwise. Hopefully, we'd be able to come up with a name which is suggestive of what the function actually does.

alanfo on 2 Jan 2020

@alanfo I'm not sure how generics would help, since the varying circumstances I'm referring to are when a user

Incorrectly assumes that string(i) is the string representation of an int, i.e. 3 -> "3".
Incorrectly assumes that i is within the range of a rune.
Ensures that i is within the range of a rune.

It isn't possible to detect all intended use cases and suggest a replacement, even with static analysis. But string(rune(i)) covers the majority of cases, and only leaves a few outliers, so my feeling is that this should be the suggested fix.

smasher164 on 2 Jan 2020

Are there other cases in which vet recommends a fix? Usually it just flags code for review.

josharian on 2 Jan 2020

👍1

@smasher164 I was looking at the matter from a different perspective i.e. assuming that the users know exactly what they're doing when using string(i) but (as we want to eventually get rid of it due to lack of clarity) suggest that they use instead a function which does the same thing but has a clearer name.

Of course, having alerted them to the problem, they might then review the code and decide it doesn't do what they expected anyway.

I agree that, if we need to suggest a fix using what we already have, then string(rune(i)) would be the one to go for but, if @Josharian is right and all we really need to do is flag it for review, then that would be better still.

alanfo on 2 Jan 2020

Are there other cases in which vet recommends a fix? Usually it just flags code for review.

@josharian vet doesn't recommend a fix, but this discussion was intend to follow up on @alandonovan's idea to use the _"'suggested fixes' feature of the analysis Diagnostics API"_ to offer a replacement. In terms of flagging for review, the CL is currently working as intended.

smasher164 on 2 Jan 2020

Before adding a cmd/vet check, I think it is best to fix the instances in the standard library that the analysis pass flags. After looking through them, they can all be fixed by replacing the conversion with string(rune(v)). Here they are at 1cfe8e91b6d742140153d943e1b09ad48c670b1f:

# bufio_test
bufio/bufio_test.go:150:10: conversion from int to string yields a string of one rune
# vendor/golang.org/x/net/dns/dnsmessage
vendor/golang.org/x/net/dns/dnsmessage/message.go:2162:59: conversion from Type (uint16) to string yields a string of one rune
# encoding/xml
encoding/xml/xml.go:1058:14: conversion from uint64 to string yields a string of one rune
encoding/xml/xml.go:1080:15: conversion from int to string yields a string of one rune
# fmt
fmt/scan.go:607:50: conversion from int64 to string yields a string of one rune
# fmt_test
fmt/fmt_test.go:239:9: conversion from untyped int to string yields a string of one rune
fmt/fmt_test.go:240:10: conversion from untyped int to string yields a string of one rune
fmt/fmt_test.go:241:10: conversion from untyped int to string yields a string of one rune
fmt/fmt_test.go:242:11: conversion from untyped int to string yields a string of one rune
fmt/fmt_test.go:1460:9: conversion from int to string yields a string of one rune
# go/types
go/types/conversions.go:31:32: conversion from int64 to string yields a string of one rune
# golang.org/x/net/dns/dnsmessage
vendor/golang.org/x/net/dns/dnsmessage/message.go:2162:59: conversion from Type (uint16) to string yields a string of one rune
# net
net/dnsclient_test.go:45:26: conversion from int to string yields a string of one rune
# net/rpc/jsonrpc
net/rpc/jsonrpc/all_test.go:130:26: conversion from int to string yields a string of one rune
net/rpc/jsonrpc/all_test.go:131:58: conversion from int to string yields a string of one rune
# reflect
reflect/value.go:2626:33: conversion from int64 to string yields a string of one rune
reflect/value.go:2631:33: conversion from uint64 to string yields a string of one rune
# runtime_test
runtime/string_test.go:286:8: conversion from int to string yields a string of one rune
runtime/string_test.go:286:24: conversion from int to string yields a string of one rune
runtime/string_test.go:295:10: conversion from int to string yields a string of one rune
# runtime/pprof/internal/profile
runtime/pprof/internal/profile/proto.go:235:45: conversion from int to string yields a string of one rune
# strconv_test
strconv/quote_test.go:183:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:184:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:185:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:186:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:187:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:188:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:189:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:190:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:191:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:192:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:193:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:194:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:195:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:196:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:197:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:198:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:199:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:200:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:201:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:202:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:203:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:204:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:205:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:206:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:207:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:208:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:209:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:210:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:211:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:212:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:213:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:214:3: conversion from untyped int to string yields a string of one rune
strconv/quote_test.go:215:3: conversion from untyped int to string yields a string of one rune
# strings_test
strings/strings_test.go:681:7: conversion from untyped int to string yields a string of one rune
strings/strings_test.go:682:30: conversion from untyped int to string yields a string of one rune

smasher164 on 25 Feb 2020

Change https://golang.org/cl/220844 mentions this issue: all: avoid string(i) where i has type int

gopherbot on 25 Feb 2020

Change https://golang.org/cl/220798 mentions this issue: dns/dnsmessage: avoid string(i) where i has type int

gopherbot on 25 Feb 2020

Thanks. I sent CLs to fix those cases.

ianlancetaylor on 25 Feb 2020

❤1

Change https://golang.org/cl/220977 mentions this issue: cmd/vet: include ifaceassert and stringintconv checks from upstream x/tools

gopherbot on 25 Feb 2020

Change https://golang.org/cl/221337 mentions this issue: std,cmd: update x/net and github.com/google/pprof

gopherbot on 27 Feb 2020

Change https://golang.org/cl/221339 mentions this issue: cmd/vet: add ifaceassert and stringintconv checks

gopherbot on 27 Feb 2020

Change https://golang.org/cl/221338 mentions this issue: cmd/compile: avoid string(int) conversion

gopherbot on 27 Feb 2020

There were an additional couple of cases in cmd/compile and google/pprof that the trybot caught, but weren't discovered with go test all. To address these cases, I broke up the commits into updating cmd/compile, re-vendoring dependencies, and adding the vet check.

smasher164 on 27 Feb 2020

Change https://golang.org/cl/221377 mentions this issue: net: report port number correctly in Plan 9 error

gopherbot on 27 Feb 2020

Change https://golang.org/cl/221437 mentions this issue: dns/dnsmessage: correct error message to be readable

gopherbot on 27 Feb 2020

Change https://golang.org/cl/221382 mentions this issue: cmd/compile: check rune type bounds as int32, not uint32

gopherbot on 27 Feb 2020

Change https://golang.org/cl/221384 mentions this issue: runtime/pprof/internal/profile: make error message readable

gopherbot on 27 Feb 2020

The vet check exists, but it is not enabled in go test. Reopening this issue to turn that on.

ianlancetaylor on 6 May 2020

As a reminder, we introduced a new process for these Go 2-related language changes in our blog post blog.golang.org/go2-here-we-come. The Go 1.15 development cycle is now over and it’s time for the final decision for the issues described at https://blog.golang.org/go1.15-proposals, including this one.

This has been implemented since March 9 in the development tree, but we haven't yet enabled it by default in the go command (in cmd/go/internal/test/test.go).

There has been no negative feedback otherwise, so accepting this proposal for Go 1.15.

— rsc for proposal review

rsc on 6 May 2020

Change https://golang.org/cl/232660 mentions this issue: cmd/go: enable stringintconv and ifaceassert vet checks by default

gopherbot on 7 May 2020

Change https://golang.org/cl/232679 mentions this issue: internal/lsp/cache: avoid string(int) conversion

gopherbot on 7 May 2020

The trybot flags three cases in the x/ repos with string(int) conversions, two of which are in internal/lsp/cache, for which I have sent CL 232679. The last case is in tools/go/ssa/interp:

https://github.com/golang/tools/blob/88e38c1d8d1c50e74524645ad116081930e6c12d/go/ssa/interp/ops.go#L1194-L1200

// integer -> string?
// TODO(adonovan): fix: test integer -> named alias of string.
if ut_src.Info()&types.IsInteger != 0 {
    if ut_dst, ok := ut_dst.(*types.Basic); ok && ut_dst.Kind() == types.String {
        return string(asInt(x))
    }
}

It seems that the conversion here is actually required, since it is interpreting the SSA that corresponds to this conversion. In that case, would it be better to disable the vet check for this package? /cc @alandonovan

smasher164 on 7 May 2020

Change https://golang.org/cl/232718 mentions this issue: go/ssa/interp: avoid vet warning about string(int) conversion

gopherbot on 7 May 2020

I commented on the CL: use Sprintf("%c", rune). Performance is not important here.

alandonovan on 7 May 2020

👍2

Change https://golang.org/cl/233899 mentions this issue: message, unicode/cldr: avoid string(int)

gopherbot on 13 May 2020

Change https://golang.org/cl/233900 mentions this issue: icmp, webdav/internal/xml: avoid string(int)

gopherbot on 13 May 2020

Change https://golang.org/cl/233902 mentions this issue: errors/fmt: avoid string(int)

gopherbot on 13 May 2020

@ianlancetaylor Do you know who should be assigned as the owner for this? It's labeled as a blocker for the Go 1.15 beta.

toothrot on 14 May 2020

@toothrot Since CL 232660 has been merged and all of the trybot issues are taken care of, this issue is resolved, and no longer a release blocker.

smasher164 on 14 May 2020

Can we improve the vet error message? I'm seeing this on tip:

x/x.go:9:14: conversion from int to string yields a string of one rune

This message states a fact without indicating a problem. If I weren't aware of this upcoming language change, I'd wonder why vet is spouting non sequiturs.

cespare on 14 May 2020

Would this be this better?

"conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprintf("%d", x)?)"

alandonovan on 14 May 2020

👍1

I think even better would be to add a link to a short wiki page where we explain about the language change, and provide two replacements, one like Alan suggested and one use fmt.Sprintf %c if they really did mean it.

josharian on 14 May 2020

Would this be this better?

But that doesn't cover the case when the user meant fmt.Sprintf("%c", x). I agree that the current error message isn't ideal, but the conversion is discouraged both because it doesn't mean a "string of digits" and because there is a more robust way of converting the codepoint to a string.

I think even better would be to add a link to a short wiki page

Indeed the package documentation for the vet check explains the alternatives in more detail. Perhaps this should be included in the wiki?

This checker flags conversions of the form string(x) where x is an integer
(but not byte or rune) type. Such conversions are discouraged because they
return the UTF-8 representation of the Unicode code point x, and not a decimal
string representation of x as one might expect. Furthermore, if x denotes an
invalid code point, the conversion cannot be statically rejected.

For conversions that intend on using the code point, consider replacing them
with string(rune(x)). Otherwise, strconv.Itoa and its equivalents return the
string representation of the value in the desired base.

smasher164 on 14 May 2020

Change https://golang.org/cl/234517 mentions this issue: doc/go1.15: mention vet warning for string(x)

gopherbot on 19 May 2020

I'm not sure that CL 234517 fixes this issue. It is good to have things spelled out in the release notes, but I think have more info, or a link to more info, in the vet error itself is what this issue is about.

josharian on 19 May 2020

This issue is about vet issuing a warning for string(int).

I'm fine with improving the vet warning, of course.

ianlancetaylor on 19 May 2020

@ianlancetaylor Ack. Filed #39151.

josharian on 20 May 2020

@josharian Thanks.

ianlancetaylor on 20 May 2020

Change https://golang.org/cl/264217 mentions this issue: [release-branch.go1.15-bundle] icmp, webdav/internal/xml: avoid string(int)

gopherbot on 21 Oct 2020

Go: cmd/vet: warn about string(int) type conversions

Most helpful comment

All 62 comments

Related issues