celestiaorg / go-header

Go library with all the services needed to request, sync and store blockchain headers.
Apache License 2.0
19 stars 18 forks source link

ci(lint): golangci-lint and fieldalignment #132

Closed ramin closed 3 months ago

ramin commented 10 months ago

At suggestion and request of @Wondertan, implemented govet field alignment linting for struct alignment/padding checks, and subsequently committed fixes for them, as we did in https://github.com/celestiaorg/celestia-node/pull/2856

Needed to take a bit of a quest to get here, steps from volume 1:

Now we have lint errors. Volume 2 was fixin'. Fixes for lint errors golangci-lint found that are corrected in this PR:

Finally, time for Return of the King. We then had "just" the fieldalignment optimizations, which have since been re-aligned. For example, here was the suggested output that was since altered

headertest/dummy_header.go:19:18: fieldalignment: struct with 80 pointer bytes could be 72 (govet)
type DummyHeader struct {
                 ^
p2p/exchange.go:36:35: fieldalignment: struct with 152 pointer bytes could be 144 (govet)
type Exchange[H header.Header[H]] struct {
                                  ^
p2p/exchange_metrics.go:29:22: fieldalignment: struct with 168 pointer bytes could be 144 (govet)
type exchangeMetrics struct {
                     ^
p2p/options.go:20:23: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type ServerParameters struct {
                      ^
p2p/options.go:125:23: fieldalignment: struct with 72 pointer bytes could be 40 (govet)
type ClientParameters struct {
                      ^
p2p/peer_stats.go:13:15: fieldalignment: struct with 72 pointer bytes could be 32 (govet)
type peerStat struct {
              ^
p2p/peer_stats.go:100:16: fieldalignment: struct with 72 pointer bytes could be 32 (govet)
type peerQueue struct {
               ^
p2p/peer_tracker.go:30:18: fieldalignment: struct with 120 pointer bytes could be 96 (govet)
type peerTracker struct {
                 ^
p2p/session.go:31:34: fieldalignment: struct with 112 pointer bytes could be 96 (govet)
type session[H header.Header[H]] struct {
                                 ^
p2p/subscriber.go:26:37: fieldalignment: struct with 48 pointer bytes could be 40 (govet)
type Subscriber[H header.Header[H]] struct {
                                    ^
p2p/subscriber_metrics.go:19:24: fieldalignment: struct with 96 pointer bytes could be 88 (govet)
type subscriberMetrics struct {
                       ^
store/batch.go:16:32: fieldalignment: struct with 40 pointer bytes could be 16 (govet)
type batch[H header.Header[H]] struct {
                               ^
store/heightsub.go:16:36: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
type heightSub[H header.Header[H]] struct {
                                   ^
store/metrics.go:15:14: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
type metrics struct {
             ^
store/options.go:14:17: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type Parameters struct {
                ^
sync/metrics.go:13:14: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
type metrics struct {
             ^
sync/ranges.go:12:33: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type ranges[H header.Header[H]] struct {
                                ^
sync/ranges.go:89:38: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type headerRange[H header.Header[H]] struct {
                                     ^
sync/sync.go:33:33: fieldalignment: struct with 360 pointer bytes could be 312 (govet)
type Syncer[H header.Header[H]] struct {
                                ^
sync/sync.go:128:12: fieldalignment: struct with 136 pointer bytes could be 96 (govet)
type State struct {
           ^
sync/sync_getter.go:12:37: fieldalignment: struct with 48 pointer bytes could be 16 (govet)
type syncGetter[H header.Header[H]] struct {
codecov-commenter commented 10 months ago

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (28ff21c) 63.60% compared to head (5137209) 63.77%.

Files Patch % Lines
store/heightsub.go 0.00% 5 Missing :warning:
p2p/options.go 33.33% 2 Missing :warning:
headertest/subscriber.go 0.00% 1 Missing :warning:
sync/ranges.go 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #132 +/- ## ========================================== + Coverage 63.60% 63.77% +0.16% ========================================== Files 39 39 Lines 3366 3387 +21 ========================================== + Hits 2141 2160 +19 - Misses 1060 1062 +2 Partials 165 165 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Wondertan commented 10 months ago

Queen's English vs the language of freedom

🤣

Wondertan commented 10 months ago

unnecessary conversions (lots of uint64 type casting where the return type is/was already uint64)

That's a relic from the time when int64 was used for heights. I thought we cleaned them up but seems like not

ramin commented 10 months ago

@Wondertan re-aligned some back to reflect idioms etc, have a look and point out any others that maybe want some re-jiggling, not too far from original in most cases

cristaloleg commented 4 months ago

IMO it will be better to not merge these changes 'cause Go will start doing this automatically, see accepted proposal https://github.com/golang/go/issues/66408

Wondertan commented 4 months ago

Agree. Do you mean its fully automatic? My read is that it requires importing and embedding the layout struct

cristaloleg commented 4 months ago

Do you mean its fully automatic?

Of course not. Implicit field from structs package will reorder fields on a specific platform. By automatically I meant that we don't need to reorder fields (manually) when fields will be added/removed.