Fix fragile List::binarySearch() usage#10047
Fix fragile List::binarySearch() usage#10047jhelferty-nv merged 3 commits intoshader-slang:masterfrom
Conversation
|
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
|
This patch was motivated by #10048. |
There was a problem hiding this comment.
Pull request overview
This PR fixes fragile usage of List::binarySearch() in the semantic checking of generic type equality constraints. The old code incorrectly used binarySearch() with pointer comparison on a member list ordered by declaration position, which only worked by chance because the allocator happened to produce pointers in declaration order. This is replaced with the existing helper function _compareDeclsInCommonParentByOrderOfDeclaration() that properly compares declarations by their position in the parent container, following the guidance from PR #7242 to use mediated access to ContainerDecl members.
Changes:
- Replaced fragile
binarySearch()usage with proper declaration order comparison helper function - Added forward declaration for
_compareDeclsInCommonParentByOrderOfDeclaration()
cmarcelo
force-pushed
the
fix-fragile-binary-search-usage
branch
from
ecc4e7a to
a9dd060
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-check-decl.cpp`:
- Around line 3581-3582: The subtraction using auto subIndex =
ancestor->getMembers().indexOf(subAncestor) and auto supIndex =
ancestor->getMembers().indexOf(supAncestor) must guard against indexOf returning
-1; if either subIndex or supIndex is -1, avoid using int(supIndex - subIndex)
and instead fall back to the existing compareDecls(...) logic (or assert/handle
error) to determine ordering. Update the code around where subIndex/supIndex are
computed so you check (subIndex < 0 || supIndex < 0) and call compareDecls(…)
when invalid, otherwise keep the current subtraction path.
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | ||
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the List class definition and indexOf method
rg -B2 -A10 'indexOf\(' --type cpp --type hpp | head -100Repository: shader-slang/slang
Length of output: 89
🏁 Script executed:
# Check Index type definition
rg 'typedef.*Index|using.*Index' --type hpp | head -30Repository: shader-slang/slang
Length of output: 89
🏁 Script executed:
# Examine the context at lines 3581-3582
sed -n '3570,3600p' source/slang/slang-check-decl.cppRepository: shader-slang/slang
Length of output: 1031
🏁 Script executed:
# Search for GenericDecl definition and its member structure
rg -B2 -A15 'class GenericDecl' --type hpp | head -80Repository: shader-slang/slang
Length of output: 89
🏁 Script executed:
# Check ContainerDecl::getMembers implementation
rg -B2 -A10 'getMembers\(\)' --type hpp --type cpp | head -100Repository: shader-slang/slang
Length of output: 89
🏁 Script executed:
# Search for List class and indexOf method definition
rg 'indexOf' -A5 -B2 --type c --type h | head -150Repository: shader-slang/slang
Length of output: 7585
🏁 Script executed:
# Check Index type definition
rg 'typedef.*Index|using.*Index' --type h | head -50Repository: shader-slang/slang
Length of output: 3685
🏁 Script executed:
# Search for GenericDecl class definition
rg -A20 'class GenericDecl' --type h | head -100Repository: shader-slang/slang
Length of output: 5103
🏁 Script executed:
# Check ContainerDecl implementation
rg -A10 'class ContainerDecl' --type h | head -150Repository: shader-slang/slang
Length of output: 2891
🏁 Script executed:
# Search for getMembers in headers
rg -B2 -A8 'getMembers\(' --type h --type c | head -100Repository: shader-slang/slang
Length of output: 1361
🏁 Script executed:
# Get complete indexOf implementation from slang-list.h
sed -n '1,/template.*indexOf/p' source/core/slang-list.h | tail -40Repository: shader-slang/slang
Length of output: 891
🏁 Script executed:
# Find the full indexOf method with return statement
rg -B2 -A15 'template.*indexOf\(const T2' source/core/slang-list.hRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Search for getDirectMemberDecls implementation
rg -B2 -A15 'getDirectMemberDecls' --type c --type h | head -150Repository: shader-slang/slang
Length of output: 9271
🏁 Script executed:
# Check if FIDDLE() marked fields are included in member lists
rg -B5 -A10 'FIDDLE.*inner' source/slang/slang-ast-decl.hRepository: shader-slang/slang
Length of output: 532
🏁 Script executed:
# Look for how GenericDecl members are enumerated
rg -B10 -A10 'class GenericDecl' source/slang/slang-ast-decl.hRepository: shader-slang/slang
Length of output: 602
🏁 Script executed:
# Find getDirectMemberDecls implementation in cpp files
rg -B2 -A20 'getDirectMemberDecls\(\)' --type c | head -100Repository: shader-slang/slang
Length of output: 6349
🏁 Script executed:
# Search for how GenericDecl members are handled in the codebase
rg 'GenericDecl.*inner' --type c --type h | head -50Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Find the complete indexOf implementation with return statement
rg -B2 -A8 'Index indexOf' source/core/slang-list.hRepository: shader-slang/slang
Length of output: 284
🏁 Script executed:
# Search for getDirectMemberDecls implementation in cpp
rg -B3 -A15 'List<Decl\*> const& ContainerDecl::getDirectMemberDecls' --type cRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Also check slang-ast-decl.cpp for implementation
grep -n 'getDirectMemberDecls' source/slang/slang-ast-decl.cpp | head -20Repository: shader-slang/slang
Length of output: 196
🏁 Script executed:
# Look at what fields are included for GenericDecl member enumeration
rg -B5 -A20 'class GenericDecl' source/slang/slang-ast-decl.cppRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Check how FIDDLE macro works - does it include fields in member lists?
rg -B2 -A5 'FIDDLE\(\)' source/slang/slang-ast-decl.h | grep -A5 'GenericDecl'Repository: shader-slang/slang
Length of output: 385
🏁 Script executed:
# Read the implementation at line 304
sed -n '300,330p' source/slang/slang-ast-decl.cppRepository: shader-slang/slang
Length of output: 687
🏁 Script executed:
# Look for how _directMemberDecls is populated for GenericDecl
rg '_directMemberDecls' --type c --type h | grep -i genericRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Search for how GenericDecl handles its inner decl in member operations
rg -B5 -A10 'GenericDecl.*addMember\|addDirectMemberDecl.*GenericDecl' --type cRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Check if there's special handling for GenericDecl's inner in the context of checkGenericTypeEqualityConstraintSubType
sed -n '3550,3600p' source/slang/slang-check-decl.cppRepository: shader-slang/slang
Length of output: 1938
🏁 Script executed:
# Look for tests or usages that show expected behavior when ancestor is GenericDecl
rg -B10 -A10 'findDeclsLowestCommonAncestor.*GenericDecl\|GenericDecl.*indexOf' --type cRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Check the actual field structure - is inner a direct member or a special field?
rg -B10 -A30 'class GenericDecl' source/slang/slang-ast-decl.h | head -60Repository: shader-slang/slang
Length of output: 1027
🏁 Script executed:
# Search for how GenericTypeConstraintDecl relates to GenericDecl
rg -B5 -A15 'class GenericTypeConstraintDecl' source/slang/slang-ast-decl.hRepository: shader-slang/slang
Length of output: 819
🏁 Script executed:
# Look for where generic parameters are added as members
rg -B5 -A10 'addMember.*GenericTypeParam\|GenericTypeParam.*addMember' --type c | head -50Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Check if there's defensive code elsewhere that handles indexOf returning -1
rg -B3 -A3 'indexOf.*!= -1\|indexOf.*>= 0\|indexOf.*< 0' --type c --type h | head -80Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Let me look at the broader context of checkGenericTypeEqualityConstraintSubType
sed -n '3550,3620p' source/slang/slang-check-decl.cppRepository: shader-slang/slang
Length of output: 2569
🏁 Script executed:
# Check findDeclsLowestCommonAncestor to understand what it returns
rg -B5 -A20 'findDeclsLowestCommonAncestor' --type c --type h | head -100Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Look for safeguards elsewhere in the codebase when using indexOf result
rg -B2 -A5 'indexOf.*-1' source/core/slang-list.hRepository: shader-slang/slang
Length of output: 237
🏁 Script executed:
# Search for where GenericTypeParamDecl instances are added as members
rg -B10 -A10 'GenericTypeParamDecl.*addMember\|addDirectMemberDecl.*Param' --type c | head -50Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Look at the broader generic decl creation flow
rg -B5 -A15 'new GenericTypeParamDecl\|GenericTypeParamDecl\(' --type c | head -80Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# See if there's any documentation about what getMembers returns for GenericDecl
rg -B3 -A3 'GenericDecl.*getMembers\|getMembers.*GenericDecl' --type c --type hRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Check if there are any assertions or checks before using indexOf result elsewhere
rg -B2 -A2 'subIndex.*supIndex\|supIndex.*subIndex' --type cRepository: shader-slang/slang
Length of output: 44
🏁 Script executed:
# Final check: confirm Index(-1) is the standard sentinel by looking at contains patterns
rg 'indexOf.*!= -1\|indexOf.*== -1\|!= Index\(-1\)' source/core/slang-list.h source/core/slang-array.hRepository: shader-slang/slang
Length of output: 44
Add guards against indexOf returning -1 to prevent incorrect index subtraction.
At lines 3581-3582, indexOf can return -1 if subAncestor or supAncestor is not present in ancestor->getMembers(). The direct subtraction int(supIndex - subIndex) then produces incorrect ordering: (-1 - 0 = -1), (0 - (-1) = 1), or (-1 - (-1) = 0), with the zero case silently triggering the diagnostic at line 3593. Check both indices and either assert they are valid or fall back to compareDecls.
Suggested guard
auto subIndex = ancestor->getMembers().indexOf(subAncestor);
auto supIndex = ancestor->getMembers().indexOf(supAncestor);
+ if (subIndex == Index(-1) || supIndex == Index(-1))
+ return compareDecls(subAncestor, supAncestor);
return int(supIndex - subIndex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | |
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); | |
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | |
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); | |
| if (subIndex == Index(-1) || supIndex == Index(-1)) | |
| return compareDecls(subAncestor, supAncestor); | |
| return int(supIndex - subIndex); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/slang/slang-check-decl.cpp` around lines 3581 - 3582, The subtraction
using auto subIndex = ancestor->getMembers().indexOf(subAncestor) and auto
supIndex = ancestor->getMembers().indexOf(supAncestor) must guard against
indexOf returning -1; if either subIndex or supIndex is -1, avoid using
int(supIndex - subIndex) and instead fall back to the existing compareDecls(...)
logic (or assert/handle error) to determine ordering. Update the code around
where subIndex/supIndex are computed so you check (subIndex < 0 || supIndex < 0)
and call compareDecls(…) when invalid, otherwise keep the current subtraction
path.
There was a problem hiding this comment.
Would someone help verify if it is applicable, is it a valid concern? My own understanding here is that we expect both to be found.
There was a problem hiding this comment.
I think it wouldn't hurt to have SLANG_ASSERT.
The assumption of the data being sorted holds now but it may change later intentionally or unintentionally.
There was a problem hiding this comment.
Added
SLANG_ASSERT(subIndex >= 0);
SLANG_ASSERT(supIndex >= 0);
and appended to the commit message
Since we are here, document with ASSERTs the assumption that both
subIndex and supIndex will be present.
There was a problem hiding this comment.
I think I've found a case where the asserts added here don't hold, will update the PR and add the test that can trigger it.
There was a problem hiding this comment.
Adding the assert would've failed on cases where one of the sides is the ancestor. I've posted a test in #10447 that would trigger such assertion. Without the assert this case gets a correct diagnostic by slang. So I've removed the asserts from here now.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
cmarcelo
force-pushed
the
fix-fragile-binary-search-usage
branch
2 times, most recently
from
c707bf4 to
e1c927e
Compare
The binarySearch() was comparing pointer addresses on the member list that is ordered by the declaration position. That may not necessarily work unless is guaranteed that the allocator produces pointers in declaration order and they are not reordered. Use indexOf() instead. See bfae49d ("Mediate access to ContainerDecl members (shader-slang#7242)") for context.
cmarcelo
force-pushed
the
fix-fragile-binary-search-usage
branch
from
e1c927e to
89d2220
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
@jhelferty-nv the PR mentioned is on top of this one, so all things being equal I rather we land (or reject) this change independently if possible. |
|
Ah. Let's get it in then. |
There was a problem hiding this comment.
Verdict: ✅ Clean — no significant issues found. 1 gap noted.
This PR replaces two binarySearch() calls with indexOf() in checkGenericTypeEqualityConstraintSubType. The old code used binarySearch() which compares pointer addresses via operator<, but the member list is ordered by declaration position — not by memory address. This made the binary search both incorrect (searching a list not sorted by the search predicate) and non-deterministic (dependent on allocator behavior). The indexOf() replacement uses identity comparison (==) with linear scan, which is correct and consistent with the existing pattern in _compareDeclsInCommonParentByOrderOfDeclaration (line 12866).
Changes Overview
Semantic checker — sibling parameter ordering (source/slang/slang-check-decl.cpp)
- What changed: Replaced
ancestor->getMembers().binarySearch(...)withancestor->getMembers().indexOf(...)for two lookups that determine declaration order of sibling generic parameters sharing a common ancestor. The O(n) cost is acceptable given typical generic parameter counts and compile-time-only execution.
Findings (1 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | slang-check-decl.cpp:3718 |
No existing test exercises sibling-parameter equality constraint ordering (the exact path this fix targets) |
|
|
||
| auto subIndex = ancestor->getMembers().binarySearch(subAncestor); | ||
| auto supIndex = ancestor->getMembers().binarySearch(supAncestor); | ||
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); |
There was a problem hiding this comment.
🟡 Gap: No regression test for sibling-parameter equality constraint ordering
The code comments at lines 3706-3710 describe the exact scenario this fixes: __generic<A : IA, B : IB> where A::T == B::T — two sibling parameters in the same GenericDecl with an associated-type equality constraint. However, no existing test exercises this specific path. The closest tests (type-equality-cononical.slang, type-equality-ancestor-ordering.slang) use interface self-types or nesting scenarios, not sibling parameters within the same generic.
Since the old binarySearch bug could silently produce wrong ordering depending on allocator behavior (non-deterministic), a targeted test would guard against future regressions in this path.
Suggestion: Consider adding a test like:
//TEST:SIMPLE(filecheck=CHECK):
interface IHasAssoc { associatedtype T; }
struct Impl : IHasAssoc { typealias T = int; }
__generic<A : IHasAssoc, B : IHasAssoc>
void testSiblings(A a, B b)
where A::T == B::T
{}
// CHECK-NOT: errorThis exercises the findDeclsLowestCommonAncestor → indexOf path with both forward and reverse orderings of sibling generic parameters.
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | ||
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); |
The binarySearch() was comparing pointer addresses on the member list
that is ordered by the declaration position. That may not necessarily
work unless is guaranteed that the allocator produces pointers in
declaration order and they are not reordered. Use indexOf() instead.
See bfae49d ("Mediate access to ContainerDecl members (#7242)") for context.