Ensure we have a bound node for the unsafe block so that region analysis works correctly.#45334
Ensure we have a bound node for the unsafe block so that region analysis works correctly.#45334gafter merged 3 commits intodotnet:masterfrom
Conversation
…sis works correctly. Fixes dotnet#4950
| // The C# binder produces an extra block for checked, unchecked, and unsafe statements, but they have no semantics of their own | ||
| boundBlock = boundBlock switch | ||
| { | ||
| { Syntax: CheckedStatementSyntax _, Locals: { IsDefaultOrEmpty: true }, Statements: { Length: 1 } s } => (BoundBlock)s[0], |
There was a problem hiding this comment.
Syntax: CheckedStatementSyntax [](start = 18, length = 30)
I think we are trying to follow the principle that semantic meaning of bound nodes shouldn't depend on their associated syntax. This condition violates the principle. #Closed
| // The C# binder produces an extra block for checked, unchecked, and unsafe statements, but they have no semantics of their own | ||
| boundBlock = boundBlock switch | ||
| { | ||
| { Syntax: CheckedStatementSyntax _, Locals: { IsDefaultOrEmpty: true }, Statements: { Length: 1 } s } => (BoundBlock)s[0], |
There was a problem hiding this comment.
Locals [](start = 52, length = 6)
The block could also have LocalFunctions. #Closed
| // The C# binder produces an extra block for checked, unchecked, and unsafe statements, but they have no semantics of their own | ||
| boundBlock = boundBlock switch | ||
| { | ||
| { Syntax: CheckedStatementSyntax _, Locals: { IsDefaultOrEmpty: true }, Statements: { Length: 1 } s } => (BoundBlock)s[0], |
There was a problem hiding this comment.
(BoundBlock) [](start = 121, length = 12)
The unconditional cast makes the code fragile, I believe. #Closed
Could you please elaborate why is that dummy block necessary for correct behavior of the region analysis. Do we have the same issue with parenthesized expressions missing from the bound tree? If we don't, could we treat unsafe/checked/unchecked statements in a similar manner? #Closed |
|
I suspect we are failing to detect when we enter and leave the region. Could we simply dig through these statements when we determining the boundaries? #Closed |
|
Done with review pass (iteration 1) #Closed |
|
|
||
| [| | ||
| IntPtr p; | ||
| unsafe |
There was a problem hiding this comment.
| unsafe | |
| {keyword} | |
| ``` #Resolved |
| private static IntPtr NewMethod(object value) | ||
| { | ||
| IntPtr p; | ||
| unsafe |
There was a problem hiding this comment.
| unsafe | |
| {keyword} | |
| ``` #Resolved |
| } | ||
| } | ||
| "; | ||
| await TestExtractMethodAsync(code.Replace("unsafe", keyword), expected.Replace("unsafe", keyword)); |
There was a problem hiding this comment.
| await TestExtractMethodAsync(code.Replace("unsafe", keyword), expected.Replace("unsafe", keyword)); | |
| await TestExtractMethodAsync(code, expected); | |
| ``` #Resolved |
| [Fact, Trait(Traits.Feature, Traits.Features.ExtractMethod)] | ||
| [WorkItem(4950, "https://github.com/dotnet/roslyn/issues/4950")] | ||
| public async Task ExtractMethodInvolvingUnsafeBlock() |
There was a problem hiding this comment.
| [Fact, Trait(Traits.Feature, Traits.Features.ExtractMethod)] | |
| [WorkItem(4950, "https://github.com/dotnet/roslyn/issues/4950")] | |
| public async Task ExtractMethodInvolvingUnsafeBlock() | |
| [Theory, InlineData("unsafe", "checked", "unchecked")] | |
| [Trait(Traits.Feature, Traits.Features.ExtractMethod)] | |
| [WorkItem(4950, "https://github.com/dotnet/roslyn/issues/4950")] | |
| public async Task ExtractMethodInvolvingUnsafeBlock(string keyword) | |
| ``` #Resolved |
| foreach (string keyword in new[] { "unsafe", "checked", "unchecked" }) | ||
| { |
There was a problem hiding this comment.
| foreach (string keyword in new[] { "unsafe", "checked", "unchecked" }) | |
| { | |
| ``` #Resolved |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
IDE test change LGTM. Just showing the expected way to normally write a parameterized test. This has teh value of making 3 independent tests that can then be individually run in case only one of them fails and you need to debug it :)
|
@AlekseyTs @CyrusNajmabadi I think I've responded to all of your comments. Do you have any others? @333fred Would you please review this PR? |
src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (iteration 2) #Closed |
|
@AlekseyTs I think I've responded to all of your comments. Do you have any others? |
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (iteration 3), assuming CI is passing
Fixes #4950