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

fix: add local imported packages to the packages to analyze when using cache #4424

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ldez
Copy link
Member

@ldez ldez commented Feb 27, 2024

To reproduce the problem:

git clone [email protected]:gabolaev/gl-issue-reproduction.git && cd $_
golangci-lint cache clean
git checkout HEAD .; golangci-lint run; sed -i 's/some text/SOME OTHER TEXT/g' somepackage/somefile.go; golangci-lint run
# git checkout HEAD .; ./golangci-lint run; sed -i 's/some text/SOME OTHER TEXT/g' somepackage/somefile.go; ./golangci-lint run

If it works you should see:

somepackage/somefile.go:9:2: printf: (*github.com/sirupsen/logrus.Logger).Warnf format %v reads arg #1, but call has 0 args (govet)
        log.Warnf("some text %v") // this line is supposed to be reported by govet printf linter
        ^
somepackage/somefile.go:9:2: printf: (*github.com/sirupsen/logrus.Logger).Warnf format %v reads arg #1, but call has 0 args (govet)
        log.Warnf("SOME OTHER TEXT %v") // this line is supposed to be reported by govet printf linter
        ^

but if it not works:

somepackage/somefile.go:9:2: printf: (*github.com/sirupsen/logrus.Logger).Warnf format %v reads arg #1, but call has 0 args (govet)
        log.Warnf("some text %v") // this line is supposed to be reported by govet printf linter
        ^

This will impact negatively the performance of a run with a cache but it will increase the stability of reports.

Fixes #4423

@ldez ldez added bug Something isn't working area: cache labels Feb 27, 2024
@ldez ldez force-pushed the fix/cache-imports branch 2 times, most recently from 849f21e to ccf27b4 Compare February 27, 2024 21:29
@ldez ldez requested review from bombsimon and Antonboom February 28, 2024 22:24
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you do any tests (manual or otherwise) on a somewhat larger codebase and/or with a lot of local imports? Are we talking millisecond increases or can this become a cardinality bomb and grow much bigger than that?

The fix looks good and is understandable so I'll approve this since I have full trust in your judgement here 😄

@ldez
Copy link
Member Author

ldez commented Feb 28, 2024

Did you do any tests (manual or otherwise) on a somewhat larger codebase and/or with a lot of local imports?

I didn't do a benchmark because this depends on how many direct packages a package depends on, so it's difficult to create accurate benchmarks.

Are we talking millisecond increases or can this become a cardinality bomb and grow much bigger than that?

As it just adds the direct packages related to one analyzed package and not the indirect packages, the cardinality is limited.

But if a package depends on thousands of local packages (I hope nobody does that), the impact will be important on performances (only for the run after the first one) but I don't have a better solution.

EDIT: I have some doubts maybe the real problem is the linter 🤔

Copy link

@gabolaev gabolaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, thank you!

@ldez ldez removed the request for review from Antonboom March 1, 2024 12:43
@ldez ldez added the blocked Need's direct action from maintainer label Mar 1, 2024
@ldez ldez force-pushed the fix/cache-imports branch from ccf27b4 to 98c486b Compare March 7, 2024 23:03
@alexandear alexandear self-requested a review March 9, 2024 16:02
Copy link
Member

@alexandear alexandear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cache blocked Need's direct action from maintainer bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

govet: printf unstable behavior with imported types
4 participants