golangci-lint icon indicating copy to clipboard operation
golangci-lint copied to clipboard

Support -all flag for unconvert linter

Open zaynetro opened this issue 3 years ago • 6 comments

Is your feature request related to a problem? Please describe.

unconvert supports -all flag but there is no way you can use it through golang-ci.

Describe the solution you'd like

It would be nice to run unconvert against multiple OSes.

Describe alternatives you've considered

Duplicate a method on Linux and FreeBSD build specific files.

Additional context

golang.org/x/sys/unix exports Statfs_t struct which has a Bavail field. Unfortunately, on FreeBSD it is int64 compared to uint64 on Linux.

Sample code that compiles on both platforms but produces the lint error on Linux:

pkg/storage/fs/ocis/ocis_unix.go:31:18: unnecessary conversion (unconvert)
    bavail := uint64(unix.Statfs_t{}.Bavail)

I have followed these steps until I got stuck:

  • Add Unconvert config to pkg/config/config.go
  • Modify test file test/testdata/unconvert.go
  • Pass config to unconvert in pkg/golinters/unconvert.go
  • Modify unconvert to test against all platforms <-- stuck here

The problem is that unconvert builds loader.Program dynamically for each OS and architecture but golangci-lint passes its own loader.Program. How do I build the Program for other configurations?

On the other hand the up-to-date unconvert no longer uses loader.Program. Does it make sense to update the golangci-lint fork based on the latest changes?

UPD: Looking at the latest unconvert it should be much easier to support -all flag.

zaynetro avatar Mar 02 '21 21:03 zaynetro

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

boring-cyborg[bot] avatar Mar 02 '21 21:03 boring-cyborg[bot]

I still don't see a way how can we replace the fork with an upstream repo. The main repo does not provide the public methods, it cannot be used as a library. It will be great if you can modify the upstream repo to support analysis.Analyzer interface.

SVilgelm avatar Mar 03 '21 04:03 SVilgelm

Let's see if there is any interest in unconvert project: https://github.com/mdempsky/unconvert/issues/52 If not I can update golangci-lint fork.

zaynetro avatar Mar 03 '21 15:03 zaynetro

Seems like there is no interest in the original repo. I will continue with a fork update.

I fail to understand how can I modify the library to use analysis.Analyzer interface. The problem I have is how to go over files for all platforms. In my local tests I figured out only how to go over files for a current platform.

I will update the fork and try to keep the current interface. With someone's help I might introduce Analyzer interface support.

UPD: Hmm golangci uses Analyzer when calling unconvert. So the issue remains: how can I go through files for all platforms in unconvert lib?

zaynetro avatar Mar 18 '21 13:03 zaynetro

@zaynetro It would be great if you can introduce also flagFastMath config in fork to have possibility to disable/enable conversions that force intermediate rounding.

More info: https://stackoverflow.com/questions/63544888/delete-conversion-changes-semantics https://github.com/mdempsky/unconvert/issues/40

zak-pawel avatar Mar 22 '21 20:03 zak-pawel

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 30 '22 06:03 stale[bot]

Technically I think we will not be able to add the flag all because this element has not been designed to be used other than inside unconvert and it doesn't fit with how golangci-lint works.

But I updated the fork inside PR #4473 and now you will be able to use safe. The element like bavail := uint64(unix.Statfs_t{}.Bavail) are ignored with safe=true.

I closed the issue based on my previous explanation.

ldez avatar Mar 09 '24 02:03 ldez