dominikh / go-tools

Staticcheck - The advanced Go linter
https://staticcheck.dev
MIT License
6.09k stars 368 forks source link

stylecheck: decide fate of CheckUnexportedReturn #482

Open dominikh opened 5 years ago

dominikh commented 5 years ago

CheckUnexportedReturn flags exported functions that return unexported types. We wrote the check, but never made it available, not even as an optional check. We should either make it available, or remove it.

cespare commented 5 years ago

On #542 you wrote

my current opinion is that it's too noisy

Can you give some examples where this seems stylistically fine to you?

My opinion is that this is a useful check, since it catches real API issues I see in practice. But I don't think it's super important.

bcmills commented 4 years ago

If an exported function returns an unexported type, then documentation tools that display only the exported API (such as https://pkg.go.dev at present) will display the exported function with a dangling reference to a type for which no documentation is shown, and for which the reader cannot easily find a list of exported methods. (I, too, have found that to be a significant usability problem in practice.)

dominikh commented 4 years ago

@bcmills arguably this is something that godoc should improve

mwat56 commented 4 years ago

CheckUnexportedReturn flags exported functions that return unexported types.

I'd say: make it available. Seems to be very useful to me.

bcmills commented 4 years ago

Another problem with unexported return-types is that they impede refactoring.

In general, one can refactor code like:

    x := somepkg.Something()
    use(x)

to

    var x somepkg.SomeType
    if y {
        x = somepkg.Something()
    }
    …
    if y {
        use(x)
    }

If the caller cannot name the type of x (because that type is unexported), then that refactoring is impossible.

jsoref commented 3 years ago

What's the argument against making this check available?

The pattern (returning an unexported return type) seems harmful to everyone involved.