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

Doesn't work with multiple packages #31

Closed
vikstrous2 opened this issue Apr 25, 2024 · 6 comments · Fixed by #33
Closed

Doesn't work with multiple packages #31

vikstrous2 opened this issue Apr 25, 2024 · 6 comments · Fixed by #33

Comments

@vikstrous2
Copy link

vikstrous2 commented Apr 25, 2024

To reproduce, add this to the examples:

import "examples/other"

func iface2() (other.Iface, error) {
	return nil, nil // want "return both the `nil` error and invalid value: use a sentinel error instead"
}

And add this to a new package under "examples":

package other

type Iface interface {
	Do()
}

Reading the code of the linter, the package of the type is not considered in the analysis and *ast.SelectorExpr is not traversed in isDangerNilType, so only types form the same package are analyzed.

One way to "fix" this might be to create a config that bypasses this type based analysis entirely by saying "all types should be checked". That still wouldn't work if a custom config is given and typed based analysis is still desired, but I think it would work for most users.

@vikstrous2
Copy link
Author

Actually, there's a test that expects this wrong behaviour right now:

func ifaceExtPkg() (io.Closer, error)            { return nil, nil }

Also, my proposed solution would break these test cases:

func unsafePtr() (unsafe.Pointer, error)         { return nil, nil }
func slice() ([]int, error)                      { return nil, nil }

So I think a different solution is needed... the type analysis can't be skipped entirely :(

@vikstrous2
Copy link
Author

vikstrous2 commented Apr 25, 2024

Adding a check specifically for array type to make sure nil slices are always allowed is one solution to the slice issue:

	if n.checkedTypes.Contains(anyType) {
		if _, ok := t.(*ast.ArrayType); ok {
			return false
		}
		return true
	}

I don't know if I agree with the other test cases though. Why shouldn't nil io.Closer and unsafe.Pointer be reported as errors?

@Antonboom
Copy link
Owner

@vikstrous2

Hi! Thank you for issue.

Quote from README:

Assumptions

  • Types from external packages are not supported.

Are you interested in this feature?

@vikstrous2
Copy link
Author

Yeah, I like the pattern of using sentinel errors and I want to make sure we can detect the use of nil pointers as "not found" errors more reliably everywhere.

@Antonboom
Copy link
Owner

@vikstrous2 new release has been published, thanks!

Wait for it in the nearest release of golangci-lint 👌

@vikstrous2
Copy link
Author

This is amazing! I can't wait to try it out in golangci-lint! It seems to work so far

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants