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

x/tools/go/analysis/passes/nilness: nilness tracking across functions #68091

Open
RPGillespie6 opened this issue Jun 20, 2024 · 9 comments
Open
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@RPGillespie6
Copy link

RPGillespie6 commented Jun 20, 2024

Proposal Details

nilness checks currently only seem to apply to the scope of a single function. I.e. it doesn't detect and flag this obvious nil pointer dereference:

package main

import (
	"fmt"
)

type Foo struct {
	Bar string
}

func printBar(f *Foo) {
	fmt.Println(f.Bar)
}

func main() {
	printBar(nil)
}

It would be beneficial to track nilness issues across function calls in order to flag scenarios such as this.

Maybe nilness isn't the right analyzer for this; basically I want to achieve the same level of safety as .NET's nullable static analyzer, but in go

@gopherbot gopherbot added this to the Proposal milestone Jun 20, 2024
@gabyhelp
Copy link

Similar Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@ianlancetaylor
Copy link
Contributor

I don't think there is any API change here, so taking it out of the proposal process.

CC @golang/tools-team

@ianlancetaylor ianlancetaylor changed the title proposal: x/tools/go/analysis/passes/nilness: nilness tracking across functions x/tools/go/analysis/passes/nilness: nilness tracking across functions Jun 20, 2024
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Jun 20, 2024
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unreleased Jun 20, 2024
@adonovan
Copy link
Member

Nilness does only very simple intraprocedural analysis; it could certainly be extended to detect and record which parameters are "definitely dereferenced" on all control paths, such as f in your example, and then to treat calls to such functions as dereference operators. (Similarly it could detect and record which function results are always nil.)

Have you tried using Uber's nilaway? It is the closest existing thing to what you're asking for.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Jun 20, 2024
@RPGillespie6
Copy link
Author

I have not heard of nilaway, thanks for the link! Looks like nilaway could indeed be a good complement to or even replacement for go vet nilness! Running against the dummy program above, I get:

/home/user/sandbox/main.go:12:14: error: Potential nil panic detected. Observed nil flow from source to dereference point:
        - sandbox/main.go:16:11: literal `nil` passed as arg `f` to `printBar()`
        - sandbox/main.go:12:14: function parameter `f` accessed field `Bar`

I think that meets the spirit of this issue. Feel free to close this issue if you don't want or plan to increase the scope of nilness analyzer to this level.

@adonovan
Copy link
Member

Glad nilaway was useful, but I think increasing the scope of nilness analyzer as proposed would be a useful little project, so let's keep this open.

@timothy-king
Copy link
Contributor

"definitely dereferenced" is probably pretty reasonable. Doing this across packages seems doable and helpful too. Requires Facts that track enough info to create future reports in other packages.

I suspect we will want to keep multiple hops out of scope. Example:

func g(x *int) { print(*x) }
func f(x *int) { g(x) }
func h() { f(nil) }

Otherwise we get into issues with needing to represent "x's nilness" in the function summary of f.

(Similarly it could detect and record which function results are always nil.)

This is probably less useful. But seems doable. Cycles do need to be dealt with though.

@adonovan
Copy link
Member

Otherwise we get into issues with needing to represent "x's nilness" in the function summary of f.

Not sure I follow. The fact summarizing a function's "definitely dereferenced" disposition would hold a bit for each pointerlike word of each parameter. So for func f(struct{a, b, c *int}) there would be three bits, each tracked independently. Multiple hops doesn't seem particularly challenging; it's a classic interprocedural fixed-point problem.

@timothy-king
Copy link
Contributor

it's a classic interprocedural fixed-point problem.

I am hoping to avoid a major rewrite (which I am not sure we have the bandwidth for). Exactly one hop is not too different than what is there today. It just needs a single pass over package functions to report within the functions and summarize calls + a simple worklist to check intraprocedural calls.

@adonovan
Copy link
Member

I am hoping to avoid a major rewrite (which I am not sure we have the bandwidth for)

I didn't say it would be easy, but it would be fun. ;-)

@adonovan adonovan modified the milestones: Unreleased, Backlog Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants