Skip to content

Conversation

sashaodessa
Copy link

  • Summary: Replace culture-sensitive string operations in PluginLoader.OrderPlugins with culture-invariant, deterministic comparisons.
  • Changes:
    • Replace ToLower()/CompareTo with:
      • StringComparer.OrdinalIgnoreCase for the ordering map
      • string.Compare(..., StringComparison.Ordinal) for tie-break sorting
    • Build orderMap to avoid linear IndexOf lookups.
  • Why: Prevent non-deterministic behavior on non-EN locales (e.g., Turkish “i”) and ensure consistent plugin order across environments.

@LukaszRozmej
Copy link
Member

Ok I'm now trying to think does it really make that much sense...

  1. Plugin's are never ordered by name - only looked up by it, thus locale shouldn't really matter
  2. There is only a small handful of ordered plugins (literally a few), so not sure if worth paying for a Dictionary - probably doesn't matter much one way or the other.

@sashaodessa
Copy link
Author

Ok I'm now trying to think does it really make that much sense...

  1. Plugin's are never ordered by name - only looked up by it, thus locale shouldn't really matter
  2. There is only a small handful of ordered plugins (literally a few), so not sure if worth paying for a Dictionary - probably doesn't matter much one way or the other.

@LukaszRozmej Thanks for the thoughtful take. The goal here isn’t performance; it’s to make the behavior fully deterministic and culture‑invariant across machines.

  • On (1): Even if we never sort by name, any OrderBy/Sort or key lookup that relies on the default string comparer is culture‑sensitive. On nodes with different locales (e.g., tr‑TR), the i/İ rules change ordering and equality, which risks non‑determinism and flaky tests.
  • On (2): The cost is essentially zero. Using StringComparer.OrdinalIgnoreCase is cheaper than culture‑aware comparisons; with only a few ordered plugins, the overhead is negligible. After construction there are no extra allocations.

Comment on lines 116 to 126
if (!fHas)
{
if (sPos == -1)
if (!sHas)
{
return f.Name.CompareTo(s.Name);
return string.Compare(f.Name, s.Name, StringComparison.OrdinalIgnoreCase);
}

return 1;
}

if (sPos == -1)
if (!sHas)
Copy link
Member

Choose a reason for hiding this comment

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

btw you didn't rename the usages of the flags.

@sashaodessa
Copy link
Author

@LukaszRozmej updated

@LukaszRozmej LukaszRozmej self-requested a review September 8, 2025 10:56
@sashaodessa
Copy link
Author

@LukaszRozmej

@LukaszRozmej
Copy link
Member

This somewhat conflicts with #9264, which we need to resolve first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants