Skip to content

lint: Check for references to loop variables#5105

Merged
algorandskiy merged 12 commits into
algorand:masterfrom
cce:enable-exportloopref
Feb 8, 2023
Merged

lint: Check for references to loop variables#5105
algorandskiy merged 12 commits into
algorand:masterfrom
cce:enable-exportloopref

Conversation

@cce
Copy link
Copy Markdown
Contributor

@cce cce commented Feb 2, 2023

Summary

After looking at #5084 I recalled there may be a linter for this class of issues, where a pointer to a loop variable is exported outside of the loop iteration, leading to unintended effects because the loop variable is actually re-used for each iteration and maintains the same pointer throughout.

https://medium.com/@betable/3-go-gotchas-590b8c014e0a
https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

I found the golangci-linter called "exportloopref" which catches examples like this one and am enabling it here for regular code and test code too.

I also found a linter called "gosec" that more aggressively checks for references to loop variables and enabled it in the warning linter config, which does not block a build, but does provide advice on PRs.

This also updates check_deps.sh to tell you if you are using the wrong version of msgp, swagger, oapi-codegen, or golangci-lint.

Test Plan

Existing tests should pass. Fixed some test helper code in assembler_test.go where the linter also found a loop pointer issue.

@cce cce added the Enhancement label Feb 2, 2023
@cce cce requested a review from winder February 2, 2023 18:11
Eric-Warehime
Eric-Warehime previously approved these changes Feb 2, 2023
Comment thread scripts/check_deps.sh Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 2, 2023

Codecov Report

Merging #5105 (42674a2) into master (6487b37) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master    #5105   +/-   ##
=======================================
  Coverage   53.42%   53.42%           
=======================================
  Files         431      431           
  Lines       54369    54369           
=======================================
+ Hits        29046    29047    +1     
- Misses      23061    23062    +1     
+ Partials     2262     2260    -2     
Impacted Files Coverage Δ
cmd/goal/clerk.go 8.77% <0.00%> (ø)
daemon/algod/api/server/v2/utils.go 12.92% <0.00%> (ø)
data/pools/transactionPool.go 49.77% <100.00%> (ø)
data/transactions/verify/txn.go 70.46% <100.00%> (ø)
ledger/store/accountsV2.go 15.95% <100.00%> (ø)
ledger/store/schema.go 47.52% <100.00%> (ø)
ledger/tracker.go 75.10% <0.00%> (-2.96%) ⬇️
ledger/blockqueue.go 82.25% <0.00%> (-2.69%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jannotti jannotti self-requested a review February 2, 2023 19:25
jannotti
jannotti previously approved these changes Feb 2, 2023
winder
winder previously approved these changes Feb 2, 2023
Comment thread scripts/check_deps.sh
Comment thread scripts/check_deps.sh
@cce cce dismissed stale reviews from winder and jannotti via 2030c50 February 2, 2023 20:00
@cce cce changed the title lint: Add exportloopref linter lint: Check for references to loop variables Feb 2, 2023
winder
winder previously approved these changes Feb 2, 2023
Comment thread ledger/store/schema.go
jannotti
jannotti previously approved these changes Feb 3, 2023
@cce cce requested a review from winder February 6, 2023 15:25
Eric-Warehime
Eric-Warehime previously approved these changes Feb 7, 2023
@cce cce dismissed stale reviews from Eric-Warehime and jannotti via 42674a2 February 7, 2023 22:08
@cce cce requested a review from Eric-Warehime February 7, 2023 22:11
@algorandskiy algorandskiy merged commit 42a112a into algorand:master Feb 8, 2023
@cce cce deleted the enable-exportloopref branch February 8, 2023 20:49
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.

6 participants