Skip to content
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

Implement a checker for the golang `staticcheck` tool #1541

Merged
merged 7 commits into from Mar 19, 2019

Conversation

@Gastove
Copy link
Contributor

commented Jan 27, 2019

Commit Message:

This commit implements go-staticcheck, a checker wrapping the staticcheck
tool. staticcheck is the successor to megacheck, which is now
deprecated.

This commit includes:

  1. A checker definition for go-staticcheck.
  2. The introduction of a new variable, flycheck-go-version, which can be
    passed to staticcheck to specify the version of the Go compiler to perform
    checks compatible with.
  3. A test for staticcheck.
  4. Updates to appropriate documentation.
  5. Updates to other Golang syntax checkers to prefer falling back to
    staticcheck before megacheck.

The documentation pass includes:

  1. rst docs for go-staticcheck itself.
  2. An update to the documentation for megacheck, specifying that it's
    deprecated.
  3. Documentation for the new flycheck-go-version variable.
  4. Updating the definition of flycheck-go-build-tags to correctly declare the
    full list of checkers that can use it.

A Note about this PR

So! Went to get this tested, and in doing so, discovered a few things. While the specs all pass on my local machine, the integration tests for go... do not. This is because the tests for Go tooling are hardcoded with the go binary installed in /usr/local/bin; on Fedora, at least, go is installed in /usr/bin, so those tests fail as well.

I'm currently testing changes to the all-tools Dockerfile; I'll make sure that PR is linked to this one once I've got everything together. My worldview here is that this means the tests on this PR wont pass until my PR is merged in to docker-tools, which is fine.

I'll look in to updating the tests to more flexibly find the go binary if I have time!

Thanks for a great tool, y'all, and my complements on superb contributor docs!

@CLAassistant

This comment has been minimized.

Copy link

commented Jan 27, 2019

CLA assistant check
All committers have signed the CLA.

Gastove added a commit to Gastove/docker-tools that referenced this pull request Jan 27, 2019

Bump go version to current, install staticcheck
This commit adds the `staticcheck` tool to this Docker image, for testing the
new [`go-staticcheck`](flycheck/flycheck#1541) linter.
@fmdkdd

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Hey, sorry for the delay. This is a very nice PR, thank you. I'll try to review it soon enough.

flycheck.el Outdated Show resolved Hide resolved
flycheck.el Show resolved Hide resolved
@fmdkdd

fmdkdd approved these changes Feb 21, 2019

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

LGTM, but you will need to clear the CI before we can merge. Currently it's failing on formatting errors in test-go.el. I've merged flycheck/docker-tools#2 so your checker tests should also run now.

@mpolden

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Great work, @Gastove! Any chance of fixing the formatting errors and getting this merged soon?

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Hello! Apologies -- Life happened. Back now! Will get this cleaned up quick as I can 👍

Gastove added some commits Jan 27, 2019

Implement a checker for the golang `staticcheck` tool
This commit implements `go-staticcheck`, a checker wrapping the `staticcheck`
tool. `staticcheck` is the successor to `megacheck`, which is now
deprecated.

This commit includes:

1. A checker definition for `go-staticcheck`.
2. The introduction of a new variable, `flycheck-go-version`, which can be
passed to `staticcheck` to specify the version of the Go compiler to perform
checks compatible with.
3. A test for `staticcheck`.
4. Updates to appropriate documentation.
5. Updates to other Golang syntax checkers to prefer falling back to
`staticcheck` before `megacheck`.

The documentation pass includes:

1. `rst` docs for `go-staticcheck` itself.
2. An update to the documentation for `megacheck`, specifying that it's
deprecated.
3. Documentation for the new `flycheck-go-version` variable.
4. Updating the definition of `flycheck-go-build-tags` to correctly declare the
full list of checkers that can use it.
PR Feedback + Enhancements
This commit changes several things:

1. Based on PR feedback from @fmdkdd, don't force `staticcheck` to run in the
local directory -- it's fine, but unnecessary.
2. Switch to using `staticcheck`'s JSON output; add a parser for it. This allows
parsing error severity, which is excellent.
3. Add a buttercup test for `flycheck-parse-go-staticcheck`, and update the
`staticcheck` integration test to correctly reflect new error parsing.

@Gastove Gastove force-pushed the Gastove:go-staticcheck branch from 6c5cc80 to 5e234ff Mar 19, 2019

@Gastove Gastove force-pushed the Gastove:go-staticcheck branch from 21f7d31 to 6ed2756 Mar 19, 2019

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Hokay. Now passing format and checkdoc; the buttercup tests are passing as well. If I'm reading the (pretty extensive) CI output correctly, I'm currently failing the integration tests. Let me see what I can do about that.

Disable go-megacheck in staticcheck tests and staticcheck in megachec…
…k, build tests

I'm trying to fix errors in CI I can't reproduce locally. My current hypothesis
is that megacheck is stomping on staticcheck, and staticcheck is harming both
megacheck and go-build.
@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Okay. I'm down to one last failing test. I'm.... pretty baffled by the error I'm getting, however. Can anyone advise? @fmdkdd?

The error originates on line 3436 of flycheck-test.el. Here's the test:

(flycheck-ert-def-checker-test go-staticcheck go nil
  :tags '(language-go external-tool)
  (let ((flycheck-disabled-checkers '(go-golint go-megacheck)))
    (flycheck-ert-with-env
        `(("GOPATH" . ,(flycheck-ert-resource-filename "language/go")))
      (flycheck-ert-should-syntax-check
       "language/go/src/staticcheck/staticcheck1.go" 'go-mode
       '(8 6 error "should omit values from range; this loop is equivalent to `for range ...`"
           :checker go-staticcheck :id "S1005")
       '(12 21 error "calling strings.Replace with n == 0 will return no results, did you mean -1?"
            :checker go-staticcheck :id "SA1018")
       '(16 6 error "func unused is unused"
            :checker go-staticcheck :id "U1000")))))

The error from ert is different-types, like so:

Test flycheck-define-checker/go-staticcheck/default condition:
    (ert-test-failed
     ((should
       (equal
	(mapcar #'flycheck-error-without-group expected)
	(mapcar #'flycheck-error-without-group flycheck-current-errors)))
      :form
      (equal
       (#s(flycheck-error #<killed buffer> go-staticcheck "/flycheck/test/resources/language/go/src/staticcheck/staticcheck1.go" 8 6 "should omit values from range; this loop is equivalent to `for range ...`" error "S1005" nil)
	  #s(flycheck-error #<killed buffer> go-staticcheck "/flycheck/test/resources/language/go/src/staticcheck/staticcheck1.go" 12 21 "calling strings.Replace with n == 0 will return no results, did you mean -1?" error "SA1018" nil)
	  #s(flycheck-error #<killed buffer> go-staticcheck "/flycheck/test/resources/language/go/src/staticcheck/staticcheck1.go" 16 6 "func unused is unused" error "U1000" nil))
       nil)
      :value nil :explanation
      (different-types
       (#s(flycheck-error #<killed buffer> go-staticcheck "/flycheck/test/resources/language/go/src/staticcheck/staticcheck1.go" 8 6 "should omit values from range; this loop is equivalent to `for range ...`" error "S1005" nil)
	  #s(flycheck-error #<killed buffer> go-staticcheck "/flycheck/test/resources/language/go/src/staticcheck/staticcheck1.go" 12 21 "calling strings.Replace with n == 0 will return no results, did you mean -1?" error "SA1018" nil)
	  #s(flycheck-error #<killed buffer> go-staticcheck "/flycheck/test/resources/language/go/src/staticcheck/staticcheck1.go" 16 6 "func unused is unused" error "U1000" nil))
       nil)))

Can anyone advise? It reads like ert is trying to compare the three forms provided (all correctly of type flycheck-error) to nil -- which, I agree, is a different type. However, if that's what's happening... how on earth is nil getting in there?

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

If I read this correctly, nil is just an empty list of errors — IOW, it seems that the checker is returning no errors, and the test suite complains about that. Do you see errors if you just open that file and turn on Flycheck?

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@cpitclaudel aaaahaha. Yes, okay -- that does, at least, help with the interpretation of the error message. However, if I load the checker, then the test files, I am seeing the expected triplet of errors. So. Just have to figure out why no errors are being parsed by the integration test.

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Okay, I'm... a bit lost. If I don't disable go-megacheck in the test for go-staticcheck, then the test fails because there are unexpected go-megacheck errors in the test results. However, if I do disable go-megacheck, then the test fails because no errors are produced. It seems like I must have wired something up incorrectly. So far, I'm not at all sure what.

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

THERE. Okay: for reasons I do not understand, disabling go-unconvert for my staticcheck test causes the test to pass. I suspect that this has something to do with the way :next-checker has been defined for some of the Go family of checkers. However, I don't understand what's going on clearly enough to feel confident in proposing changes, and I don't want to expand the scope of this PR any further. My inclination is to leave things where they are; please let me know if further changes are required!

@cpitclaudel cpitclaudel merged commit d48181d into flycheck:master Mar 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

👏 🎉 Thanks a lot! Looking forward to many more contributions :)

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

(I should have requested a squash — or performed it myself — sorry for the commit history noise @fmdkdd !)

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

@cpitclaudel Arguably, the only benefit of squashing is when using git blame. The downside is added friction when merging PRs. (I've re-enabled the squash & merge button, so at least we can squash ourselves when commits are not signed).

@Gastove Adding a new checker to a chain will usually have effects on other checker tests, as you figured out. I wish the test output would make that explicit, but the thing to remember is that tests are meant to mimic the behavior of Flycheck from a user standpoint: when you open a file, the first valid checker is selected, then its :next-checker will be run. So if you add a checker, you may have to disable another one in order for your checker to be run.

Anyway, thanks for pulling through on this PR! 👍

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@fmdkdd oh yes! I quite like :next-checker as a feature for users, and have made use of it myself! That said: the thing that was specifically bewildering me was that disabling go-megacheck in my tests also appeared to disable go-staticcheck. That is: with go-megacheck enabled, my go-staticcheck test results would contain results for both linters (expected); with go-megacheck disabled, the linting check would return nil. The solution was to disable go-unconvert, which... I still don't grok.

I can make a to-do for myself to be sure that behaviour isn't present for users, submit any follow-up work in a new PR.

Given that megacheck is both deprecated and its checks have been fully subsumed by staticcheck, I'd also be in favour of a "should activate" condition on megacheck that disables it if staticcheck is available, or similar. Thoughts?

@Gastove Gastove deleted the Gastove:go-staticcheck branch Mar 21, 2019

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

If megacheck has been deprecated and superseded, we should just drop it altogether. Users who wish to use the checker may still copy its definition to their own config.

If you have time for this, please do open a PR.

@Gastove

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Works for me. I'll open an issue, just so I don't lose track of it; I should have time this weekend 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.