SA5000: doesn't detect assignment following the return of a nil map or named nil map

[elansys-kc]

staticcheck 2020.2.3 (v0.1.3)

Compiled with Go version: go1.15.8 Main module: honnef.co/go/[email protected] (sum: h1:qTakTkI6ni6LFD5sBwwsdSO+AQqbSIxOauHTTQKZ/7o=) Dependencies: github.com/BurntSushi/[email protected] (sum: h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=) golang.org/x/[email protected] (sum: h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=) golang.org/x/[email protected] (sum: h1:myAQVi0cGEoqQVR5POX+8RR2mrocKqNN1hmeMqhX27k=) golang.org/x/[email protected] (sum: h1:po9/4sTYwZU9lPhi1tOrb4hCv3qrhiQ77LZfGa2OjwY=) golang.org/x/[email protected] (sum: h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=)

func main() {
	newmap := testMap()
	if newmap == nil {
		fmt.Println("there is no spoon")
	}
	newmap["bob"] = true

	newmap2 := testNamedMap()
	if newmap2 == nil {
		fmt.Println("there is no spoon")
	}
	newmap2["bob"] = true
}

func testMap() map[string]bool {
var newmap map[string]bool
	return newmap
}

func testNamedMap() (newmap map[string]bool) {
	return newmap
}

I expect a linter improvement will not be that straight forward but it may be worth an open or closed issue in any case. Perhaps this would be more easily fixed with a language change or if go acquires null safety? Or perhaps it is easier to improve, than I realise?

"https://groups.google.com/g/golang-nuts/c/0QYIcxpLOCk"

[dominikh]

This might work as an extension of SA5011.

[Tiny-Box]

Hi, @dominikh. I noticed in the 2021.1.1 Release notes that SA5011 no longer infers possible nil pointer dereferences from comparisons done outside of control flow and no longer claims that indexing a nil slice will cause a nil pointer to dereference. This solution is not mentioned as a solution for the issue mentioned. Additionally, in the scenario where xxx := doSomething(), if doSomething could potentially return either nil or a map, it seems we don't have a good way to check for this. I wondered if SA5011 stopped doing this type of check because of a high number of false negatives being identified, as mentioned in the Release notes. Was this decision made after weighing the pros and cons of continuing this check?

[dominikh]

I wondered if SA5011 stopped doing this type of check because of a high number of false negatives [sic] being identified, as mentioned in the Release notes

If the release notes state the reason, then that's most likely why we did it.

Was this decision made after weighing the pros and cons of continuing this check?

Staticcheck has near zero tolerance for false positives. It's a tool for finding some issues, not for proving that your code is free of issues. As such, we usually do not keep (parts of) checks that can incorrectly match valid, normal code.

However, I don't see how that change to SA5011 relates to this issue. The pattern in this issue still involves comparisons as part of control flow.

[Tiny-Box]

It's a tool for finding some issues, not for proving that your code is free of issues.

I understand that Staticcheck may prioritize accuracy over comprehensiveness, especially if it cannot achieve both without producing false positives. Based on the issue you mentioned, the creator points out a runtime error when a map is initialized as nil and then assigned a value, which is covered to some extent by SA5000 and SA5011. However, Staticcheck does not currently report this type of error because it has not achieved a balance between comprehensiveness and minimizing false positives.

Is my understanding right?

[dominikh]

However, Staticcheck does not currently report this type of error because it has not achieved a balance between comprehensiveness and minimizing false positives.

Is my understanding right?

That seems like an oddly philosophical way of saying that we haven't had the time to work on this issue yet.