API review rename of AnchorMode#67313
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the API review decision to rename VirtualizeAnchorMode.Beginning to VirtualizeAnchorMode.Start, updates Virtualize<TItem>.AnchorMode’s default accordingly, and adjusts the VirtualizeItemComparerAnalyzer to use concurrent collections.
Changes:
- Rename
VirtualizeAnchorMode.BeginningtoStart, including XML docs andVirtualize<TItem>.AnchorModedefault value. - Update Components unit tests and BasicTestApp test assets to use
Start. - Change the analyzer’s per-operation-block tracking collections to concurrent variants.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Components/Web/test/Virtualization/VirtualizeTest.cs | Updates test helper default anchor mode to Start. |
| src/Components/Web/src/Virtualization/VirtualizeAnchorMode.cs | Renames enum member Beginning → Start and updates docs. |
| src/Components/Web/src/Virtualization/Virtualize.cs | Updates AnchorMode default and doc reference to Start. |
| src/Components/Web/src/PublicAPI.Unshipped.txt | Updates public API baseline for the renamed enum member. |
| src/Components/test/testassets/BasicTestApp/VirtualizationAnchorModeWindowScroll.razor | Updates UI and defaults to use Start. |
| src/Components/test/testassets/BasicTestApp/VirtualizationAnchorMode.razor | Updates UI and defaults to use Start. |
| src/Components/Analyzers/src/VirtualizeItemComparerAnalyzer.cs | Switches internal tracking collections to concurrent types. |
| { | ||
| var componentStack = new Stack<ComponentState>(); | ||
| var completedVirtualizeComponents = new List<ComponentState>(); | ||
| var componentStack = new ConcurrentStack<ComponentState>(); |
There was a problem hiding this comment.
I'm somewhat still confused. While this is a thread-safe collection, I don't get how the logic can work deterministically.
Roslyn will be invoking multiple operation actions concurrently. So there is no guarantee of order of Push/TryPeek calls.
Is the implementation relying on the order of method calls in user code and relying on getting the callbacks in the same order? I don't think this would work reliable and I'm quite surprised there are no flaky tests because of that (maybe it's somehow working by chance or there is something obvious I'm missing?)
There was a problem hiding this comment.
Actually the fact that we didn't have any flaky tests related to this analyzer before switching to the concurrent collection is also concerning.
There was a problem hiding this comment.
Good point. I might have been too fast with agreeing the problem exists. In https://github.com/dotnet/aspnetcore/blame/a097744953e211590f86d0947dfcfafe14b6789d/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/RouteHandlerAnalyzer.cs#L45 that I used as an example of thread safe analyzer, the concurrent data structure is used in multiple blocks (RegisterOperationBlockStartAction, RegisterOperationBlockEndAction).
Blocks can run in parallel but code inside of one block (operation actions) always runs sequentially in the given thread:
https://github.com/dotnet/roslyn/blob/ab4cbf6de14bebf0b90771aaa1de52d780984e58/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs#L1289
That's why nothing failed before and that's why we also have other non-thread safe structures in other analyzers, as long as they are block-scoped:
I will remove the type change from this PR.
There was a problem hiding this comment.
Is the implementation relying on the order of method calls in user code and relying on getting the callbacks in the same order?
Yes but for the analyzer to stop working correctly, the implementation for ExecuteOperationActions's loop would have to change, e.g. be transformed to be parallel or some sorting/reordering would need to be added before Roslyn enters that loop.
There was a problem hiding this comment.
So you mean operation callbacks are not called in parallel? That's quite surprising to me and sounds unexpected. @333fred is it safe to assume a guaranteed ordering of operation actions?
There was a problem hiding this comment.
I do not think it is safe to assume this. If you want to iterate via a stack, you should do so yourself via RegisterOperationBlockAction, and then iterating through ChildOperations manually.
There was a problem hiding this comment.
In other words, we would not consider it a breaking change to start doing callbacks in parallel or out of order in any fashion, so long as Start comes before End.
| var componentStack = new Stack<ComponentState>(); | ||
| var completedVirtualizeComponents = new List<ComponentState>(); | ||
| var componentStack = new ConcurrentStack<ComponentState>(); | ||
| var completedVirtualizeComponents = new ConcurrentBag<ComponentState>(); |
There was a problem hiding this comment.
@halter73 pointed to me in another PR that ConcurrentBag has big perf implications and ConcurrentQueue might be preferred in most cases.
Rename following the decision in #66799 (comment).
Fixes #66799