nilaway icon indicating copy to clipboard operation
nilaway copied to clipboard

Nil Detection when inject some dependency to other package

Open muhadif opened this issue 1 year ago • 1 comments

Hi, nilaway was awesome tool for me, but is it possible to checking nil when we access to injected dependency which have potential nil like my code below?

func main() {
	exampleRepo := NewExampleRepo()
	exampleModule := NewExampleModule(exampleRepo)
	exampleModule.GetFoo()
}
package main

type ExampleModule interface {
	GetFoo()
}

type exampleModule struct {
	repo ExampleRepo
}

// NewExampleModule forgot to add example Repo to struct
func NewExampleModule(repo ExampleRepo) ExampleModule {
	return &exampleModule{}
}

func (e exampleModule) GetFoo() {
	e.repo.GetFoo()
}
package main

type ExampleRepo interface {
	GetFoo()
}

type exampleRepo struct {
}

func NewExampleRepo() ExampleRepo {
	return &exampleRepo{}
}

func (e exampleRepo) GetFoo() {
}

I already run it with nilaway but still no error shown. But when I run the code directly the code will give nil panic.

muhadif avatar Nov 17 '23 00:11 muhadif

I believe this falls within struct initialization, which is a feature we are working to enable more broadly 🙂

Basically, right now, NilAway will complain if there is an explicit assignment of nil to e.repo and then a dereference without checking, but it won't track that the default value the field is initialized to can be nil. We do have support for checking this, but it's currently not enabled by default or exposed easily through global configuration.

You can test this mode by adding the following "pragma" comment above your package declaration for the two files above:

// <nilaway struct enable>
package main

which should enable struct initialization checking for that package. I'd be interested in knowing if that does what you want.

Note that there are two reasons this feature is not enabled by default: a) performance (mostly on very large codebases), b) it can add a non-insignificant number of false positives. That said, if this does what you want, we can consider exposing it as a global configuration option sooner, and we are definitely working on improving it and enabling it by default in the medium term.

lazaroclapp avatar Nov 22 '23 18:11 lazaroclapp