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

ST1017: possible false positive in non-if conditions #410

Closed
dmitshur opened this Issue Feb 4, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@dmitshur
Copy link
Contributor

dmitshur commented Feb 4, 2019

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

This comment has been minimized.

Copy link

ainar-g commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

dmitshur commented Feb 5, 2019

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

This comment has been minimized.

Copy link
Owner

dominikh commented Feb 7, 2019

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

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

@dmitshur

This comment has been minimized.

Copy link
Contributor Author

dmitshur commented Feb 7, 2019

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

@dominikh

This comment has been minimized.

Copy link
Owner

dominikh commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Owner

dominikh commented Feb 15, 2019

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.

@dominikh dominikh closed this Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment