Fix parenthesis simplification around binary expressions #17241
Conversation
|
@Pilchie? @CyrusNajmabadi? @roslyn-ide? |
|
Looking. |
| </code> | ||
|
|
||
| Await TestAsync(input, expected) | ||
| End Function | ||
|
|
||
| #End Region |
There was a problem hiding this comment.
why are there so many changes in this file?
There was a problem hiding this comment.
The tests in this file had been organized at one point, but recently added tests weren't added with the same organization. I organized them.
There was a problem hiding this comment.
IOW, I moved a bunch of tests around.
There was a problem hiding this comment.
I marked the newly added tests. Sorry about the diff.
|
Code changes look good. Don't understand the test changes though... |
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)] | ||
| [WorkItem(11958, "https://github.com/dotnet/roslyn/issues/11958")] | ||
| public async Task EnsureParenthesesInStringConcatenation() |
There was a problem hiding this comment.
Here's a new test
| <Fact, Trait(Traits.Feature, Traits.Features.Simplification)> | ||
| Public Async Function TestVisualBasic_DontRemoveParenthesesAroundXmlElementWhenPreviousTokenIsGreaterThan() As Task | ||
| Public Async Function TestCSharp_SimplifyIfBinaryExpressionTypeIsImplicitNumericConversion() As Task |
There was a problem hiding this comment.
Here's a new test
| <Fact, Trait(Traits.Feature, Traits.Features.Simplification)> | ||
| Public Async Function TestVisualBasic_DontRemoveParenthesesAroundXmlElementWhenPreviousTokenIsLessThan() As Task | ||
| Public Async Function TestCSharp_SimplifyIfBinaryExpressionTypeIsIdentityConversion() As Task |
There was a problem hiding this comment.
Here's a new test
| <Fact, Trait(Traits.Feature, Traits.Features.Simplification)> | ||
| Public Async Function TestVisualBasic_DontRemoveParenthesesAroundEmptyXmlElementWhenPreviousTokenIsGreaterThan() As Task | ||
| Public Async Function TestCSharp_DontSimplifyIfOperatorOverloadsWouldNoLongerByCalled() As Task |
There was a problem hiding this comment.
Here's a new test
| <Fact, Trait(Traits.Feature, Traits.Features.Simplification)> | ||
| Public Async Function TestVisualBasic_DontRemoveParenthesesAroundEmptyXmlElementWhenPreviousTokenIsLessThan() As Task | ||
| Public Async Function TestCSharp_DontSimplifyIfItWouldChangeStringConcatenation() As Task |
There was a problem hiding this comment.
Here's a new test
| <Fact, Trait(Traits.Feature, Traits.Features.Simplification)> | ||
| Public Async Function TestVisualBasic_RemoveParenthesesAroundXmlElementInInvocation() As Task | ||
| Public Async Function TestCSharp_SimplifyInStringConcatenationIfItWouldNotChangeMeaning2() As Task |
There was a problem hiding this comment.
Here's a new test
| <Fact, Trait(Traits.Feature, Traits.Features.Simplification)> | ||
| Public Async Function TestVisualBasic_RemoveParenthesesAroundXmlElement() As Task | ||
| Public Async Function TestCSharp_SimplifyInStringConcatenationIfItWouldNotChangeMeaning1() As Task |
There was a problem hiding this comment.
Here's a new test
|
👍 |
|
Looking forward to getting some check marks so I can merge. 😄 |
|
I have no idea what we're supposed to target any more. @jasonmalinowski sent mail about branches and then ret-con'd it, but it wasn't clear how much was ret-con'd. So, I don't know what to target. |
|
master is safe to target, and is probably the right place to go. The only thing that was retconned was dev15.1.x. |
|
Well then, yes, it should be against master 😊 |
|
retest this please |
Fixes #11958
This change adds a couple of semantic checks when determining whether parentheses can be removed from around a addition or multiplication expression contained within another addition or multiplication expression. For example,
a + (b + c)ora * (b * c). Essentially, we're now a bit more conservative and will only remove parentheses if the following semantic rules hold true:This ensures that we don't incorrectly remove parentheses in several cases, such as the following string concatenation example.
cc @dotnet/roslyn-ide