Skip to content

Conversation

alingse
Copy link
Contributor

@alingse alingse commented Jul 12, 2024

This is a bug I discovered during work.

append all its data while range over it is generally not the intended behavior and can lead to duplicate data.

for _, n := range ns {
    ...
    rs = append(rs, n)     // OK
    rs = append(rs, ns...) // Bug
    ...
}

I implemented the checker firstly in my repo and use the go-linter-runner github action to scan top 10k Go repos. This found real bugs, documented in alingse/sundrylint#1

Signed-off-by: alingse <alingse@foxmail.com>
@alingse alingse changed the title add rangeAppendAll checker checkers: add rangeAppendAll Jul 12, 2024
@alingse
Copy link
Contributor Author

alingse commented Jul 15, 2024

@cristaloleg would you review it when you have time?

@cristaloleg
Copy link
Member

Hey @alingse I'm not sure this is a good general check. @quasilyte ?

@alingse
Copy link
Contributor Author

alingse commented Jul 22, 2024

截屏2024-07-22 13 25 58

I search the top github repos, and find this bugs, they are all the same wrong pattern

	for _, p := range pluginsForDatamover {
			if !slices.Contains(plugins, p) {
				plugins = append(plugins, pluginsForDatamover...) // Wrong
			}
		}
					for _, v := range childNodes {
						if v.Type() == NWhen {
							n.whenNodes = append(n.whenNodes, childNodes...) // Wrong
						}
		for _, ip := range ips {
			if ip.IsCollinear {
				continue
			}
			ii.Intersections = append(ii.Intersections, ips...) // Wrong
		}

I think it is a rare but general mistake, and I add isFirstArgInitSlice to avoid some case that they really want range-and-append-all pattern like in https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/util/selectors/bimultimap_test.go#L360:9:

		for i, v := range remaining {
			r := append([]int(nil), remaining...) // copy remaining
			r = append(r[:i], r[i+1:]...)         // delete placed index
			p := permute(append(placed, v), r)    // place index and permute
			permutations = append(permutations, p...)
		}

🤔

alingse and others added 4 commits July 22, 2024 18:16
Co-authored-by: Oleksandr Redko <oleksandr.red+github@gmail.com>
Co-authored-by: Oleksandr Redko <oleksandr.red+github@gmail.com>
Signed-off-by: alingse <alingse@foxmail.com>
Signed-off-by: alingse <alingse@foxmail.com>
@alingse
Copy link
Contributor Author

alingse commented Jul 23, 2024

@alexandear I have update the commits.

@alingse alingse requested a review from alexandear August 19, 2024 06:00
@alingse
Copy link
Contributor Author

alingse commented Aug 28, 2024

@alexandear this PR need your review again.

@alingse
Copy link
Contributor Author

alingse commented Sep 6, 2024

@cristaloleg @quasilyte Could you review this PR and merge it if it is ok? 🤔

@cristaloleg
Copy link
Member

@quasilyte #1423 (comment)

@alingse
Copy link
Contributor Author

alingse commented Sep 27, 2024

@quasilyte need your help. would you review this PR ?

@alingse
Copy link
Contributor Author

alingse commented Oct 8, 2024

@cristaloleg could you merge it? and make a new release ?

@cristaloleg cristaloleg merged commit bb8efef into go-critic:master Oct 8, 2024
2 checks passed
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.

4 participants