CI: fix some more linter advice for old code#6317
CI: fix some more linter advice for old code#6317algorandskiy merged 4 commits intoalgorand:masterfrom
Conversation
TARGET_VAR="err"
NEW_VAR="err1"
if [ "$#" -ge 1 ]; then
TARGET_VAR="$1"
fi
if [ "$#" -ge 2 ]; then
NEW_VAR="$2"
fi
echo "Will rename shadowed '$TARGET_VAR' variables to '$NEW_VAR'"
extract_location() {
local error_line="$1"
echo "$error_line" | grep -o "[^:]*:[0-9]*:[0-9]*" | head -1
}
echo "Running 'make lint' to find shadowed '$TARGET_VAR' variables..."
lint_output=$(make lint | grep "\"$TARGET_VAR\" shadows")
issue_count=$(echo "$lint_output" | grep -c .)
echo "Found $issue_count potential '$TARGET_VAR' shadowing issues"
echo "$lint_output" | while read -r line; do
echo "----------------------------------------"
echo "Processing: $line"
if [[ $line == *"\"$TARGET_VAR\" shadows"* ]]; then
# Extract file and position information
location=$(extract_location "$line")
if [ -z "$location" ]; then
echo "Failed to extract location from: $line"
continue
fi
echo "About to execute: gopls rename -w '$location' '$NEW_VAR'"
gopls rename -w "$location" "$NEW_VAR"
rename_result=$?
if [ $rename_result -eq 0 ]; then
echo "Successfully renamed variable '$TARGET_VAR' to '$NEW_VAR' at $location"
else
echo "Failed to rename variable with exit code $rename_result."
fi
fi
done
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6317 +/- ##
==========================================
- Coverage 51.65% 51.64% -0.01%
==========================================
Files 649 649
Lines 86964 86964
==========================================
- Hits 44918 44914 -4
- Misses 39192 39193 +1
- Partials 2854 2857 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
Seems good. What's weird is that we're just doing what the compiler would have done anyway. So, if this is worth a linter complaining about, why is it ok to paper it over with a script? Presumably, the linter is worried that when you shadowed err you didn't really mean to, and you instead meant to assign to the top-level err and have some code, lower down, return it. I suppose we don't need to worry that we're papering over that mistake, because after the transformation, we would instead get a linter error that err1 was ignored.
Is that all right? I had to type it out to convince myself.
Yes, it forces you to consider whether you wanted to shadow or not — we originally introduced this linter because a unintentional use of a shadowed variable led to a real bug, so we turned it on for new code. Retroactively going back and updating the codebase just makes it uniform, so the same standard is followed everywhere. |
3 participants
Summary
Following on #6315 this fixes all issues raised by the nilerr linter, and some of the govet issues, for older parts of the codebase. For govet it focuses just on error-shadowing warnings of the form
shadow: declaration of "err" shadows declaration at line Xby using a script (source in comment on 9e92a30) that parses each error and callsgopls renameto renameerrtoerr1,err1toerr2, etc.Test Plan
Existing tests should pass.