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 the int conversion warning in linter #3968

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Fix the int conversion warning in linter #3968

merged 3 commits into from
Aug 26, 2024

Conversation

KyleJu
Copy link
Collaborator

@KyleJu KyleJu commented Aug 26, 2024

Fix the int conversion warning in Linter, suggested in securego/gosec#1185 (comment). Nolint the intended use of int64 to int conversion

@KyleJu KyleJu changed the title Fix the int conversion issue in linter Fix the int conversion warning in linter Aug 26, 2024
@KyleJu KyleJu requested a review from past August 26, 2024 19:46
@past
Copy link
Member

past commented Aug 26, 2024

According to this comment the latest version of golangci-lint should not have many false positives. Can you try first to update ours and see if the errors are gone?

@KyleJu
Copy link
Collaborator Author

KyleJu commented Aug 26, 2024

According to this comment the latest version of golangci-lint should not have many false positives. Can you try first to update ours and see if the errors are gone?

The false positive case mentioned is an "uint8 -> int" overflow false positive, or "byte to int". In our cases, we are casting int 64 to int 32 intentionally, which doesn't fall into the false positive.

After the upgrade, the linter failed again. Regardless I will update the latest golangci-lint version. Thanks for the comment!

Copy link
Member

@past past left a comment

Choose a reason for hiding this comment

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

Ideally we would stop doing the error-prone type casting and use int (or uint64) consistently, but we don't need to block the linter fix on this change.

@KyleJu KyleJu merged commit 15e7481 into main Aug 26, 2024
11 of 12 checks passed
@KyleJu KyleJu deleted the fix-int-conv branch August 26, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants