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

[chore] Add intrange linter #38009

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rockdaboot
Copy link
Contributor

The intrange linter checks that for loops use the Go 1.22+ range loop syntax if possible.

@rockdaboot
Copy link
Contributor Author

remarks / observations

The --fix option for golangci-lint intrange seems to be overly enthusiastic about these kind of conversions and ignores possible side-effects. Example:

for i := 0; i < foo.Len(); i++

and

for i := range foo.Len()

do different things; the range loop calls foo.Len() only once, the traditional loop calls it with every loop iteration. This lead to failing tests in the transformprocessor.

Even simpler, a range loop has different semantics if the ranged-over integer variable is changed inside the loop body.

The --fix option unnecessarily creates new unconvert issues that needs to be fixed manually. Example

prometheusreceiver/internal/metricfamily.go:241:19: unnecessary conversion (unconvert)
                        for range int32(span.Offset) {

where for i := int32(0); i < span.Offset; i++ { has been translated.
This required some several manual changes.

Other issues that requires manual changes

tailsamplingprocessor/and_helper.go:1: : # github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor [github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor.test]
./processor_benchmarks_test.go:45:6: declared and not used: i (typecheck)

Conclusion

Blindly converting for loops into range loops with the intrange linter may cause havoc.
Opened upstream:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant