Fix crash in foreach deconstruction over null coalescing#84417
Fix crash in foreach deconstruction over null coalescing#84417adrientoub wants to merge 2 commits into
Conversation
dotnet-policy-service
Bot
added
Community
|
@dotnet-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
This PR updates the C# compiler semantic model’s declared-symbol dispatch to handle foreach deconstruction statements (ForEachVariableStatementSyntax), and adds an analyzer regression test that exercises the foreach (var (x, y) in dict ?? new Dictionary<...>()) pattern to prevent a crash/regression.
Changes:
- Add
GetDeclaredSymbol(ForEachVariableStatementSyntax)and routeSyntaxKind.ForEachVariableStatementthrough it inGetDeclaredSymbolCore. - Add a
UseCollectionInitializercollection-expression regression test covering foreach deconstruction over null-coalescing with an object creation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs | Adds semantic-model support for ForEachVariableStatementSyntax in declared-symbol lookup and dispatch. |
| src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests_CollectionExpression.cs | Adds a regression test to ensure the analyzer/code fix handles (and doesn’t crash on) foreach deconstruction over ?? new Dictionary<...>(). |
|
Do you happen to have an exception stack for the original crash? Thanks! |
|
This fix doesn't look right to me. Nodes that have multiple variables should not return the first one and disregard the rest. They should just return nothing. That's how fields work, for example. |
| /// Given a foreach variable statement, get the symbol for the (first) iteration variable. | ||
| /// </summary> | ||
| /// <param name="forEachVariableStatement">The foreach variable statement.</param> | ||
| public ILocalSymbol GetDeclaredSymbol(ForEachVariableStatementSyntax forEachVariableStatement) |
There was a problem hiding this comment.
public ILocalSymbol GetDeclaredSymbol(ForEachVariableStatementSyntax forEachVariableStatement)
I do not think we should be adding an API like this, and with behavior like this. It sounds like the consumer should be using public abstract ISymbol GetDeclaredSymbol(SingleVariableDesignationSyntax declarationSyntax, CancellationToken cancellationToken = default(CancellationToken)); API to get symbols for the declared locals.
There was a problem hiding this comment.
Agreed with Aleksey. Nothing should be passing a ForEachVariableStatementSyntax to GetDeclaredSymbol, and if it does then that component is the broken part.
|
@adrientoub Please add a link to a GitHub issue you are trying to fix. Open one if it doesn't exist. |
The UseCollectionInitializer analyzer crashed when analyzing a foreach deconstruction statement (
foreach (var (x, y) in ...)) whose collection was a null-coalescing expression over an object creation (dict ?? new Dictionary<string, List<int>>()).Fixed by adding the missing case and adding related tests to avoid regression on this specific issue.
Found this issue when enabling analyzer on my code base and this prevented all analyzers from running until this pattern was removed.
Microsoft Reviewers: Open in CodeFlow