Go-tools: ST1017: possible false positive in non-if conditions

[dmitshur]

I have some code that looks like this:

// Render the tabnav.
err := htmlg.RenderComponents(w, tabnav{
    Tabs: []tab{
        {
            Content:  iconText{Icon: octicon.Person, Text: "Overview"},
            URL:      "/about",
            Selected: "/about" == req.URL.Path,
        },
        {
            Content:  iconText{Icon: octicon.DeviceDesktop, Text: "Setup"},
            URL:      "/about/setup",
            Selected: "/about/setup" == req.URL.Path,
        },
    },
})
if err != nil {
    return err
}

I get don't use Yoda conditions (ST1017) reports on "/about" == req.URL.Path and "/about/setup" == req.URL.Path lines.

I'm not sure if it's a true positive in this case. I can see how yoda conditions in if statements are not worth having, and thus worth reporting. But in this case, having the "/about" and "/about/setup" alignment improves readability IMO.

To me, it's not clear that swapping the comparison order is an improvement:

{
    Content:  iconText{Icon: octicon.Person, Text: "Overview"},
    URL:      "/about",
    Selected: req.URL.Path == "/about",
},
{
    Content:  iconText{Icon: octicon.DeviceDesktop, Text: "Setup"},
    URL:      "/about/setup",
    Selected: req.URL.Path == "/about/setup",
},

What do you think?

If we agree on this being a false positive, maybe a broad fix is to limit scope to if statements that cannot rely on indentation the way struct literals can, and a more narrow one is to ignore struct literals with aligned fields.

[ainar-g]

To be honest, both seem like a bad style to me, as both have magical strings. Better:

// Render the tabnav.
const (
    aboutPath      = "/about"
    aboutSetupPath = "/about/setup"
)
err := htmlg.RenderComponents(w, tabnav{
    Tabs: []tab{
        {
            Content:  iconText{Icon: octicon.Person, Text: "Overview"},
            URL:      aboutPath,
            Selected: aboutPath == req.URL.Path,
        },
        {
            Content:  iconText{Icon: octicon.DeviceDesktop, Text: "Setup"},
            URL:      aboutSetupPath,
            Selected: aboutSetupPath == req.URL.Path,
        },
    },
})
if err != nil {
    return err
}

[dmitshur]

I didn't expect it to have an effect on ST1017, but using such constants makes staticcheck no longer emit ST1017 on those lines.

I agree using consts for paths is better, and that was the long term plan. Using string literals was a part of the first prototype. Perhaps I'll expedite the process of starting to use consts.

[dominikh]

I didn't expect it to have an effect on ST1017

Well, the definition of a yoda condition is <literal> <binop> <non-literal>.

[dmitshur]

I see. I thought it would be <const> <binop> <non-const>. Why isn’t it?

[dominikh]

Why isn’t it?

Well, because that's not the pattern we're trying to flag :-) – lit op non-lit is common in languages where assignment is an expression and pretty much always an anti-pattern in Go (if we ignore your case). Most often you'll see it with very basic literals, such as 0 (if 0 == ret).

What side of an expression a const should be on seems a lot more opinionated, and will depend on things other than trying to avoid an accidental assignment.

[dominikh]

I'm going to close this for now. It's a rare and weird enough edge case that I am comfortable recommending a //lint:ignore, writing your code differently, or disabling the check altogether.

I will reconsider this if more people run into the same issue.