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

Support -all flag for unconvert linter #1809

Closed
zaynetro opened this issue Mar 2, 2021 · 7 comments
Closed

Support -all flag for unconvert linter #1809

zaynetro opened this issue Mar 2, 2021 · 7 comments
Labels
dependencies Relates to an upstream dependency won't fix This will not be worked on

Comments

@zaynetro
Copy link

zaynetro commented Mar 2, 2021

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 zaynetro added the enhancement New feature or improvement label Mar 2, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 2, 2021

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

@SVilgelm
Copy link
Member

SVilgelm commented Mar 3, 2021

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.

@ldez ldez added the dependencies Relates to an upstream dependency label Mar 3, 2021
@zaynetro
Copy link
Author

zaynetro commented Mar 3, 2021

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

@zaynetro
Copy link
Author

zaynetro commented Mar 18, 2021

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?

@zak-pawel
Copy link

@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
mdempsky/unconvert#40

@stale
Copy link

stale bot commented Mar 30, 2022

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 stale bot added the stale No recent correspondence or work activity label Mar 30, 2022
@ldez ldez removed the stale No recent correspondence or work activity label Mar 30, 2022
@ldez ldez added won't fix This will not be worked on and removed enhancement New feature or improvement labels Mar 9, 2024
@ldez
Copy link
Member

ldez commented Mar 9, 2024

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 ldez closed this as completed Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Relates to an upstream dependency won't fix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants