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

lint: update staticcheck to 2021.1.1 #72313

Merged
merged 10 commits into from
Nov 19, 2021

Conversation

stevendanna
Copy link
Collaborator

This version contains performance improvements according to the
upstream changelog.

The biggest downside is that the whole-program mode has been removed
from the unused linter. For information about why it was removed see

https://staticcheck.io/changes/2020.2/#unused-whole-program

See individual commits for fixes to new violations added by the new version.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna
Copy link
Collaborator Author

Not very scientific, but on this branch it ran a bit faster than the last run on master.

Master:
Screenshot 2021-11-02 at 10 40 00

Branch:
Screenshot 2021-11-02 at 10 39 26

@stevendanna stevendanna marked this pull request as ready for review November 2, 2021 17:32
@stevendanna stevendanna requested a review from a team November 2, 2021 17:32
@stevendanna stevendanna requested review from a team as code owners November 2, 2021 17:32
@stevendanna stevendanna requested a review from a team November 2, 2021 17:32
@stevendanna stevendanna requested review from a team as code owners November 2, 2021 17:32
@stevendanna stevendanna requested review from dt and removed request for a team November 2, 2021 17:32
@stevendanna
Copy link
Collaborator Author

I don't feel particularly strongly about this. I was hoping the speedup would be a bit more dramatic.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Thanks for this! We should upgrade regardless - it will make it easier to stay up to date in the future.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt)

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I also think this should be merged.

pkg/geo/geomfn/angle.go Show resolved Hide resolved
@stevendanna stevendanna force-pushed the update-staticcheck branch 2 times, most recently from fc45cc1 to e845007 Compare November 4, 2021 12:22
@stevendanna
Copy link
Collaborator Author

We'll need to hold off merging this for a minute. It currently likely causes TestRoachVet to fail on macOS. It appears that one of the updates we pulled in means that we now see the build warning described in #70819 as part of the roachvet output. Unfortunately, that means that RoachVet fails since it assumes any output is a failure.

@stevendanna
Copy link
Collaborator Author

#72686 should resolve the warning

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 34 of 34 files at r21, 3 of 3 files at r22, 1 of 1 files at r23, 1 of 1 files at r24, 20 of 20 files at r25, 1 of 1 files at r26, 1 of 1 files at r27, 1 of 1 files at r28.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @srosenberg, @stevendanna, and @tbg)


pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go, line 423 at r29 (raw file):

}

func (h *handler) invoke(update roachpb.Span) {

Another bug...none of the below updates to h were previously persisted, so why wasn't anything downstream affected?


pkg/sql/inverted/expression_test.go, line 84 at r27 (raw file):

func (u *UnknownExpression) IsTight() bool { return u.tight }
func (u *UnknownExpression) SetNotTight()  { u.tight = false }

Surprised nothing else was broken considering the previous setter was effectively no-op?

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @srosenberg, @stevendanna, and @tbg)

@stevendanna stevendanna force-pushed the update-staticcheck branch 2 times, most recently from 681ca46 to cad791b Compare November 18, 2021 10:28
This lint checks for the use of context-aware APIs that aren't
supported by DriverConn.

Release note: None
This lint checks for unnecessary uses of Sprintf. I don't love this
linter, but there were only a few violations of it.

Release note: None
This lint checks for uses of regular expressions in a loop where the
regular expresion compilation can be hoisted.

Release note: None
This removes type assertions on variables that are already the type
being asserted. This isn't a completely equivalent transformation,
since in the case of nil interface types, these type assertions would
have previously produced a panic.

Release note: None
A comparison with the untyped nil here would never be true in this
case because of the cast.

Release note: None
The SetNotTight function modifies the receiver so needs a pointer
receiver to be useful.

Release note: None
strings.TrimSuffix does a HasSuffix check, so it can be called
unconditionally.

Release note: None
This method sets the `initialized` field, so needs a pointer receiver
for the assignment to be effective.

Release note: None
This version contains performance improvements according to the
upstream changelog.

The biggest downside is that the whole-program mode has been removed
from the unused linter. For information about why it was removed see

https://staticcheck.io/changes/2020.2/#unused-whole-program

This PR also updates the following additional dependencies

- [github.com/BurntSushi/toml v0.3.1...v0.4.1](BurntSushi/toml@v0.3.1...v0.4.1)
- [golang.org/x/mod v0.4.2...v0.5.1](golang/mod@v0.4.2...v0.5.1)
- [golang.org/x/sys 69cdffdb9359...a2f17f7b995](golang/sys@69cdffd...a2f17f7)
- [golang.org/x/tools v0.1.5...v0.1.7](golang/tools@v0.1.5...v0.1.7)

Release note: None
@stevendanna
Copy link
Collaborator Author

bors r=srosenberg,RaduBerinde,tbg

@craig
Copy link
Contributor

craig bot commented Nov 19, 2021

Build succeeded:

@craig craig bot merged commit 28bb1ea into cockroachdb:master Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants