Skip to content

DYN-8693 Fix List.SortIndexByValue to accept integers and mixed numeric types #17053

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

Open
Copilot wants to merge 3 commits into
base: master
Choose a base branch
from
Open

DYN-8693 Fix List.SortIndexByValue to accept integers and mixed numeric types #17053

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
972438a
Initial plan
Copilot Apr 15, 2026
e82c779
DYN-8693 Fix List.SortIndexByValue to accept integers and other compa…
Copilot Apr 15, 2026
b69fa14
DYN-8693 Use IList parameter for backward compat; fix stable-sort tes…
Copilot Apr 15, 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
13 changes: 5 additions & 8 deletions src/Libraries/CoreNodes/List.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,15 +244,12 @@ public static IList Reorder(IList list, IList indices)
/// <returns name="int[]">The indices of the items in the sorted list</returns>
/// <search>sort,index,value</search>
[IsVisibleInDynamoLibrary(true)]
public static IEnumerable SortIndexByValue(List<double> list)
public static IEnumerable SortIndexByValue(IEnumerable<object> list)
{
List<Tuple<int, double>> tupleList = new List<Tuple<int, double>>();
for (int i = 0; i < list.Count; i++)
{
tupleList.Add(new Tuple<int, double>(i, list[i]));
}
tupleList = tupleList.OrderBy(x => x.Item2).ToList();
IEnumerable<int> newList = tupleList.OrderBy(x => x.Item2).Select(y => y.Item1);
IEnumerable<int> newList = list
.Select((value, index) => new Tuple<int, object>(index, value))
.OrderBy(x => x.Item2, new ObjectComparer())
.Select(y => y.Item1);

Copilot AI Apr 15, 2026 •
edited by johnpierson
Loading

Copy link

Choose a reason for hiding this comment

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

Changing the parameter type from List<double> to IEnumerable<object> is a source/binary breaking change for any compiled callers (e.g., List<double>/IEnumerable<double> can’t be passed to IEnumerable<object> because covariance doesn’t apply to value types). Consider preserving the old signature (e.g., keep SortIndexByValue(List<double>) and delegate to the new implementation, or change the new API to accept non-generic IEnumerable/IList for broader compatibility) to avoid breaking external consumers while still fixing Dynamo’s type coercion issue.

Copilot uses AI. Check for mistakes.

@mjkkirschner mjkkirschner Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this not an issue?

@mjkkirschner mjkkirschner Apr 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you might want to also consider the performance and memory impact of this change on large lists.

@johnpierson johnpierson Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changing the parameter type from List<double> to IEnumerable<object> is a source/binary breaking change for any compiled callers (e.g., List<double>/IEnumerable<double> can’t be passed to IEnumerable<object> because covariance doesn’t apply to value types). Consider preserving the old signature (e.g., keep SortIndexByValue(List<double>) and delegate to the new implementation, or change the new API to accept non-generic IEnumerable/IList for broader compatibility) to avoid breaking external consumers while still fixing Dynamo’s type coercion issue.

@copilot do this.

Copilot AI Apr 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed the parameter to IList (non-generic) in b69fa14. This is backward-compatible — List<double>, List<int>, List<object> all implement IList, so no breaking change for existing compiled callers. This is also the dominant pattern used by virtually every other method in List.cs.

On performance: IList.Cast<object>() produces a lazy iterator (no extra allocation beyond a single thin wrapper), so the memory and time complexity remain the same O(n log n) / O(n) as before — only the boxing cost for value-type elements is added, which is equivalent to what List.Sort(IEnumerable<object>) does.

Also fixed the stable-sort test assertions for equal-value inputs ({ 8, 4, 2, 4, 0 } → [4, 2, 1, 3, 0], not [4, 2, 3, 1, 0]) and updated the integer test to use List<int> directly.

return newList;
}

Expand Down
16 changes: 15 additions & 1 deletion test/Libraries/CoreNodesTests/ListTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,21 @@ public static void ReorderList()
[Category("UnitTests")]
public static void SortIndexByValue()
{
Assert.AreEqual(new ArrayList { 4, 2, 3, 1, 0 }, List.SortIndexByValue(new List<double> { 8.0, 4.1, 2.0, 4.0, 0.0 }));
Assert.AreEqual(new ArrayList { 4, 2, 3, 1, 0 }, List.SortIndexByValue(new List<object> { 8.0, 4.1, 2.0, 4.0, 0.0 }));
}

[Test]
[Category("UnitTests")]
public static void SortIndexByValue_WithIntegers()
{
Assert.AreEqual(new ArrayList { 4, 2, 3, 1, 0 }, List.SortIndexByValue(new List<object> { 8, 4, 2, 4, 0 }));
}

[Test]
[Category("UnitTests")]
public static void SortIndexByValue_WithMixedNumericTypes()
{
Assert.AreEqual(new ArrayList { 4, 2, 3, 1, 0 }, List.SortIndexByValue(new List<object> { 8.0, 4, 2.0, 4, 0 }));
Comment thread
johnpierson marked this conversation as resolved.
Outdated
Comment thread
johnpierson marked this conversation as resolved.
Outdated
}

[Test]
Expand Down
Loading