Add 'floatcompare' linter

[mweb]

https://github.com/mweb/floatcompare

Go linter to search for float comparison. Since floating-point numbers have some issues with rounding, a comparison of float might not return the expected result. Therefore, this linter helps to find possible bugs with floating point number comparison.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

Could you rebase your PR? Thanks.

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description about the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license and the file must contain the required information by the license, ex: author, year, etc.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic(), log.fatal(), os.exit(), or similar.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters.

.golangci.example.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.example.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

• The linter should not use SSA. (currently, SSA does not support generics)
• The linter repository should have a CI, tests, a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

[mweb]

Thanks for the review, I think I have fixed most of the open points. In my view, there are three open points:

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must not have false positives/negatives. (the team will help to verify that)
• The linter should be published as a binary. (useful to diagnose bug origins)

With the first two, I have no idea how I should be doing this, and it is written that you might help. The last one is recommended, this means it is optional? I will have a look how I could provide such a release with my linter.

[mweb]

I added binaries to the release of the linter, and I believe I added a description about the linter to the pull request or do I have to add the description to the commit message?

[ldez]

do I have to add the description to the commit message?

You don't have to squash:

• It's better during a review to address changes as commits, it's easier to follow the changes.
• We squash the commits of the PR during the merge.

So the commit message doesn't need to be updated.

[ldez]

I have some problems with this linter.

go-critic already tried to implement this rule but the rule was removed and moved outside go-critic.

It's unclear why the rule has been pushed outside go-critic but I think it's because of the number of false-positives.

Otherwise, it is closed to https://go-critic.com/overview.html#truncatecmp

[mweb]

I have some problems with this linter.

go-critic already tried to implement this rule but the rule was removed and moved outside go-critic.

It's unclear why the rule has been pushed outside go-critic but I think it's because of the number of false-positives.

This is interesting since the linter implements some mentioned problems in a later pull request.
There are some valid points that the current floatcompare linter doesn't implement, for example x != x for nan checks and the x == x. The other raised point for comparison against numbers, I might consider this to be a configurable parameter since some people might be ok with this in their code.

Would it help if the linter was improved this way, or do you prefer not to have such a linter integrated?

Otherwise, it is closed to https://go-critic.com/overview.html#truncatecmp

On this point, I disagree since these two rules are very different since this checks for truncate problems whereas float comparison is a different problem as you might see in the description of the floatcompare linter and the referenced link.