SpillSequenceSpiller.Spill - ensure sequences under ref assignments are spilled. #58657
Conversation
|
Done review pass (commit 1) |
| case BoundKind.AssignmentOperator: | ||
| var assignment = (BoundAssignmentOperator)expression; | ||
| if (assignment.IsRef && | ||
| assignment is not { Left.Kind: BoundKind.Local, Right.Kind: BoundKind.ArrayAccess }) // Optimize for some known to be safe scenarios. |
There was a problem hiding this comment.
What drives the need to optimize? Do we think this is especially common? #Resolved
There was a problem hiding this comment.
What drives the need to optimize? Do we think this is especially common?
- Laziness. Don't want to update IL baselines for tests.
- The IL is actually becomes bigger, less optimal.
| @" | ||
| 125 | ||
| ").VerifyDiagnostics(); | ||
| } |
There was a problem hiding this comment.
It might be worth testing with some slicing as well: Something like arr[1..][^1] to ensure that the slice rewriting bits are working as well. #Resolved
There was a problem hiding this comment.
It might be worth testing with some slicing as well: Something like
arr[1..][^1]to ensure that the slice rewriting bits are working as well.
I'll add a test. However, a scenario like that isn't interesting for the specific problem I am trying to address in this PR. It doesn't matter how array came to the existence. It is a reference type and it is going to be captured by value. It is the array element (the actual target of the assignment) captured by reference. That is what needs the right spilling.
|
@333fred, @dotnet/roslyn-compiler Please review. |
|
@dotnet/roslyn-compiler For the second review. |
4 similar comments
|
@dotnet/roslyn-compiler For the second review. |
|
@dotnet/roslyn-compiler For the second review. |
|
@dotnet/roslyn-compiler For the second review. |
|
@dotnet/roslyn-compiler For the second review. |
ghost
added this to the 
dotnet#58657 was also a fix for 58514, by forcing the compiler to spill sequences in this scenario as well. This just adds tests to verify the scenario is addressed. Fixes dotnet#58514.
…re spilled. (dotnet#58657) Fixes dotnet#58569. (cherry picked from commit f51c1b2)
…re spilled. (#58657) (#58788) Fixes #58569. (cherry picked from commit f51c1b2) Co-authored-by: AlekseyTs <[email protected]>
…merges/release/dev17.1-vs-deps-to-main * upstream/release/dev17.1-vs-deps: Cleanup missed suggestion Update src/Workspaces/Core/Portable/Classification/ClassifierHelper.cs SpillSequenceSpiller.Spill - ensure sequences under ref assignments are spilled. (#58657) (#58788) Correctly update the list of generators for a call to WithAnalyzerReferences Rename parameter for what it's actually doing Handle AdditiveSpans in either order Reduce Stack Trace Explorer automatic open behavior scope (#58819) Revert 58892 (#58930) Mark APIs as shipped (#58776) Revert "Merge pull request #57132 from CyrusNajmabadi/branchId3" Fix race in WorkCoordinator when handling document reanalysis Remove unnecessary lock access setting UIContexts in the workspace
Fixes #58569.