You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(#94): treat len checks through a local variable as nil checks - #528
Fixes#94. NilAway treats if len(s) == 0 { return } as a nil check on s, but not the same check written through a local variable:
slLen:=len(sl)
ifslLen==0 {
returnnil
}
returnsl[0] // false positive: sl may be nil
This adds a LenLocalVar rich check effect. It is created when a single local variable is assigned exactly len(s) or len(s) - c for a non-negative integer constant c (n := len(s), var n = len(s), i := len(s) - 1, also spelled len(s) + -1). Positive offsets such as len(s) + 1 are not tracked, because len(s) + 1 > 0 holds even when s is nil and the existing AddNilCheck matchers would still treat it as a nil check. Other expressions containing len(s) (e.g. max(len(s), 1)) are not tracked either.
Both the variable and s must be unaliased locals: declared in the function (parameters included), not referenced from any function literal in it, and never address-taken (&v, or a pointer receiver method call on v). Package-level variables, struct fields, map entries and dereferenced pointers are not tracked, since a call between the assignment and the check can change them without an assignment NilAway can see. When a later conditional compares that variable, the variable is replaced with its length expression and the existing AddNilCheck logic runs on the result. So n == 0, n > 0, 0 != n, !(n == 0) and i >= 0 (with i := len(s) - 1) behave the same as their direct len(s) forms.
The effect is invalidated when the variable or s is reassigned, or when the variable is incremented or decremented. Compound assignments like n += len(s) do not create the effect. The other rich check effects are protected by a guard nonce, but this one is not. So propagateRichChecks drops it at a merge unless every incoming path carries it or an equal effect (same variable, same slice, same offset), which is what a branch-local n = len(s) produces.
The effect's true/false branches depend on the triggering conditional, unlike the other rich check effects. So blocksAndPreprocessingFromCFG asks it for the checks through nilChecksFor(cond) and then uses the same merge logic as the other effects.
Testing
Added lengthCheckViaLocalVarTest to testdata/src/go.uber.org/slices/slices.go, including the example from the issue.
Positive cases, including len(a) - 1, len(a) + 0, len(a) + -1, and a variable reassigned to the same length expression on one or both branches. Without the fix they report unexpected diagnostics.
Negative cases:
reassignment of the variable, n++, and reassignment of the slice
a variable reassigned on only one path, or to a different slice's length or a different offset on one or both branches
len(a) + 1 and len(a) - -1 with n > 0, n >= 1, n == 1 and n >= 0; n >= 0 on its own; and the n == 0 branch
max(len(a), 1) and f(len(a))
field and global targets reset by a call
a package-level slice, a struct field, or a map entry cleared by a call; a slice or length variable captured by a closure or passed by address; a pointer receiver method call on a named slice type
go test ./..., make lint, make integration-test and make golden-test pass locally. The golden test reports 2360 errors on main and 2299 on this branch.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Improved slice safety analysis when a length check uses a local variable holding the slice’s length.
Recognizes safe indexing when the length value still matches the slice and the condition establishes a positive length.
Avoids treating checks as safe when the length or slice may have changed, or when the condition does not guarantee a positive length.
Tests
Added coverage for length offsets, changed values, and conditions that do not establish safe indexing, including cases involving globals and parameters.
Walkthrough
The change adds LenLocalVar effects for supported local assignments from len(s) and len(s) ± c. It checks variable eligibility and invalidates tracked effects when relevant values change. Conditional handling derives branch nil checks from the saved length expression, and control-flow merging recognizes equivalent effects. Tests cover supported checks and cases where the length fact may no longer apply.
The new local length tracking can treat a pointer-to-array as proven non-nil after a length check, which hides a nil dereference diagnostic in a narrow case. A pre-existing direct check has the same gap. The change is mergeable once the tracked argument is limited to slices, or if the narrow gap is accepted.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: len checks made through a local variable are now treated as nil checks, matching the LenLocalVar rich check effect added in the changeset. It uses the fix(#94) prefix,…
Description check
The description states the motivation (issue #94), explains the new LenLocalVar rich check effect, lists tracking limits and invalidation rules, and describes the tests added to testdata/src/go.uber.o…
Linked Issues check
Issue [#94] requires NilAway to stop reporting a false positive on sl[0] when slLen := len(sl) is followed by if slLen == 0 { return }. The PR adds a LenLocalVar rich check effect in `rich_che…
Out of Scope Changes check
All changes relate to issue [#94]. The production changes in rich_check_effect.go and backprop_util.go implement local-variable length tracking and its merge and invalidation rules. The test addit…
Full details: Title check
Explanation
The title names the main change: len checks made through a local variable are now treated as nil checks, matching the LenLocalVar rich check effect added in the changeset. It uses the fix(#94) prefix, which references the linked issue. It is specific and concise.
Full details: Description check
Explanation
The description states the motivation (issue #94), explains the new LenLocalVar rich check effect, lists tracking limits and invalidation rules, and describes the tests added to testdata/src/go.uber.org/slices/slices.go. It matches the changeset closely.
Full details: Linked Issues check
Explanation
Issue [#94] requires NilAway to stop reporting a false positive on sl[0] when slLen := len(sl) is followed by if slLen == 0 { return }. The PR adds a LenLocalVar rich check effect in rich_check_effect.go. The effect is created when a single unaliased local is assigned len(s) or len(s) - c (non-negative constant c). When a later conditional compares that variable, the variable is replaced by its length expression and the existing AddNilCheck logic runs. blocksAndPreprocessingFromCFG in backprop_util.go derives the branch checks from the condition through nilChecksFor(cond). Invalidation on reassignment, increment/decrement, and merge handling are described and tested. The test file testdata/src/go.uber.org/slices/slices.go adds lengthCheckViaLocalVarTest, which includes the issue pattern as a positive case and negative cases for reassignment, aliasing, and unsupported expressions. The author reports that the golden test error count drops from 2360 to 2299 and that go test ./... passes. This assessment relies on the PR description and the change summary. The source diff was not re-read for this pass. No unresolved requirement is found.
Full details: Out of Scope Changes check
Explanation
All changes relate to issue [#94]. The production changes in rich_check_effect.go and backprop_util.go implement local-variable length tracking and its merge and invalidation rules. The test additions in testdata/src/go.uber.org/slices/slices.go cover supported forms, rejected forms, and the safety boundaries that the feature needs (globals, fields, map entries, closures, address-taken values, pointer receivers). The change summaries list no unrelated edits, and the ignored file list is empty. The source diff was not re-read for this pass, so the conclusion relies on the summaries and PR description.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Hi @rishiraj38, thanks for taking this issue. I'll ask you to please address the AI comments before a full human review. If you think this issues don't apply, please state why and resolve them.
Both CodeRabbit comments were valid. The latest commit fixes them and adds regression tests that fail without the fix. Golden results on stdlib are unchanged.
Hi @rishiraj38, sorry for the long turn around time. I'm currently on vacation, so expect slow responses :)
The problem with this implementation is that it assumes that the length of the slice can only be changed in the same scope, which is not necessarily true. The following code would result in a false negative:
package main
varg []int= []int{1, 2, 3, 4, 5}
funcmain() {
n:=len(g)
clearG()
ifn>0 {
_=g[0]
}
}
funcclearG() {
// Needs to be a separate function, otherwise nil-assingment catches thisg=nil
}
> nilaway .> go run .
panic: runtime error: index out of range [0] with length 0
goroutine 1 [running]:
main.main()
nilaway-sandbox/main.go:10 +0x68
exit status 2
Since the full work to track reassignments and invalidate the length seems substantial I would suggest we simply drop non-local variables from the analysis from now. Otherwise we risk introducing false negatives by over-indexing global variables.
No rush, enjoy the vacation. The latest commit does that: len is only tracked when its argument is a local variable, so globals, fields, map entries and dereferenced pointers are out. Locals captured by a closure or passed by address are dropped too, since a call can change them the same way. Your example is in the tests and now reports. The earlier CI failures were runner shutdowns, not test failures.
⚠️6 benchmarks with a significant change ran on different hardware
Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.
❌ NilAway errors reported on stdlib are different 📉.
2394 errors on base branch (main, ccd0056) 2333 errors on test branch (c84d34c)
Diffs
+ /opt/hostedtoolcache/go/1.26.10/x64/src/crypto/internal/fips140/sha512/sha512.go:247:7: Potential nil panic detected. Observed nil flow from source to dereference point: + - ed25519/ed25519_test.go:189:32: result 0 of `DecodeString()` lacking guarding; passed as arg `message` to `Sign()` via the assignment(s):+ - `hex.DecodeString(...)` to `message` at ed25519/ed25519_test.go:184:2+ - ed25519/ed25519.go:118:29: function parameter `message` passed as arg `message` to `SignCtx()`+ - ed25519/ed25519.go:202:34: function parameter `message` passed as arg `message` to `signCtx()`+ - ed25519/ed25519.go:213:38: function parameter `message` passed as arg `message` to `signWithDom()`+ - ed25519/ed25519.go:242:11: function parameter `message` passed as arg `p` to `Write()`+ - sha512/sha512.go:247:7: function parameter `p` sliced into+ /opt/hostedtoolcache/go/1.26.10/x64/src/crypto/internal/fips140/sha512/sha512.go:247:7: Potential nil panic detected. Observed nil flow from source to dereference point: + - ed25519/ed25519_test.go:196:38: result 0 of `DecodeString()` lacking guarding; passed as arg `message` to `VerifyWithOptions()` via the assignment(s):+ - `hex.DecodeString(...)` to `message` at ed25519/ed25519_test.go:184:2+ - ed25519/ed25519.go:251:31: function parameter `message` passed as arg `message` to `VerifyCtx()`+ - ed25519/ed25519.go:287:28: function parameter `message` passed as arg `message` to `verifyWithDom()`+ - ed25519/ed25519.go:307:11: function parameter `message` passed as arg `p` to `Write()`+ - sha512/sha512.go:247:7: function parameter `p` sliced into+ /opt/hostedtoolcache/go/1.26.10/x64/src/crypto/internal/fips140/sha512/sha512.go:247:7: Potential nil panic detected. Observed nil flow from source to dereference point: + - ed25519/ed25519_test.go:203:31: result 0 of `DecodeString()` lacking guarding; passed as arg `message` to `VerifyWithOptions()` via the assignment(s):+ - `hex.DecodeString(...)` to `message` at ed25519/ed25519_test.go:184:2+ - ed25519/ed25519.go:251:31: function parameter `message` passed as arg `message` to `VerifyCtx()`+ - ed25519/ed25519.go:287:28: function parameter `message` passed as arg `message` to `verifyWithDom()`+ - ed25519/ed25519.go:307:11: function parameter `message` passed as arg `p` to `Write()`+ - sha512/sha512.go:247:7: function parameter `p` sliced into+ /opt/hostedtoolcache/go/1.26.10/x64/src/crypto/internal/fips140/sha512/sha512.go:247:7: Potential nil panic detected. Observed nil flow from source to dereference point: + - ed25519/ed25519_test.go:208:31: result 0 of `DecodeString()` lacking guarding; passed as arg `message` to `VerifyWithOptions()` via the assignment(s):+ - `hex.DecodeString(...)` to `message` at ed25519/ed25519_test.go:184:2+ - ed25519/ed25519.go:251:31: function parameter `message` passed as arg `message` to `VerifyCtx()`+ - ed25519/ed25519.go:287:28: function parameter `message` passed as arg `message` to `verifyWithDom()`+ - ed25519/ed25519.go:307:11: function parameter `message` passed as arg `p` to `Write()`+ - sha512/sha512.go:247:7: function parameter `p` sliced into+ /opt/hostedtoolcache/go/1.26.10/x64/src/crypto/internal/fips140/sha512/sha512.go:247:7: Potential nil panic detected. Observed nil flow from source to dereference point: + - ed25519/ed25519_test.go:213:31: result 0 of `DecodeString()` lacking guarding; passed as arg `message` to `VerifyWithOptions()` via the assignment(s):+ - `hex.DecodeString(...)` to `message` at ed25519/ed25519_test.go:184:2+ - ed25519/ed25519.go:251:31: function parameter `message` passed as arg `message` to `VerifyCtx()`+ - ed25519/ed25519.go:287:28: function parameter `message` passed as arg `message` to `verifyWithDom()`+ - ed25519/ed25519.go:307:11: function parameter `message` passed as arg `p` to `Write()`+ - sha512/sha512.go:247:7: function parameter `p` sliced into+ /opt/hostedtoolcache/go/1.26.10/x64/src/crypto/internal/fips140/sha512/sha512.go:247:7: Potential nil panic detected. Observed nil flow from source to dereference point: + - ed25519/ed25519_test.go:323:22: result 0 of `DecodeString()` lacking guarding; passed as arg `message` to `Verify()` via the assignment(s):+ - `hex.DecodeString(...)` to `msg` at ed25519/ed25519_test.go:304:3+ - ed25519/ed25519.go:222:38: function parameter `message` passed as arg `message` to `VerifyWithOptions()`+ - ed25519/ed25519.go:251:31: function parameter `message` passed as arg `message` to `VerifyCtx()`+ - ed25519/ed25519.go:287:28: function parameter `message` passed as arg `message` to `verifyWithDom()`+ - ed25519/ed25519.go:307:11: function parameter `message` passed as arg `p` to `Write()`+ - sha512/sha512.go:247:7: function parameter `p` sliced into+ /opt/hostedtoolcache/go/1.26.10/x64/src/go/types/assignments.go:455:5: Potential nil panic detected. Observed nil flow from source to dereference point: + - types/assign
...(truncated)...
osition 0
- - big/nat.go:82:10: result 0 of `setWord()` returned from `setUint64()` in position 0- - big/nat.go:256:44: result 0 of `setUint64()` passed as arg `y` to `mul()`- - big/natmul.go:26:24: function parameter `y` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/math/big/natmul.go:286:22: Potential nil panic detected. Observed nil flow from source to dereference point: - - big/nat.go:72:10: nilable value returned from `setWord()` in position 0- - big/nat.go:859:22: result 0 of `setWord()` passed as arg `x` to `sqr()` via the assignment(s):- - `z.setWord(1)` to `z` at big/nat.go:847:2- - big/natmul.go:105:23: function parameter `x` passed as arg `x` to `karatsubaSqr()`- - big/natmul.go:286:22: function parameter `x` sliced into-- (Same nil source could also cause potential nil panic(s) at 1 other place(s): "big/natmul.go:286:48".)- /opt/hostedtoolcache/go/1.26.10/x64/src/math/big/natmul.go:50:26: Potential nil panic detected. Observed nil flow from source to dereference point: - - big/nat.go:72:10: nilable value returned from `setWord()` in position 0- - big/nat.go:82:10: result 0 of `setWord()` returned from `setUint64()` in position 0- - big/nat.go:256:21: result 0 of `setUint64()` passed as arg `x` to `mul()`- - big/natmul.go:50:26: function parameter `x` sliced into-- (Same nil source could also cause potential nil panic(s) at 1 other place(s): "big/natmul.go:57:19".)- /opt/hostedtoolcache/go/1.26.10/x64/src/math/big/natmul.go:79:8: Potential nil panic detected. Observed nil flow from source to dereference point: - - big/nat.go:72:10: nilable value returned from `setWord()` in position 0- - big/nat.go:859:22: result 0 of `setWord()` passed as arg `x` to `sqr()` via the assignment(s):- - `z.setWord(1)` to `z` at big/nat.go:847:2- - big/natmul.go:79:8: function parameter `x` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/net/ip.go:285:12: Potential nil panic detected. Observed nil flow from source to dereference point: - - net/ip.go:531:9: literal `nil` returned from `ParseIP()` in position 0- - net/example_test.go:301:14: result 0 of `ParseIP()` used as receiver to call `Mask()` via the assignment(s):- - `net.ParseIP(...)` to `ipv4Addr` at net/example_test.go:298:2- - net/ip.go:285:12: read by method receiver `ip` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/net/ip.go:285:12: Potential nil panic detected. Observed nil flow from source to dereference point: - - net/ip.go:531:9: literal `nil` returned from `ParseIP()` in position 0- - net/example_test.go:306:14: result 0 of `ParseIP()` used as receiver to call `Mask()` via the assignment(s):- - `net.ParseIP(...)` to `ipv6Addr` at net/example_test.go:303:2- - net/ip.go:285:12: read by method receiver `ip` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/net/ip.go:490:20: Potential nil panic detected. Observed nil flow from source to dereference point: - - httpproxy/proxy.go:200:26: unassigned variable `ip` passed as arg `ip` to `match()`- - httpproxy/proxy.go:342:20: passed as parameter `ip` to `cidrMatch.match()` (implementing `matcher.match()`)- - httpproxy/proxy.go:343:25: function parameter `ip` passed as arg `ip` to `Contains()`- - net/ip.go:490:20: function parameter `ip` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/unicode/utf8/utf8.go:183:15: Potential nil panic detected. Observed nil flow from source to dereference point: - - build/build.go:1357:13: literal `nil` returned from `skipSpaceOrComment()` in position 0- - build/build.go:1386:30: result 0 of `skipSpaceOrComment()` passed as arg `p` to `DecodeRune()` via the assignment(s):- - `skipSpaceOrComment(...)` to `data` at build/build.go:1381:2,- - `data` to `rest` at build/build.go:1384:2- - utf8/utf8.go:167:27: function parameter `p` passed as arg `p` to `decodeRuneSlow()`- - utf8/utf8.go:183:15: function parameter `p` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/unicode/utf8/utf8.go:183:15: Potential nil panic detected. Observed nil flow from source to dereference point: - - csv/reader.go:307:33: result 0 of `readLine()` lacking guarding; passed as arg `b` to `nextRune()` via the assignment(s):- - `r.readLine()` to `line` at csv/reader.go:306:3- - csv/reader.go:293:26: function parameter `b` passed as arg `p` to `DecodeRune()`- - utf8/utf8.go:167:27: function parameter `p` passed as arg `p` to `decodeRuneSlow()`- - utf8/utf8.go:183:15: function parameter `p` sliced into- /opt/hostedtoolcache/go/1.26.10/x64/src/vendor/golang.org/x/crypto/cryptobyte/asn1.go:383:34: Potential nil panic detected. Observed nil flow from source to dereference point: - - cryptobyte/asn1.go:375:89: unassigned variable `bytes` passed as arg `n` to `asn1Unsigned()`- - cryptobyte/asn1.go:383:34: function parameter `n` sliced into-- (Same nil source could also cause potential nil panic(s) at 2 other place(s): "cryptobyte/asn1.go:387:5", and "cryptobyte/asn1.go:393:18".)
❌ Patch coverage is 92.53731% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.91%. Comparing base (ccd0056) to head (ca2e70a).
The reason will be displayed to describe this comment to others. Learn more.
I was checking where the decrease in benchmark performance is coming from, and by calling isUnaliasedLocalVar on all paths we're ending with quadratic performance since this function eventually ends up performing an expensive body walk. This is causing any assignment (think foo = bar +1) to perform the full walk.
We should instead instead reorder the conditions as such:
Current:
if!ok||!isUnaliasedLocalVar(rootNode, ident) { // full body walk on all assignmentsreturnnil
}
lenArg, offset:=lenOffsetArg(rootNode.Pass(), rhs[0])
iflenArg==nil { // cheap len args check laterreturnnil
}
Suggested:
if!ok {
returnnil
}
lenArg, offset:=lenOffsetArg(rootNode.Pass(), rhs[0])
iflenArg==nil// cheap check first. lenArg == nil is true on almost all cases and discards the need for the walk||!isUnaliasedLocalVar(rootNode, ident) { // now perform the expensive checkreturnnil
}
The reason will be displayed to describe this comment to others. Learn more.
Done in the latest commit. The len match now runs first, and isUnaliasedLocalVar only runs on assignments that are n := len(s) or len(s) ± c. On my machine the benchmarks are back in line with main (go/types 304ms before, 272ms after, 269ms on main; the other packages are within noise).
I also rebased on main and ran the golden test locally. The new entries in the bot's diff are not new nil sources. Each one either keeps the same source and dereference with a different path in the trace, or was already listed under "Same nil source could also cause..." for an error this PR removes, so it now gets reported on its own.
A conditional on a local variable that holds a length expression, such
as `n := len(s); if n == 0 { ... }` or `i := len(s) - 1; if i >= 0`,
now guards `s` the same way the equivalent direct `len(s)` comparison
does. The variable is tracked with a new LenLocalVar rich check effect,
which is invalidated when the variable or `s` is reassigned (or the
variable is incremented/decremented).
… on every path
A LenLocalVar effect is now only created for a local variable assigned
exactly `len(s)` or `len(s) ± c` with an integer constant `c`. Nested
forms such as `max(len(s), 1)` and fields or globals, which a call can
modify, are no longer tracked.
Since a LenLocalVar is not protected by a guard nonce, it is now also
dropped at a merge unless every incoming path carries it, so a variable
assigned a different length on one branch is no longer used as a nil
check on either slice.
…cts by value
A LenLocalVar for `n := len(s) + 1` turned `n > 0` into `len(s) + 1 > 0`,
which AddNilCheck treats as a nil check on `s` even though it holds when
`s` is nil. Only offsets that keep the expression at or below `len(s)`
are tracked now.
propagateRichChecks masked LenLocalVar effects by identity, so a
branch-local `n = len(s)` that equals the effect on the other path
dropped both at the merge. Equal effects now cover each other.
The argument of `len` was not restricted to local variables, so a
package-level slice cleared by a call between `n := len(g)` and `n > 0`
was still treated as non-nil. Require both the variable holding the
length and the `len` argument to be variables declared in the function
whose value can only change through its own statements: not globals,
fields, index or deref expressions, variables captured by a function
literal, or variables whose address is taken (explicitly or via a
pointer receiver method call).
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
Limit the tracked length argument to slice types.
NodeTriggersLenLocalVar accepts a local variable of any type as the len argument. A local pointer-to-array parameter such as p *[1]int passes this check. n := len(p) followed by if n > 0 { _ = *p } then marks p non-nil, which suppresses the diagnostic for *p. Calling f(nil) still panics, because len(p) on a pointer to an array does not evaluate p. The PR scope covers slice values only, so this path should reject other types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Motivation
Fixes #94. NilAway treats
if len(s) == 0 { return }as a nil check ons, but not the same check written through a local variable:This adds a
LenLocalVarrich check effect. It is created when a single local variable is assigned exactlylen(s)orlen(s) - cfor a non-negative integer constantc(n := len(s),var n = len(s),i := len(s) - 1, also spelledlen(s) + -1). Positive offsets such aslen(s) + 1are not tracked, becauselen(s) + 1 > 0holds even whensis nil and the existingAddNilCheckmatchers would still treat it as a nil check. Other expressions containinglen(s)(e.g.max(len(s), 1)) are not tracked either.Both the variable and
smust be unaliased locals: declared in the function (parameters included), not referenced from any function literal in it, and never address-taken (&v, or a pointer receiver method call onv). Package-level variables, struct fields, map entries and dereferenced pointers are not tracked, since a call between the assignment and the check can change them without an assignment NilAway can see. When a later conditional compares that variable, the variable is replaced with its length expression and the existingAddNilChecklogic runs on the result. Son == 0,n > 0,0 != n,!(n == 0)andi >= 0(withi := len(s) - 1) behave the same as their directlen(s)forms.The effect is invalidated when the variable or
sis reassigned, or when the variable is incremented or decremented. Compound assignments liken += len(s)do not create the effect. The other rich check effects are protected by a guard nonce, but this one is not. SopropagateRichChecksdrops it at a merge unless every incoming path carries it or an equal effect (same variable, same slice, same offset), which is what a branch-localn = len(s)produces.The effect's true/false branches depend on the triggering conditional, unlike the other rich check effects. So
blocksAndPreprocessingFromCFGasks it for the checks throughnilChecksFor(cond)and then uses the same merge logic as the other effects.Testing
lengthCheckViaLocalVarTesttotestdata/src/go.uber.org/slices/slices.go, including the example from the issue.len(a) - 1,len(a) + 0,len(a) + -1, and a variable reassigned to the same length expression on one or both branches. Without the fix they report unexpected diagnostics.n++, and reassignment of the slicelen(a) + 1andlen(a) - -1withn > 0,n >= 1,n == 1andn >= 0;n >= 0on its own; and then == 0branchmax(len(a), 1)andf(len(a))go test ./...,make lint,make integration-testandmake golden-testpass locally. The golden test reports 2360 errors on main and 2299 on this branch.