Skip to content

unexported-return: do not allow unexported interfaces #1458

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.

Sign up for GitHub

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

Open
tie wants to merge 1 commit into
base: master
Choose a base branch
from
Open

unexported-return: do not allow unexported interfaces #1458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 8 additions & 21 deletions rule/unexported_return.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,34 +80,21 @@ func (*UnexportedReturnRule) Name() string {
func exportedType(typ types.Type) bool {
switch t := typ.(type) {
case *types.Alias:
obj := t.Obj()
switch {
// Builtin types have no package.
case obj.Pkg() == nil:
case obj.Exported():
default:
_, ok := t.Underlying().(*types.Interface)
return ok
}
return true
return exportedTypeName(t.Obj())
case *types.Named:
obj := t.Obj()
switch {
// Builtin types have no package.
case obj.Pkg() == nil:
case obj.Exported():
default:
_, ok := t.Underlying().(*types.Interface)
return ok
}
return true
return exportedTypeName(t.Obj())
case *types.Map:
return exportedType(t.Key()) && exportedType(t.Elem())
case interface {
Elem() types.Type
}: // array, slice, pointer, chan
return exportedType(t.Elem())
}
// Be conservative about other types, such as struct, interface, etc.
// Be conservative about other unnamed types, such as struct, interface, etc.
return true
}

func exportedTypeName(obj *types.TypeName) bool {
// Builtin types have no package.
return obj.Pkg() == nil || obj.Exported()
}
9 changes: 9 additions & 0 deletions testdata/golint/unexported_return.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ func ExportedIntReturner() int { // MATCH /exported func ExportedIntReturner ret
return int{}
}

type unexportedInterface interface {
UnexportedInterface()
}

// UnexportedInterfacePointer returns an unexported interface pointer type from this package.
func UnexportedInterfacePointer() *unexportedInterface { // MATCH /exported func UnexportedInterfacePointer returns unexported type *foo.unexportedInterface, which can be annoying to use/
Copy link

@veeam-denis veeam-denis Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, having pointer for an interface type as a result in 99.99% of cases is a mistake. Although, we can check it, I'd focus on a non-pointer result.

Copy link
Contributor

@ccoVeille ccoVeille Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a rule for detecting this? Or is it something already detected?

I have no computer with me to check unfortunately

Copy link
Collaborator

@chavacava chavacava Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an interface (vs concrete type) is usually not the best option (unless the func is a struct factory or something like that)

Copy link

@veeam-denis veeam-denis Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chavacava I agree, but here we have a pointer to an interface, which is even worse :)

I can imagine returning a settable value of an interface type and for that we return a pointer to it, but that's something I personally never saw in real life (neither I had a reason to do that in my own code). I saw this for input arguments that were meant to be the output ones as well, but not for the returned value.

return nil
}

type config struct {
N int
}
Expand Down
Loading