string-of-int missed a cast that `go vet` caught

[Groxx]

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!

1. Clone https://github.com/uber/cadence somewhere
2. git checkout beb64a7b
3. Make the changes in the diff output above ^, as this commit contains the fixed line
4. make lint | grep string-of-int should download and build everything necessary automatically. Everything should be isolated / not install a bunch of stuff globally on your machine.
5. 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 :)

[chavacava]

Hi @Groxx, thanks for filling the issue (and sorry for the late response)

Il will study the problem this weekend but at a first glance I suspect that the rule does not have enough type information to deduce that the function common.WorkflowIDToHistoryShard returns an int therefore the function isIntExpression of the rule

func (w *lintStringInt) isIntExpression(es []ast.Expr) bool {
...
	t := w.file.Pkg.TypeOf(es[0])
	if t == nil {
		return false
	}
...

returns false... A couple of println in the isIntExpression function will help :)

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.

Rules do not mutate the AST or the type information. Anyway to be sure you can activate only string-of-int when running revive

[chavacava]

Hi again @Groxx , I've reproduced the bug and after analysis, I confirm that the problem's root is the lack of type information about the variable shardId. As you can see in the following debug session, there is no type information attached to shardId therefore the function isIntExpression conservatively returns false.

To fix, I think, we will need to evolve the type-checking mechanism in order to use the latest building libraries of the language. Of course we welcome any help!

[Groxx]

It's still kinda strange that it can't figure that type out though, as it's a direct call to a func WorkflowIDToHistoryShard(workflowID string, numberOfShards int) int {...

Anyway. I'm not entirely sure what you mean by the "evolve the type-checking mechanism in order to use the latest building libraries of the language" - I don't suppose there's a github issue or something around it already? Is it referring to the golang.org/x/tools/go/gcexportdata use? I'm not familiar with that, unfortunately. I've used golang.org/x/tools/go/packages and had very good results, but I don't know how different they are tbh.

[Groxx]

Ah hah. I can make a minimal repro with two packages, like below. Seems like type information isn't propagated?

// in a file:
import (
    "fmt"
    "./other"
)
func BadCast() {
    i := other.ThingReturningInt()
    s := string(i)
    fmt.Println(s)
}
// in an "other" subfolder
package other
func ThingReturningInt() int {
    return 5
}

If that's known: https://pkg.go.dev/golang.org/x/tools/go/analysis can at least make that kind of lookup work, I've built tools with it in the past. It's not really suitable out-of-the-box except for "toys" due to how its runner calls os.Exit, but the approach is sound.

[chavacava]

revive collects type information by using the Config.Check function from the go/types library . This function, with the advent of modules, was somewhat deprecated in favor of go/packages in the context of building analysis tools. We need to replace the use of Config.Check with functions from the go/packages (e.g. Load) that will provide inter-package type-checking. This replacement is not hard (I've already did it in gusano, a tool that reuses a huge part of revive code) but we need to take care to constraint the performance overhead of inter-package type checking to the cases that are strictly necessary (ie when applying rules that require type checking)