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

Move to fieldalignment linter from deprecated maligned linter #1252

Closed
dfarrell07 opened this issue Apr 9, 2021 · 4 comments · Fixed by #1306
Closed

Move to fieldalignment linter from deprecated maligned linter #1252

dfarrell07 opened this issue Apr 9, 2021 · 4 comments · Fixed by #1306
Assignees

Comments

@dfarrell07
Copy link
Member

What would you like to be added:

Move from the deprecated maligned linter to the new fieldalignment linter.

Why is this needed:

Upgrading to golangci-lint 1.39.0 in submariner-io/shipyard#519:

Deprecated linters:

  • maligned, replaced by fieldalignment
@dfarrell07 dfarrell07 added this to the 0.9-rc0 milestone Apr 9, 2021
@dfarrell07 dfarrell07 self-assigned this Apr 9, 2021
@tpantelis tpantelis removed this from the 0.9-rc0 milestone Apr 20, 2021
@dfarrell07
Copy link
Member Author

This is also reported as a warning in Go linting:

The linter 'maligned' is deprecated (since v1.38.0) due to: The repository of the linter has been archived by the owner. Replaced by govet 'fieldalignment'."

@skitt skitt assigned skitt and unassigned dfarrell07 May 4, 2021
skitt added a commit to skitt/submariner that referenced this issue May 4, 2021
fieldalignment replaces maligned. This currently raises a number of
linting errors because fieldalignment cares about pointer ordering
(for GC); see golang/go#44877 for details.

Fixes: submariner-io#1252
Signed-off-by: Stephen Kitt <[email protected]>
@skitt
Copy link
Member

skitt commented May 5, 2021

The switch itself is implemented in #1306, however it shows that fieldalignment flags more issues than maligned: it checks struct size, and “pointer size”, i.e. how much of the struct needs to be scanned for GC. This is really a minor optimisation (see golang/go#44877 for details) but it can’t be disabled.

Further, golangci-lint can use maligned’s suggestions, at least to present them to the user; fieldalignment is also capable of producing suggestions, but golangci-lint can’t use them yet (see golangci/golangci-lint#1779). Until that happens, switching to fieldalignment seems counter-productive to me: it is liable to find issues in structs without being able to explain how to fix them, and it does find lots of issues related to pointer coverage which are hard to fix.

@dfarrell07
Copy link
Member Author

dfarrell07 commented May 5, 2021

Okay, thanks for digging in and summarizing so clearly @skitt.

Can we add golangci-lint ignores for this linter matching text "pointer size" (or similar) to turn fieldalignment more into a drop-in replacement for maligned?

Further, golangci-lint can use maligned’s suggestions, at least to present them to the user; fieldalignment is also capable of producing suggestions, but golangci-lint can’t use them yet

I don't see either fieldalignment or maligned mentioned in the supported linters there, but I do think I remember maligned's suggestions.

So maybe the plan could be something like:

  1. Wait until fieldalignment and maligned have suggestion parity.
  2. Use ignore rules for "pointer size" if possible, otherwise wait on ability to disable that check, and then phase in the struct scanning optimizations if we want later.

skitt added a commit to skitt/submariner that referenced this issue May 5, 2021
fieldalignment replaces maligned. Errors related to pointer ordering
(for GC) are ignored; see golang/go#44877
for details.

Fixes: submariner-io#1252
Signed-off-by: Stephen Kitt <[email protected]>
@skitt
Copy link
Member

skitt commented May 5, 2021

Can we add golangci-lint ignores for this linter matching text "pointer size" (or similar) to turn fieldalignment more into a drop-in replacement for maligned?

Ah yes, I’d forgotten we could do that! I’ve added the relevant configuration in the updated PR. If it passes everything here I’ll mark it as ready for review.

skitt added a commit to skitt/submariner that referenced this issue May 5, 2021
fieldalignment replaces maligned. Errors related to pointer ordering
(for GC) are ignored; see golang/go#44877
for details.

Fixes: submariner-io#1252
Signed-off-by: Stephen Kitt <[email protected]>
tpantelis pushed a commit that referenced this issue May 6, 2021
fieldalignment replaces maligned. Errors related to pointer ordering
(for GC) are ignored; see golang/go#44877
for details.

Fixes: #1252
Signed-off-by: Stephen Kitt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants