Skip to content

Return insertion point from List::binarySearch() misses #10048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cmarcelo wants to merge 3 commits into from
Closed

Return insertion point from List::binarySearch() misses #10048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89d2220
Fix fragile List::binarySearch() usage
cmarcelo Feb 16, 2026
eeb7f47
Return insertion point from List::binarySearch() misses
cmarcelo Feb 16, 2026
966dc2a
Merge branch 'master' into binary-search-return-insertion-point-on-mi…
jhelferty-nv Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions source/core/slang-list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,8 @@ class List
else
imax = imid - 1;
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Consider adding a brief doc comment above the method signature to document the return-value contract for future callers unfamiliar with the bitwise complement convention. Something like:

Suggested change
imax = imid - 1;
// If not found, return the bitwise negation of the insertion index.
// Callers can check `result >= 0` for found, and recover the
// insertion index with `~result` for not-found.
return ~imin;

}
// TODO: The return value on a failed search should be
// the bitwise negation of the index where `obj` should
// be inserted to be in the proper sorted location.
return -1;
// If not found, return the bitwise negation of the insertion index.
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The inline comment is accurate but minimal for a contract-breaking change to a core utility. Consider adding a doc comment above the method signature documenting the full return value semantics, since the bitwise complement convention is non-obvious to anyone who hasn't seen it in Java/C#:

Suggested change
// If not found, return the bitwise negation of the insertion index.
// If not found, return the bitwise negation of the insertion index.
// A non-negative return means the element was found at that index.
// A negative return means not found; use ~result to get the insertion point.

Comment on lines 578 to +579
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Consider expanding the comment slightly to document the full return-value contract at the method level, since callers need to know how to interpret both success and failure cases. Something like:

// If not found, return the bitwise negation of the insertion index.
// Callers can test result < 0 for "not found", and use ~result to obtain the insertion point.

This is minor — the existing comment is accurate, and the unit tests serve as living documentation for the convention.

return ~imin;
}

template<typename T2>
Expand Down
4 changes: 2 additions & 2 deletions source/slang/slang-check-decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3689,8 +3689,8 @@ void SemanticsDeclHeaderVisitor::checkGenericTypeEqualityConstraintSubType(
return compareDecls(subAncestor, supAncestor);
}

auto subIndex = ancestor->getMembers().binarySearch(subAncestor);
auto supIndex = ancestor->getMembers().binarySearch(supAncestor);
auto subIndex = ancestor->getMembers().indexOf(subAncestor);
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix. This was a latent bug: getMembers() returns members in declaration order, not sorted by pointer value, so binarySearch with the default < comparator on Decl* was using memory address ordering — essentially undefined for finding an element. It happened to work in the old code because both misses returned -1 (difference = 0), but would have been wrong any time one was found and the other wasn't.

Switching to indexOf (linear scan) is the correct approach for an unsorted list. The O(n) vs O(log n) performance difference is irrelevant here since member lists are typically small.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The old code was using binarySearch on getMembers() which is a List<Decl*> in declaration order — not sorted by pointer address. The default comparer uses </== on pointer values, so binarySearch could silently miss elements that are present. Switching to indexOf (linear scan with pointer equality) is the correct fix and also incidentally fixes a pre-existing latent bug.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix. The original code was using binarySearch with the default comparer (which compares Decl* pointer values), but getMembers() is not sorted by pointer address. indexOf (linear scan with pointer equality) is the correct approach here, and is consistent with how _compareDeclsInCommonParentByOrderOfDeclaration (line ~12790) does the same kind of member-order lookup via linear iteration.

auto supIndex = ancestor->getMembers().indexOf(supAncestor);
Comment on lines +3692 to +3693
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard against indexOf misses before subtracting.

indexOf returns -1 if either ancestor isn’t present in the member list; that collapses ordering (or yields a bogus diff) and can trigger the “canonical order” diagnostic incorrectly. Consider asserting presence or falling back to compareDecls when either index is < 0.

💡 Suggested defensive fallback 
-        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 < 0 || supIndex < 0)
+        {
+            return compareDecls(subAncestor, supAncestor);
+        }
📝 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 < 0 || supIndex < 0)
{
return compareDecls(subAncestor, supAncestor);
}
🤖 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 code
computes subIndex and supIndex via
ancestor->getMembers().indexOf(subAncestor/supAncestor) but doesn’t handle
indexOf returning -1; if either is <0 the subtraction yields bogus ordering and
triggers the canonical order diagnostic incorrectly. Update the logic around the
indexOf calls (variables subIndex and supIndex) to assert presence or,
preferably, check for <0 and in that case fall back to calling compareDecls(…)
(or otherwise skip the ordering subtraction) so you only use the index
subtraction when both indices are >= 0; ensure the canonical-order diagnostic
path uses the fallback when a member isn’t found.


return int(supIndex - subIndex);
};
Expand Down
2 changes: 1 addition & 1 deletion source/slang/slang-language-server-auto-format.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ List<Edit> formatSource(
return 1;
return 0;
});
if (exclusionRangeId != -1)
if (exclusionRangeId >= 0)
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Under the old semantics != -1 happened to work because misses always returned exactly -1. With the new semantics, misses return ~imin (e.g. -1, -2, -3…), so >= 0 is the right way to partition found (non-negative) from not-found (negative).

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct update. With the new binarySearch semantics, >= 0 is the canonical way to test for "found". 👍

continue;
pos += lengthStr.getLength();
if (pos < line.getLength() && line[pos] == '\'')
Expand Down
138 changes: 138 additions & 0 deletions tools/slang-unit-test/unit-test-list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// unit-test-list.cpp

#include "core/slang-basic.h"
#include "unit-test/slang-unit-test.h"

using namespace Slang;

namespace
{
template<typename T, typename GetValueFunc>
void checkListIsSortedBy(const List<T>& list, const GetValueFunc& getValue)
{
for (Index i = 1; i < list.getCount(); i++)
SLANG_CHECK(getValue(list[i - 1]) <= getValue(list[i]));
}

template<typename T>
void checkListIsSorted(const List<T>& list)
{
checkListIsSortedBy(list, [](const T& value) { return value; });
}
} // namespace

SLANG_UNIT_TEST(list)
{
{
List<int> values = {10, 20, 30, 40};

SLANG_CHECK(values.binarySearch(5) == ~0);
SLANG_CHECK(values.binarySearch(10) == 0);
SLANG_CHECK(values.binarySearch(25) == ~2);
SLANG_CHECK(values.binarySearch(30) == 2);
SLANG_CHECK(values.binarySearch(50) == ~4);

auto insertAndCheck = [&](int queryValue, Index expectedInsertIndex)
{
Index searchResult = values.binarySearch(queryValue);
SLANG_CHECK(searchResult == ~expectedInsertIndex);

Index insertIndex = ~searchResult;
values.insert(insertIndex, queryValue);

checkListIsSorted(values);

Index foundIndex = values.binarySearch(queryValue);
SLANG_CHECK(foundIndex >= 0);
SLANG_CHECK(values[foundIndex] == queryValue);
};

insertAndCheck(5, 0);
insertAndCheck(25, 3);
insertAndCheck(50, 6);
}

{
struct Entry
{
int key;
};

List<Entry> entries;
entries.add(Entry{1});
entries.add(Entry{4});
entries.add(Entry{9});

auto comparer = [](const Entry& entry, int key)
{
if (entry.key < key)
return -1;
if (entry.key > key)
return 1;
return 0;
};

auto insertAndCheck = [&](int queryKey, Index expectedInsertIndex)
{
Index searchResult = entries.binarySearch(queryKey, comparer);
SLANG_CHECK(searchResult == ~expectedInsertIndex);

Index insertIndex = ~searchResult;
entries.insert(insertIndex, Entry{queryKey});

checkListIsSortedBy(entries, [](const Entry& entry) { return entry.key; });

Index foundIndex = entries.binarySearch(queryKey, comparer);
SLANG_CHECK(foundIndex >= 0);
SLANG_CHECK(entries[foundIndex].key == queryKey);
};

SLANG_CHECK(entries.binarySearch(0, comparer) == ~0);
SLANG_CHECK(entries.binarySearch(4, comparer) == 1);
SLANG_CHECK(entries.binarySearch(6, comparer) == ~2);
SLANG_CHECK(entries.binarySearch(10, comparer) == ~3);

insertAndCheck(0, 0);
insertAndCheck(6, 3);
insertAndCheck(10, 5);
}

// Empty list
{
List<int> values;
SLANG_CHECK(values.binarySearch(42) == ~0);
}

// Single element
{
List<int> values = {10};
SLANG_CHECK(values.binarySearch(10) >= 0);
SLANG_CHECK(values.binarySearch(5) == ~0);
SLANG_CHECK(values.binarySearch(15) == ~1);

Index searchResult = values.binarySearch(5);
values.insert(~searchResult, 5);
SLANG_CHECK(values.getCount() == 2);
checkListIsSorted(values);
SLANG_CHECK(values[0] == 5);
SLANG_CHECK(values[1] == 10);
SLANG_CHECK(values.binarySearch(5) >= 0);
}

// Duplicate values
{
List<int> values = {10, 20, 20, 30};

Index foundIndex = values.binarySearch(20);
SLANG_CHECK(foundIndex >= 0);
SLANG_CHECK(values[foundIndex] == 20);

Index searchResult = values.binarySearch(15);
SLANG_CHECK(searchResult < 0);
values.insert(~searchResult, 15);

checkListIsSorted(values);

SLANG_CHECK(values.binarySearch(15) >= 0);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +1 to +138
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent test coverage. The tests exercise:

  • Basic hits and misses with exact insertion-point verification
  • Insert-and-verify round-trip pattern (the primary use case for the new semantics)
  • Custom comparers with heterogeneous types
  • Empty list, single element, and duplicate value edge cases

The insertAndCheck lambda pattern nicely demonstrates the intended usage of ~result as an insertion index.

Loading