lint: add a linter to avoid using `==nil` to assert empty slice or map
It's such a huge change!!! I'm not sure whether merging this PR is good. Need discussion.
They are fixed automatically by the linter.
What problem does this PR solve?
Issue Number: close #52074, ref #51807
Problem Summary:
It's easy to make mistake to use arr == nil to tell whether a slice is empty, but if the slice is []T{}, this condition will not meet. #51807 is one of the bugs made by it. Therefore I tried to add a linter to avoid this situation.
This PR also fixed similar issues for map.
What changed and how does it work?
- Don't allow using
arr == nil. - If you really want to use
arr == nilto check whether is not initialized (or empty return), you can useemptyslice.IsNil(arr), which is ignored by the linter.
It gives a lot of error, and I didn't fix them all yet. I'm not sure whether it's good to add this linter.
Check List
Tests
- [ ] Unit test
- [x] Integration test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test
- [ ] I checked and no code files have been changed.
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
None
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign bb7133, d3hunter, easonn7, gmhdbjd, wjhuang2016, yujuncen, zanmato1984 for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
- OWNERS
- br/OWNERS
- br/pkg/lightning/OWNERS
- dumpling/OWNERS
- pkg/autoid_service/OWNERS
- pkg/ddl/OWNERS
- pkg/distsql/OWNERS
- pkg/domain/OWNERS
- pkg/executor/importer/OWNERS
- pkg/expression/OWNERS
- pkg/infoschema/OWNERS
- pkg/meta/OWNERS
- pkg/parser/OWNERS
- pkg/sessionctx/variable/OWNERS
- pkg/table/OWNERS
- pkg/tablecodec/OWNERS
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Codecov Report
:x: Patch coverage is 79.91968% with 100 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 54.7550%. Comparing base (3475795) to head (8619e3e).
:warning: Report is 3608 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #52073 +/- ##
=================================================
- Coverage 70.7242% 54.7550% -15.9692%
=================================================
Files 1486 1598 +112
Lines 439452 612754 +173302
=================================================
+ Hits 310799 335514 +24715
- Misses 109205 253918 +144713
- Partials 19448 23322 +3874
| Flag | Coverage Δ | |
|---|---|---|
| integration | 49.1448% <55.9006%> (?) |
|
| unit | 71.6143% <75.7085%> (-0.1098%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Components | Coverage Δ | |
|---|---|---|
| dumpling | ∅ <87.5000%> (∅) |
|
| parser | ∅ <ø> (∅) |
|
| br | 58.2345% <63.6363%> (+7.1946%) |
:arrow_up: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
LGTM, but...
How to trigger this lint manually? I found a lot of checks related to lint like
make lint,make static-check,make bazel_lint... :(
I think make bazel_lint can trigger this lint.
This PR also provides a main package in /build/linter/emptynil/cmd. Using -fix argument will fix the found issues automatically.
i think it's a little over-reacting to the bug in description. misusing between len(xx) == 0 and xx == nil are human errors, that cannot be avoided by replacing xx == nil with isNil(xx), we still might make the same mistake by misusing len(xx) == 0 and isNil(xx), and it sacrifices readability.
PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@YangKeao: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-br-integration-test | 8619e3ea7c2f7be75c84bbefe2fa04db60c6d26a | link | true | /test pull-br-integration-test |
| pull-lightning-integration-test | 8619e3ea7c2f7be75c84bbefe2fa04db60c6d26a | link | true | /test pull-lightning-integration-test |
| pull-unit-test-ddlv1 | 8619e3ea7c2f7be75c84bbefe2fa04db60c6d26a | link | true | /test pull-unit-test-ddlv1 |
| pull-integration-e2e-test | 8619e3ea7c2f7be75c84bbefe2fa04db60c6d26a | link | true | /test pull-integration-e2e-test |
| pull-integration-realcluster-test-next-gen | 8619e3ea7c2f7be75c84bbefe2fa04db60c6d26a | link | true | /test pull-integration-realcluster-test-next-gen |
| pull-unit-test-next-gen | 8619e3ea7c2f7be75c84bbefe2fa04db60c6d26a | link | true | /test pull-unit-test-next-gen |
Full PR test history. Your PR dashboard.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.