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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scrub clean in staticcheck. Add staticcheck to .travis.yml file #393

Merged
merged 3 commits into from Apr 17, 2017

Conversation

Projects
None yet
3 participants
@y0ssar1an
Copy link
Contributor

y0ssar1an commented Apr 16, 2017

staticcheck is an awesome static analyzer that's described by the author as "go vet on steroids". IMHO it should be a part of every Go project. It's like having an extra full-time bug-finding engineer on the team. The list of bugs it can catch is quite extensive.

The good news is: staticcheck didn't find any serious bugs in dep. 馃帀 Well done, guys! Nice coding 馃槃

The minor issues it did find are...

  1. a missing err check
  2. use of a deprecated stdlib func
  3. an ineffective, but innocent break statement that was intended as a no-op

These are small issues, but I think there's a clear benefit in catching future bugs before they're merged.

@googlebot googlebot added the cla: yes label Apr 16, 2017

- go test -race $(go list ./... | grep -v vendor)
- go build ./hack/licenseok
- find . -path ./vendor -prune -o -type f -name "*.go" -printf '%P\n' | xargs ./licenseok
- ./hack/validate-vendor.bash

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

I deindented the YAML file by one level. I'm not sure if the indentation was added intentionally, but I'm happy to reverse that change.

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

No worries, this is fine 馃槃

- echo "This is an override of the default install deps step in travis."
before_script:
- PKGS=$(go list ./... | grep -v /vendor/)
- go get -v honnef.co/go/tools/cmd/staticcheck

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

I added the before_script section so that fetching staticcheck and collecting the $PKGS list wouldn't pollute the logs. I also changed grep -v vendor to grep -v /vendor/ to ensure that only the vendor directory is filtered.

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

SGTM!

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Oh also, might want to apply a similar /vendor/ change to the grep below?

@@ -43,7 +43,7 @@ func (cmd *initCommand) Register(fs *flag.FlagSet) {}
type initCommand struct{}

func trimPathPrefix(p1, p2 string) string {
if filepath.HasPrefix(p1, p2) {
if strings.HasPrefix(p1, p2) {
return p1[len(p2):]
}
return p1

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

filepath.HasPrefix just calls strings.HasPrefix. The reasons filepath.HasPrefix was deprecated are described here. We should double-check this code this code for correctness. For now, I think using strings.HasPrefix is an improvement. It makes clear this is a pure string comparison. There isn't any added safety from using filepath (like you would get from using filepath.Join instead of strings.Join).

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Yeah, this has been raised, and we have an extensive issue discussing it - #296. Let's leave this one as-is for now, and deal with it over there.

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

Wow! That's a much deeper rabbit hole than I expected! 馃暢馃悋 I'm glad smart people are already looking into this 馃槂. I've reverted this change and added an --ignore flag to tell staticcheck to ignore filepath.HasPrefix in cmd/dep/init.go and context.go.

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Awesome, thank you!

@@ -78,6 +78,9 @@ func (tc *IntegrationTestCase) Cleanup() {
j = bytes.Replace(j, cmds, n, -1)
j = append(j, '\n')
err = ioutil.WriteFile(filepath.Join(tc.rootPath, "testcase.json"), j, 0666)
if err != nil {
tc.t.Logf("Failed to update testcase %s: %s", tc.name, err)
}

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

If writing the updated golden file fails, we should at least log the error. Should we complain more loudly than that?

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Yes - let's upgrade that to an Errorf. Especially because, if we don't, there's no guarantee the user would even see the log message (if the test otherwise passed).

@@ -464,8 +464,6 @@ func diffLocks(l1 gps.Lock, l2 gps.Lock) *LockDiff {
diff.Add = append(diff.Add, add)
i2next = i2 + 1 // Don't evaluate to this again
continue // Keep looking for a matching project
case +1: // Project has been removed, handled below
break

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

This break is an innocent no-op. staticcheck looks for cases where the author mistakenly thought they were breaking out of an outer loop, but were really just breaking out of the switch. That seems like a good check to have. I think it's worth deleting this innocent code if it lets us incorporate staticcheck into CI.

We could also make staticcheck happy by doing this:

case +1: // Project has been removed, handled below
    // no code here

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

I think it probably makes it a bit more visually complete for the casual reader not necessarily familiar with strings.Compare() to go with the empty branch alternative.

@y0ssar1an y0ssar1an changed the title Scrub clean in staticcheck Scrub clean in staticcheck. Add staticcheck to .travis.yml file Apr 16, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

This is great! Adding more static checking like this is definitely something that benefits the project...especially if it's not full of false positives, like some linters are. This seems like a good set.

Just a couple notes...

- go test -race $(go list ./... | grep -v vendor)
- go build ./hack/licenseok
- find . -path ./vendor -prune -o -type f -name "*.go" -printf '%P\n' | xargs ./licenseok
- ./hack/validate-vendor.bash

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

No worries, this is fine 馃槃

- echo "This is an override of the default install deps step in travis."
before_script:
- PKGS=$(go list ./... | grep -v /vendor/)
- go get -v honnef.co/go/tools/cmd/staticcheck

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

SGTM!

- echo "This is an override of the default install deps step in travis."
before_script:
- PKGS=$(go list ./... | grep -v /vendor/)
- go get -v honnef.co/go/tools/cmd/staticcheck

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Oh also, might want to apply a similar /vendor/ change to the grep below?

@@ -78,6 +78,9 @@ func (tc *IntegrationTestCase) Cleanup() {
j = bytes.Replace(j, cmds, n, -1)
j = append(j, '\n')
err = ioutil.WriteFile(filepath.Join(tc.rootPath, "testcase.json"), j, 0666)
if err != nil {
tc.t.Logf("Failed to update testcase %s: %s", tc.name, err)
}

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Yes - let's upgrade that to an Errorf. Especially because, if we don't, there's no guarantee the user would even see the log message (if the test otherwise passed).

@@ -464,8 +464,6 @@ func diffLocks(l1 gps.Lock, l2 gps.Lock) *LockDiff {
diff.Add = append(diff.Add, add)
i2next = i2 + 1 // Don't evaluate to this again
continue // Keep looking for a matching project
case +1: // Project has been removed, handled below
break

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

I think it probably makes it a bit more visually complete for the casual reader not necessarily familiar with strings.Compare() to go with the empty branch alternative.

Ryan Boehning added some commits Apr 16, 2017

Ryan Boehning
Add missing err check when updating a golden file
Report the error when we fail to write the updated golden file to disk.
The missing error check was caught by staticcheck.
Ryan Boehning
Remove ineffective break statement from diffLocks
A switch statement in diffLocks compares two project roots. One of the
cases is basically a no-op that just breaks out of the switch and does
nothing else. Unfortunately, staticcheck complains about this no-op
code. This commit just deletes the break to make staticcheck happy.
.travis.yml Outdated
- go build -v ./cmd/dep
- go vet $PKGS
- staticcheck $STATICCCHECK_IGNORE $PKGS
- test -z "$(gofmt -s -l . 2>&1 | grep -v /vendor/ | tee /dev/stderr)"

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

whoops - looks like this needs to be grep -v vendor/ instead of grep -v /vendor/

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 16, 2017

Contributor

Good catch! Fixed.

@y0ssar1an

This comment has been minimized.

Copy link
Contributor

y0ssar1an commented Apr 16, 2017

Great feedback, @sdboyer! I agree with all your suggestions. If you wouldn't mind, can you take a second look at .travis.yml and let me know if that looks okay to you?
EDIT: lol I keep getting the quotes on $STATICCHECK_IGNORE wrong. I do so love shell script quoting rules 馃槂
EDIT 2: I gave up on $STATICCHECK_IGNORE and now just have a single long -ignore flag. You win this round, bash 馃樀

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 16, 2017

Hmmm...failures on some go versions but not others from staticcheck. 馃槩 Inconsistency is a little worrisome.

If it's easier, I'd be fine with replacing the direct calls to filepath.HasPrefix() with a call to a single function that, for now, just calls filepath.HasPrefix(), but the work in #296 can then ultimately replace. At least that way there'll only be one spot to ignore in staticcheck.

Ryan Boehning
Add staticcheck to .travis.yml
Add the $PKGS var, to avoid having to type $(go list ./... | grep -v
/vendor/) over and over.
Add leading and trailing slashes to vendor (vendor -> /vendor/) so that
a go files containing "vendor" won't be filtered.
Remove unnecessary indentation.
Add the before_script section, so $PKGS can be calculated and
staticcheck can be fetched without polluting the logs.
Add the $STATICCHECK_IGNORE var, which tells staticcheck to ignore the
deprecation warning for filepath.HasPrefix in two files.
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 17, 2017

LGTM now. Thanks for this - and, yay first contribution! May there be many more 馃槈

@sdboyer sdboyer merged commit bca5bdb into golang:master Apr 17, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#393 from y0ssar1an/master
Scrub clean in staticcheck. Add staticcheck to .travis.yml file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment