string-of-int missed a cast that `go vet` caught
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 5.6k
- Forks
- 330
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 9
Description
The bug
I just started hooking up revive, and results are very good so far! Many thanks for this project :)
When adding the string-of-int check, I toyed around with the equivalent go vet -stringintconv check, and found a discrepancy that seems.... odd.
Undoing a fix in this commit reproduces it, the shardIDstr := string(shardID) line below is not found:
~/gocode/src/github.com/uber/cadence @beb64a7b !1
❯ git diff
diff --git service/frontend/adminHandler.go service/frontend/adminHandler.go
index f4168390..179fb4a9 100644
--- service/frontend/adminHandler.go
+++ service/frontend/adminHandler.go
@@ -248,7 +248,7 @@ func (adh *adminHandlerImpl) DescribeWorkflowExecution(
}
shardID := common.WorkflowIDToHistoryShard(request.Execution.WorkflowID, adh.numberOfHistoryShards)
- shardIDstr := string(rune(shardID)) // originally `string(int_shard_id)`, but changing it will change the ring hashing
+ shardIDstr := string(shardID) // originally `string(int_shard_id)`, but changing it will change the ring hashing
shardIDForOutput := strconv.Itoa(shardID)
historyHost, err := adh.GetMembershipMonitor().Lookup(common.HistoryServiceName, shardIDstr)
~/gocode/src/github.com/uber/cadence @beb64a7b !1
❯ make lint | grep string-of-int
<nothing>
Since the same variable is used in strconv.Itoa(shardID), it's pretty clear that shardID is an int, and this is not a line that should have succeeded. go vet -stringintconv ./... with go 1.15.7 finds it, as further evidence:
❯ go vet -stringintconv ./...
# github.com/uber/cadence/service/frontend
service/frontend/adminHandler.go:251:16: conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)
I'm really not sure where this behavior could be coming from, as the string-of-int code is pretty simple at a glance, and it found all the other instances.
My suspicion is that there could be some mutating of the AST / type data by other rules, but I don't really know where to start looking for that.
To Reproduce
I've tried making a small repro for this, but the same kinds of lines of code + the same config file don't reproduce it... so unfortunately I can only point you to the repo where I encountered this. Thankfully it's open source!
- Clone https://github.com/uber/cadence somewhere
git checkout beb64a7b- Make the changes in the diff output above ^, as this commit contains the fixed line
make lint | grep string-of-intshould download and build everything necessary automatically. Everything should be isolated / not install a bunch of stuff globally on your machine.- Note that line 251 does not result in a warning.
Revive's version and related libraries are all pinned by the go.mod, and the config toml and --exclude flags are part of the make lint target (and they will be printed), so this should be easily reproducible.
Let me know if you have any trouble running those steps, I'd be happy to try to fix it :)
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Reproduce the discrepancy with service/frontend/adminHandler.go at commit beb64a7b, the shown cast change, and make lint; compare that result with go vet -stringintconv ./.... Then inspect revive's string-of-int check and its interaction with AST or type data. Done means the check reports the missed conversion without regressing the other instances.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100