Fix fragile List::binarySearch() usage #10047

github-actions · 2026-03-20T18:24:12Z

🟡 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: error

This exercises the findDeclsLowestCommonAncestor → indexOf path with both forward and reverse orderings of sibling generic parameters.

coderabbitai · 2026-02-17T01:08:40Z

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, locate and examine the List class definition and indexOf method rg -B2 -A10 'indexOf\(' --type cpp --type hpp | head -100

Repository: shader-slang/slang

Length of output: 89

🏁 Script executed:

# Check Index type definition rg 'typedef.*Index|using.*Index' --type hpp | head -30

Repository: 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.cpp

Repository: 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 -80

Repository: shader-slang/slang

Length of output: 89

🏁 Script executed:

# Check ContainerDecl::getMembers implementation rg -B2 -A10 'getMembers\(\)' --type hpp --type cpp | head -100

Repository: 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 -150

Repository: shader-slang/slang

Length of output: 7585

🏁 Script executed:

# Check Index type definition rg 'typedef.*Index|using.*Index' --type h | head -50

Repository: shader-slang/slang

Length of output: 3685

🏁 Script executed:

# Search for GenericDecl class definition rg -A20 'class GenericDecl' --type h | head -100

Repository: shader-slang/slang

Length of output: 5103

🏁 Script executed:

# Check ContainerDecl implementation rg -A10 'class ContainerDecl' --type h | head -150

Repository: shader-slang/slang

Length of output: 2891

🏁 Script executed:

# Search for getMembers in headers rg -B2 -A8 'getMembers\(' --type h --type c | head -100

Repository: 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 -40

Repository: 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.h

Repository: shader-slang/slang

Length of output: 44

🏁 Script executed:

# Search for getDirectMemberDecls implementation rg -B2 -A15 'getDirectMemberDecls' --type c --type h | head -150

Repository: 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.h

Repository: 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.h

Repository: shader-slang/slang

Length of output: 602

🏁 Script executed:

# Find getDirectMemberDecls implementation in cpp files rg -B2 -A20 'getDirectMemberDecls\(\)' --type c | head -100

Repository: 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 -50

Repository: 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.h

Repository: shader-slang/slang

Length of output: 284

🏁 Script executed:

# Search for getDirectMemberDecls implementation in cpp rg -B3 -A15 'List<Decl\*> const& ContainerDecl::getDirectMemberDecls' --type c

Repository: 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 -20

Repository: 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.cpp

Repository: 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.cpp

Repository: 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 generic

Repository: 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 c

Repository: 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.cpp

Repository: 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 c

Repository: 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 -60

Repository: 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.h

Repository: 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 -50

Repository: 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 -80

Repository: 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.cpp

Repository: 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 -100

Repository: 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.h

Repository: 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 -50

Repository: 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 -80

Repository: 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 h

Repository: 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 c

Repository: 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.h

Repository: 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.

Suggested change

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.

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.

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.

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.

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.

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.

@cmarcelo Where is the test? This is #10047, I don't see it in the diff.

Ahh, #10447. I'm half-blind.

github-actions · 2026-03-09T00:46:41Z

indexOf() returns -1 when the element is not found. The subtraction int(supIndex - subIndex) then produces incorrect results:

Both not found → int(-1 - (-1)) = 0, incorrectly signaling equality.

One not found → a nonsensical comparison value.

(I see this was already discussed, and the author found a real case where the assumption doesn't hold.)

Suggestion: Consider replacing this block with the existing _compareDeclsInCommonParentByOrderOfDeclaration() helper (line ~12452 in this same file), which already handles exactly this use case — comparing two declarations by order of appearance in a common parent — and includes proper error handling via SLANG_UNEXPECTED. The compareDecls() function already uses this same pattern at line ~12575:

if (ContainerDecl* lca = findDeclsLowestCommonAncestor(lLCAChild, rLCAChild)) { res = _compareDeclsInCommonParentByOrderOfDeclaration(lca, lLCAChild, rLCAChild); }

Note that _compareDeclsInCommonParentByOrderOfDeclaration asserts lhs->parentDecl == commonParent, which should hold after findDeclsLowestCommonAncestor modifies the pointers by reference. If the case you found violates this invariant too, it may point to a deeper issue in the LCA logic worth investigating.

I think the current code (and the new version here) will just give "an ordering" and the proper diagnostic will already happen later -- I can definitely add such new case but would like some developer familiar with the code to make the call here.

github-actions · 2026-03-17T19:04:17Z

Missing defensive assertions & existing helper

Two concerns with this indexOf replacement:

1. No guard for indexOf returning -1. While unlikely in practice (since findDeclsLowestCommonAncestor modifies subAncestor/supAncestor by-reference to be direct children of ancestor), the subtraction int(supIndex - subIndex) would silently produce a wrong result if either lookup fails. At minimum, add:

SLANG_ASSERT(subIndex >= 0); SLANG_ASSERT(supIndex >= 0);

2. Consider reusing _compareDeclsInCommonParentByOrderOfDeclaration(). This function already exists at line ~12770 in the same file and does exactly this comparison — finding the relative order of two sibling declarations in a common parent — with proper assertions, an optimized bidirectional search (from both ends), and important documented ordering semantics (the TODO(tfoley) comment about subtle ordering constraints).

This is exactly the pattern compareDecls() uses at line ~12893:

if (ContainerDecl* lca = findDeclsLowestCommonAncestor(lLCAChild, rLCAChild)) res = _compareDeclsInCommonParentByOrderOfDeclaration(lca, lLCAChild, rLCAChild);

Replacing the indexOf block with a single call to _compareDeclsInCommonParentByOrderOfDeclaration(ancestor, subAncestor, supAncestor) would be more robust and consistent. (It would need a forward declaration or relocation since it's currently static and defined later in the file.)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fragile List::binarySearch() usage #10047

Uh oh!

Diff view

Diff view

There are no files selected for viewing

github-actions bot Mar 20, 2026

Uh oh!

coderabbitai bot Feb 17, 2026

Uh oh!

cmarcelo Feb 18, 2026

Uh oh!

jkwak-work Feb 23, 2026

Uh oh!

cmarcelo Feb 23, 2026

Uh oh!

cmarcelo Feb 26, 2026

Uh oh!

cmarcelo Mar 9, 2026

Uh oh!

jhelferty-nv Mar 20, 2026

Uh oh!

jhelferty-nv Mar 20, 2026

Uh oh!

github-actions bot Mar 9, 2026

Uh oh!

cmarcelo Mar 9, 2026

Uh oh!

github-actions bot Mar 17, 2026

Uh oh!

Uh oh!

Fix fragile List::binarySearch() usage #10047

Uh oh!

Fix fragile List::binarySearch() usage #10047

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

github-actions bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 17, 2026

Choose a reason for hiding this comment

Missing defensive assertions & existing helper

Uh oh!

Uh oh!