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

Deprecate/remove interfacer from supported linters #541

Closed
dackroyd opened this issue May 20, 2019 · 19 comments
Closed

Deprecate/remove interfacer from supported linters #541

dackroyd opened this issue May 20, 2019 · 19 comments
Labels
topic: cleanup Related to code, process, or doc cleanup

Comments

@dackroyd
Copy link

mvdan/interfacer@c200402

Deprecated: A tool that suggests interfaces is prone to bad suggestions, so its usefulness in real code is limited. This tool will remain available as a proof of concept, and for others to examine and learn from.

@tpounds tpounds added the topic: cleanup Related to code, process, or doc cleanup label Oct 3, 2019
@jirfag
Copy link
Member

jirfag commented Oct 14, 2019

Hi,

  1. we can't do it before v2.0.0
  2. I'm not sure we should do it because some users may find it useful.
  3. but we can disable it from default set of linters

@jirfag jirfag added this to the v2.0.0 milestone Oct 14, 2019
@andig
Copy link

andig commented Nov 8, 2019

I second this. Interfacer produces false positives, too.

@stale
Copy link

stale bot commented Nov 9, 2020

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 Nov 9, 2020
@sayboras sayboras removed the stale No recent correspondence or work activity label Nov 9, 2020
@sayboras sayboras self-assigned this Nov 9, 2020
@pkuca
Copy link
Contributor

pkuca commented Feb 12, 2021

Interfacer is used to create a fair comparison with gometalinter in the benchmarks - how should that be handled?

#1689 is a dupe

@ldez
Copy link
Member

ldez commented Feb 12, 2021

gometalinter is now archived so a fair comparison with a dead project seems impossible.
https://github.com/alecthomas/gometalinter#go-meta-linter

@pkuca
Copy link
Contributor

pkuca commented Feb 12, 2021

Agreed - knowing that, I can take this unless @sayboras has stuff in progress I see it's queued up for v2, so nevermind.

@sayboras sayboras removed their assignment Feb 13, 2021
@sayboras
Copy link
Member

Feel free to take over this. Thanks 💯

@pkuca
Copy link
Contributor

pkuca commented Feb 22, 2021

Is it worth leaving this open until v2 and the removal PR's start coming in? I'd probably just close it because the interfacer is deprecated properly now (thanks @SVilgelm )

@SVilgelm
Copy link
Member

I think, it's better to create an issue and label it as v2 and we will re-open this PR later

@richardwilkes
Copy link

There should be a way to disable this warning for those of us that still want to use things like interfacer with golangci-lint. As it is, since the introduction of this, our tests now fail because of this (spurious to us) warning.

@ldez
Copy link
Member

ldez commented Mar 4, 2021

@richardwilkes as the linter is deprecated by the author itself, we recommend disabling this linter.

There is an internal hidden flag to hide this message but without any warranty to be stable in the future.

@richardwilkes
Copy link

Ah... what is that flag? Not 100% necessary, as I found I can filter the stderr easily enough... but I actually find both interfacer and maligned to be useful. And the option to use govet's fieldalignment is a non-starter, as it is just plain wrong in its analysis.

@ldez
Copy link
Member

ldez commented Mar 4, 2021

maligned has been also deprecated by the author itself: https://github.com/mdempsky/maligned

maligned has been replaced by fieldalignment linter in govet

"The repository of the linter has been archived by the owner. Use govet 'fieldalignment' instead."

https://golangci-lint.run/usage/configuration/

 govet:
    enable:
      - fieldalignment

or

 govet:
    enable-all: false

@ldez
Copy link
Member

ldez commented Mar 4, 2021

Those linters will be removed in the future, so it's time to disable it from your configuration 😉

@richardwilkes
Copy link

Understood... however, removing a linter that provides a valuable service (like maligned) when the replacement doesn't actually work correctly, is not good. It doesn't seem to understand the correct sizes of struct fields and issues warnings that are incorrect. Example -- for this struct:

type foo struct {
    a string
    b []string
    c string
}

govet will report "fieldalignment: struct with 48 pointer bytes could be 40 (govet)"

However, using unsafe.Sizeof() shows the struct is actually 56 bytes, with 16 bytes consumed for each string and 24 for the slice. With 8 byte alignment on my platform, there is nothing that can be improved. maligned works fine for this same case.

@ldez
Copy link
Member

ldez commented Mar 4, 2021

I suggest you open an issue here: https://github.com/golang/go/issues/new/choose

The issues related to golang.org/x/tools are globally handle quickly as it's not a part of the core.

drpaneas added a commit to drpaneas/OpenDiablo2 that referenced this issue Mar 24, 2021
* Replace 'maligned' with 'fieldalignment' [1]
* Remove 'interfacer' [2]
* Enable linting against go test files as well
* Disable 'funlen' linter for go table-driven tests

[1] golangci/golangci-lint#1765
[2] golangci/golangci-lint#541
@KnBrBz
Copy link

KnBrBz commented Oct 7, 2021

Understood... however, removing a linter that provides a valuable service (like maligned) when the replacement doesn't actually work correctly, is not good. It doesn't seem to understand the correct sizes of struct fields and issues warnings that are incorrect. Example -- for this struct:

type foo struct {
    a string
    b []string
    c string
}

govet will report "fieldalignment: struct with 48 pointer bytes could be 40 (govet)"

However, using unsafe.Sizeof() shows the struct is actually 56 bytes, with 16 bytes consumed for each string and 24 for the slice. With 8 byte alignment on my platform, there is nothing that can be improved. maligned works fine for this same case.

Here fieldalignment is a note:

Note that there are two different diagnostics reported. One checks struct size,
and the other reports "pointer bytes" used. Pointer bytes is how many bytes of the
object that the garbage collector has to potentially scan for pointers, for example:

struct { uint32; string }

have 16 pointer bytes because the garbage collector has to scan up through the string's
inner pointer.

struct { string; *uint32 }

has 24 pointer bytes because it has to scan further through the *uint32.

struct { string; uint32 }

has 8 because it can stop immediately after the string pointer.

blkperl added a commit to blkperl/argo-events that referenced this issue Feb 1, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
whynowy pushed a commit to argoproj/argo-events that referenced this issue Feb 1, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
juliev0 pushed a commit to juliev0/argo-events that referenced this issue Feb 3, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
juliev0 pushed a commit to juliev0/argo-events that referenced this issue Feb 3, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
usamaB pushed a commit to usamaB/argo-events that referenced this issue Feb 7, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
Signed-off-by: Usama Kaleem <[email protected]>
aweis89 pushed a commit to aweis89/argo-events that referenced this issue Feb 15, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
Signed-off-by: Aaron Weisberg <[email protected]>
BulkBeing pushed a commit to BulkBeing/argo-events that referenced this issue Mar 7, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
juliev0 pushed a commit to juliev0/argo-events that referenced this issue Mar 29, 2022
This commit updates golangci-lint and removes the interfacer check because it
was marked as deprecated and will be removed. See
golangci/golangci-lint#541 for more information.

Signed-off-by: William Van Hevelingen <[email protected]>
@stale
Copy link

stale bot commented Nov 13, 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 Nov 13, 2022
@ldez ldez removed the stale No recent correspondence or work activity label Nov 13, 2022
@ldez
Copy link
Member

ldez commented Mar 3, 2024

Related to the deprecation process #1987

@ldez ldez closed this as completed Mar 3, 2024
@ldez ldez removed this from the v2.0 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cleanup Related to code, process, or doc cleanup
Projects
None yet
Development

No branches or pull requests

10 participants