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

a new checker for go-mode: go vet shadow #765

Closed
kostya-sh opened this Issue Oct 20, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@kostya-sh
Contributor

kostya-sh commented Oct 20, 2015

A shadow check is an experimental feature of go vet tool and currently disabled by default.

The following command can be used to run just a shadow check:

go tool vet -shadow=true

It would be good to include this check as an additional checker for go-mode.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 20, 2015

Thank you for this suggestion. That would be an option to the existing go vet syntax checker, but I'll not add that myself. Please understand that as I do not use Go extensions for Go syntax checking are not a high priority for me personally.

I'm closing this as wontfix, which just means that I for my part will not work on this. But I'll merge a pull request or patch, these are always very welcome. Please do not hesitate to try, and ask if you have any question about contributing to Flycheck.

@kostya-sh

This comment has been minimized.

Contributor

kostya-sh commented Oct 20, 2015

Fair enough. I will try to implement this myself and submit a PR then.

I am thinking about making flags that are passed to go vet on command line customizable via flycheck-def-option-var flycheck-go-vet-args. Please let me know if this is a valid approach.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 20, 2015

I think I'd prefer a specific option for shadow, I.e. flycheck-go-vet-enable-shadow, used with the option-flag.

@kostya-sh

This comment has been minimized.

Contributor

kostya-sh commented Oct 21, 2015

I thought about this as well. Unfortunately there is no clean and simple way to run vet with shadow check enabled.

Basically there are two options:

  • Use -test flag that enables all checks including shadow but this flag very likely will go away in the future versions of vet tool
  • Enable all 20 or so checks explicitly on command line. This will require changing flycheck every time a new check is added to vet tool

Both of these options will not work very well with different versions of vet that potentially might support different set of flags.

In addition there is -strictshadow flag that some users might want to enable as well.

It seems to me that letting users decide what flags to pass to go vet depending on the installed version and their preferences is the best approach in this case.

@kostya-sh

This comment has been minimized.

Contributor

kostya-sh commented Oct 21, 2015

An alternative could be a separate shadow checker and flycheck-go-vet-shadow-strict option.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 21, 2015

@kostya-sh I'm sorry, but you've lost me. I'm really confused now. Apparently -shadow=true isn't enough, is it? Could you please show me a complete go vet invocation with -shadow=true?

A second checker is not a reasonable option, and I'm not so much in favour of passing arbitrary arguments to checkers. I'd really like to understand the problem completely first.

@kostya-sh

This comment has been minimized.

Contributor

kostya-sh commented Oct 21, 2015

@lunaryorn , sorry it is confusing indeed. Hopefully the following examples will help:

Run all checks except shadow:

go tool vet -all *.go

Run only shadow check:

go tool vet -shadow *.go

Run all checks including shadow (help text for -test flag: for testing only: sets -all and -shadow)

go tool vet -test *.go

or (list all flags)

go tool vet -asmdecl -assign .... -shadow ... -unusedresult  *.go

Note that the following command runs only shadow check:

go tool vet -all -shadow *.go 

In addition to all this there is quite useful flag called -shadowstrict that makes shadow check more strict.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 21, 2015

@kostya-sh Ah, so shadow enables just one specific check for shadowed variables. Sorry, that wasn't clear to me. I'm not sure how we can handle that, though.

I'd argue that vet's interface is simply broken: Enabling a specific check shouldn't disable all others. No other compiler I know of would do that: If you run clang with -Wunused it doesn't disable all other warnings either.

@kostya-sh

This comment has been minimized.

Contributor

kostya-sh commented Oct 21, 2015

@lunaryorn, I agree that vet interface can be improved however:

  • even if this issue is fixed it won't be released until Go 1.6 (Feb 2016)
  • if flycheck starts using the new fixed interface it won't work with the previous versions of the tool

Are there any particular reasons you do not want to add option to specify command line arguments? Such option seems pretty reasonable to me, it gives a user flexibility to configure go vet invocations without need to re-implement support for all past and future arguments in flycheck.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 21, 2015

@kostya-sh I'm beginning to think that it wasn't a good idea to add Go support to Flycheck. It's caused more trouble than almost all other languages so far, mostly because of their own peculiar ideas of command line interfaces and project layout 😒

I don't like explicit arguments because they don't play well with customise and don't compose—you can't set one flag globally, and another locally via local variables, without custom code. I add them where necessary—C++, for instance, where it's common to take flags from a build system—but I'm not convinced that supporting an experimental flag of a tool is good enough a reason to allow arbitrary flags for go vet.

@kostya-sh

This comment has been minimized.

Contributor

kostya-sh commented Oct 21, 2015

@lunaryorn, I find Go support in flycheck very useful and it works extremely well.

After the installation of flycheck out of the box (no configuration was necessary) it works with any Go file in my GOPATH, even with the source code of the Go compiler and the standard library. The feedback is almost instant as well. IBM had to re-implement Java compiler to make this possible for java in Eclipse!

So please don't get discouraged by occasional problems and keep supporting it in flycheck! ;)

Shadow check is experimental mostly because it is extremely difficult to perform this analysis accurately and it currently reports false positives. The check is still very useful and helps making less mistakes during development.

You mentioned that having a second checker is unreasonable. What are the reasons for that? ;)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 22, 2015

@kostya-sh The complexity of the syntax checker chain grows with the number of checkers, and the Go chain is already somewhat complex.

A separate checker might be the best solution, though, as long as shadow analysis is experimental in go vet, but that's not a syntax checker suitable for Flycheck core.

However, you can easily write a separate syntax checker outside of Flycheck and hook it into the existing syntax checker chain, with flycheck-add-next-checker. So a separate flycheck-go-vet-shadow package with a go-vet-shadow syntax checker might be the best option imho.

lunaryorn added a commit that referenced this issue Mar 2, 2016

Add option to check for shadowing in go vet
Starting from Go 1.6 "go vet -all -shadow" means all default checkers
and also shadow checker. This allows to add flycheck option to enable
shadow check.

See GH-765, closes GH-897.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment