Extensions: Use deterministic order for MergedNamespaceSymbol.GetMembers and GetExtensionMembers#80552
Extensions: Use deterministic order for MergedNamespaceSymbol.GetMembers and GetExtensionMembers#80552jcouv wants to merge 1 commit intodotnet:mainfrom
Conversation
…ers and GetExtensionMembers
49f0447 to
3026624
Compare
| copy[0] = copy[last]; | ||
| copy[last] = temp; | ||
|
|
||
| return ImmutableCollectionsMarshal.AsImmutableArray(copy); |
|
|
||
| // In DEBUG, swap the first and last elements of a read-only array, yielding a new read only array. | ||
| // This helps to avoid depending on accidentally sorted arrays. | ||
| // Use more aggressive reordering with Array.Reverse(copy) for spot-checking |
AlekseyTs
left a comment
There was a problem hiding this comment.
I do not think we should be making these changes. We do not need to ensure this level of equivalency between DEBUG and RELEASE. For the purpose of semantic analysis, the order of members doesn't matter. Deterministic emit is the goal, and I think we are good there with respect to extensions feature. The goal behind the comment in the test plan is to confirm that whatever is causing the difference won't make problems for emit. And I think it doesn't.
|
FWIW, there is an existing issue capturing that order of diagnostics should be deterministic: #1506 |
I do not agree with some of the opinions stated on the issue. BTW, the discussion happened 10 years ago and our opinions and priorities could change since then. For example, we intentionally use unordered processing in our implementation in order to boost performance as long as the produced binary is deterministic. We even have APIs that include "Unordered" in their names. In my opinion, deterministic order of diagnostic is a nice to have property, but it is not an ultimate goal. |
|
Also, note that this PR was changing behavior for scenarios that aren't necessary using extension blocks. If, for example, the change was targeting exclusively the new scenarios, I would be more sympathetic to the change. |
Addresses ordering issue tracked by #78968
Relates to test plan #76130