Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

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

Open
ldez wants to merge 2 commits into golangci:master
base: master
Choose a base branch
Loading
from ldez:fix/cache-imports

Conversation

@ldez
Copy link
Member

@ldez ldez commented Feb 27, 2024 •
edited
Loading

To reproduce the problem:

git clone git@github.com: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

gabolaev reacted with thumbs up emoji
@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
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 reacted with heart emoji
Copy link
Member Author

ldez commented Feb 28, 2024 •
edited
Loading

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

Reviewers

@alexandear alexandear alexandear approved these changes

@bombsimon bombsimon bombsimon approved these changes

+1 more reviewer

@gabolaev gabolaev gabolaev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: cache blocked Need's direct action from maintainer bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

govet: printf unstable behavior with imported types

AltStyle によって変換されたページ (->オリジナル) /