Conversation
…iolations Agent-Logs-Url: https://github.com/github/gh-aw/sessions/acdcfee6-bf25-4163-be26-348e308ab7aa Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…eline and Map trade-off note Agent-Logs-Url: https://github.com/github/gh-aw/sessions/acdcfee6-bf25-4163-be26-348e308ab7aa Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the functional-pragmatist workflow guidance/examples to avoid slice preallocation patterns (make([]T, len(x)), make([]T, 0, len(x))) that are flagged by CodeQL.
Changes:
- Rewrites Map/Filter helper examples to build results via
var+appendinstead of pre-sized/pre-cap slices. - Updates “immutability” and inline-loop examples to remove
make(..., len(...))/make(..., 0, len(...))patterns. - Replaces a defensive-copy snippet (
make+copy) withslices.Clone, and updates the guideline bullet to warn against preallocation patterns.
Comments suppressed due to low confidence (2)
.github/workflows/functional-pragmatist.md:474
- Similar to Map, switching Filter to
var result []Tchanges the behavior when there are no matches (or the input is empty): it can returnnilrather than a non-nil empty slice. If this is meant as a copy/paste helper implementation, it’s usually better to keep the empty-slice semantics stable (or call out this difference explicitly) to avoid surprises in callers/tests/marshalling.
// Filter returns elements that match the predicate
func Filter[T any](slice []T, fn func(T) bool) []T {
var result []T
for _, v := range slice {
if fn(v) {
.github/workflows/functional-pragmatist.md:1392
- Switching the defensive-copy example to
slices.Clone(s.items)subtly changes nil/empty behavior compared tomake+copy: ifs.itemsis nil,slices.Clonereturns nil (whereasmake([]Item, 0)would return a non-nil empty slice). If this snippet is meant to preserve “always return a slice” semantics for callers/JSON, consider calling that out or using a pattern that keeps empty-slice semantics. Also, if the motivation is “avoid user-controlled allocation sizes”,Clonestill allocates proportional tolen(s.items)—the mitigation should be validating/capping untrusted lengths, not just changing the API used.
// Return copy to prevent mutation of internal state
func (s *Service) GetItems() []Item {
return slices.Clone(s.items)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Note: uses var+append to avoid CodeQL violations from make([]U, len(slice)) | ||
| func Map[T, U any](slice []T, fn func(T) U) []U { | ||
| result := make([]U, len(slice)) | ||
| for i, v := range slice { | ||
| result[i] = fn(v) | ||
| var result []U | ||
| for _, v := range slice { | ||
| result = append(result, fn(v)) |
There was a problem hiding this comment.
The updated Map example uses a nil slice (var result []U) and appends into it. This changes the empty-input behavior vs the previous make([]U, len(slice)): Map([]T{}, ...) will now return nil instead of a non-nil empty slice, which can affect callers (e.g., JSON null vs [], reflect.DeepEqual, or equality assertions). If you want to keep the prior semantics while still avoiding capacity/length derived from the input, consider initializing to an empty (non-nil) slice without using len(slice) for sizing, or explicitly documenting the nil-vs-empty behavior change in the guidance.
This issue also appears in the following locations of the same file:
- line 470
- line 1389
| // After: Immutable initialization using append | ||
| var filters []Filter | ||
| for _, name := range names { | ||
| filters = append(filters, Filter{Name: name}) | ||
| } |
There was a problem hiding this comment.
In this “Example transformations” snippet, the “Before” and “After” blocks are now effectively identical (both declare var filters []Filter and then append in a loop), so it no longer demonstrates a transformation. Also, the label “Immutable initialization using append” is misleading because the slice variable is still being mutated. Consider either restoring a contrasting “Before” example, or making the “After” example the functional helper call (and renaming the label to avoid implying immutability).
See below for a potential fix:
// Before: Mutable slice construction with index-based assignment
filters := make([]Filter, len(names))
for i, name := range names {
filters[i] = Filter{Name: name}
}
// After: Functional slice initialization
| activeNames := sliceutil.Map(activeItems, func(item Item) string { return item.Name }) | ||
|
|
||
| // Or inline if it's clearer: | ||
| activeNames := make([]string, 0, len(items)) | ||
| var activeNames []string | ||
| for _, item := range items { |
There was a problem hiding this comment.
This single Go code block declares activeNames multiple times (var activeNames []string, then later activeNames := ..., then again var activeNames []string). If someone copies this block into a function, it won’t compile due to redeclaration in the same scope. Consider splitting alternatives into separate code blocks or using distinct variable names per alternative (e.g., activeNamesImperative / activeNamesPipeline).
See below for a potential fix:
var activeNamesImperative []string
for _, item := range items {
if item.Active {
activeNamesImperative = append(activeNamesImperative, item.Name)
}
}
// After: Functional pipeline
activeItems := sliceutil.Filter(items, func(item Item) bool { return item.Active })
activeNamesPipeline := sliceutil.Map(activeItems, func(item Item) string { return item.Name })
// Or inline if it's clearer:
var activeNamesInline []string
for _, item := range items {
if item.Active {
activeNamesInline = append(activeNamesInline, item.Name)
| - Inline loops are often clearer than generic helpers | ||
| - Use type parameters (generics) for helpers to avoid reflection | ||
| - Preallocate slices when size is known: `make([]T, len(input))` | ||
| - Avoid `make([]T, len(input))` and `make([]T, 0, len(input))` — use `var result []T` + `append` instead; CodeQL flags these patterns because the slice length/capacity is derived from user-controlled input, which can trigger incorrect memory allocation analysis |
There was a problem hiding this comment.
The new guideline text attributes the CodeQL finding to “incorrect memory allocation analysis”, but the underlying concern is typically uncontrolled or potentially large allocations when sizing from untrusted/user-controlled input (DoS risk). Consider rewording to emphasize validating/capping untrusted sizes (and that preallocation is fine for trusted, bounded lengths) rather than a blanket prohibition, so the guidance doesn’t read like it’s just teaching people to evade CodeQL.
| - Avoid `make([]T, len(input))` and `make([]T, 0, len(input))` — use `var result []T` + `append` instead; CodeQL flags these patterns because the slice length/capacity is derived from user-controlled input, which can trigger incorrect memory allocation analysis | |
| - When sizing slices or maps from user-controlled or otherwise untrusted input, validate and cap the size before calling `make` to avoid uncontrolled allocations and potential DoS; preallocating with `make([]T, len(input))` or `make([]T, 0, len(input))` is fine when `len(input)` comes from trusted, bounded, or already-validated data |
CodeQL flags
make([]T, len(x))andmake([]T, 0, len(x))as violations. Thefunctional-pragmatistworkflow was actively recommending these patterns in helper examples and guidelines.Changes
make([]U, len(slice))+ index assignment →var result []U+appendmake([]T, 0, len(slice))→var result []T+appendmake([]Filter, len(names))+ index assignment →var filters []Filter+appendmake([]string, 0, len(items))→var activeNames []stringmake([]Item, len(s.items))+copy→slices.Clone(s.items)make([]T, len(input))with an explicit warning explaining why CodeQL flags these patterns