Another false positive

[AlekSi]

err := c.s.Send(msg)
c.sendM.Unlock()
if err != nil {
	c.close(errors.Wrap(err, "failed to send message"))
	return
}

In general, I think ifshort should skip cases when there is some code between assignment and if

[esimonov]

Thanks for reporting, that's a really interesting one.

I'm not sure about your proposition however. Most of the times, having some code between the two statements does not lead to consequences that your snippet aims to show. On the other hand, building a reliable detector for cases like yours sounds too ambitious for a small pet project like ifshort. At least, no simple heuristics popped into my head at first glance; I'd be grateful for any ideas.

If I don't come up with some feasible way of detecting cases where the in-between code makes using short syntax impossible, I'm afraid that I can't offer you anything but an optional setting in a future minor release, which would enable the behaviour you propose.

[AlekSi]

Do you want this linter to prefer false positives or false negatives? Personally, I think false negatives are better for stylistic linters like this one – it is better to keep code slightly inconsistent than wrong or with a lot of //nolint directives.

Also, not sure I agree with "most of the times" – I would say if there is some code between assignment and if, in practice, it is there for side-effects.

[dackroyd]

Do you have a different example that would reflect this clearly? I do believe they exist, but I'm suspect the example provided would be clearer following the ifshort rule.

When we're dealing with Unlock, it would be more common to be dealing with that using defer (especially since Go 1.14 and defer having almost no overhead). Here I'm assuming a Lock call, which would then be followed by deferring the Unlock immediately:

c.sendM.Lock()
defer c.sendM.Unlock()

if err := c.s.Send(msg); err != nil {
	c.close(errors.Wrap(err, "failed to send message"))
	return
}

It's also typical that err is handled immediately after the call that assigns it, since the error needs to be checked before attempting to use any of the other returned values. An example of an exception here perhaps could be cancelling a context timeout?

ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second)
err := c.s.Send(ctx, msg)
cancel()

if err != nil {
	... handle ...
}

I suspect in a lot of cases, that could be defer cancel() instead though, resolving the issue:

ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second)
defer cancel()

if err := c.s.Send(ctx, msg); err != nil {
	... handle ...
}

[sashayakovtseva]

Also having false positive when using a variable inside the embedded struct. Here's an example:

package test

import (
	"net/http"
)

type click struct {
	clickMeta
}

type clickMeta struct {
	referrer string
}

func handle(w http.ResponseWriter, req *http.Request) {
	q := req.URL.Query()

	referrer := q.Get("referrer")
	if referrer == "" {
		referrer = req.Referer()
	}

	clk := click{
		clickMeta: clickMeta{
			referrer: referrer,
		},
	}

	// some logic here
	_ = clk

	w.WriteHeader(http.StatusOK)
}

This yields

test.go:18:2: variable 'referrer' is only used in the if-statement (test.go:19:2); consider using short syntax (ifshort)
        referrer := q.Get("referrer")
        ^

However it's clear that I set referrer in the embedded struct. Without embedding there is no false positive though.

[AlekSi]

Do you have a different example that would reflect this clearly? I do believe they exist, but I'm suspect the example provided would be clearer following the ifshort rule. When we're dealing with Unlock, it would be more common to be dealing with that using defer (especially since Go 1.14 and defer having almost no overhead).

That will hold c.sendM for the duration of c.close that can be slow.

[alingse]

same with @sashayakovtseva code