NormalizeWhitespace introduces a space between in nullable type #22234
Conversation
|
@dotnet/roslyn-compiler for review (tiny fix). Thanks |
|
|
||
| // no space between int and ? | ||
| Assert.Equal("class C\r\n{\r\n int? P\r\n {\r\n }\r\n}", syntaxNode.ToFullString()); | ||
| } |
There was a problem hiding this comment.
Are we handling x is int ? y : z;?
There was a problem hiding this comment.
Yes. Added test.
The logic for handling conditional expressions is a few lines above where I made the change (SyntaxNormalizer.cs line 403), so it takes precedence.
| var syntaxNode2 = SyntaxFactory.ParseExpression("x is DateTime? y: z").NormalizeWhitespace(); | ||
|
|
||
| // space between DateTime and ? | ||
| Assert.Equal("x is DateTime ? y : z", syntaxNode2.ToFullString()); |
There was a problem hiding this comment.
Perhaps x is int??y:z, x as object??y as well.
There was a problem hiding this comment.
Added test. The : z portion gets parse as trivia, so I removed it.
Also added a helper method to dump syntax trees, like I recently added to VB test helpers.
jcouv
force-pushed
the
normalize-syntax
branch
from
b589d97
to
b5bb518
Compare
|
Do we have similar issue in VB? Consider adding appropriate tests for VB as well. #Closed |
| .NormalizeWhitespace(); | ||
|
|
||
| // no space between int and ? | ||
| Assert.Equal("class C\r\n{\r\n int? P\r\n {\r\n }\r\n}", syntaxNode.ToFullString()); |
There was a problem hiding this comment.
\r\n [](start = 33, length = 4)
I think the test will be more robust if it would use platform specific line breaks, Consider using verbatim string literal instead.
#Closed
There was a problem hiding this comment.
When I used a verbatim string in an earlier commit, the tests failed on Linux due to line breaks.
I'll try to see if we have another assert helper for dealing with that.
There was a problem hiding this comment.
From discussion with Andy, the issue with line terminators on Linux is known and not trivial to fix (we need to decide what the behavior should be when modifying syntax in a predominantly Windows-terminated file on Linux).
I added more details to #8537 as follow-up issue.
|
Done with review pass (iteration 5). #Closed |
* dotnet/master: (351 commits) Avoid scheduling an unnecessary dispose task for EmptyAsyncToken Support for ref-readonly locals (dotnet#22269) ported tests from dotnet#21263 (dotnet#22296) NormalizeWhitespace should not add a space in nullable type (dotnet#22234) Fix the work item number associated with the unit test Add unit test Terminate a sentence Add instructions for upgrading PowerShell Unsafe local should require /unsafe flag on compilation (dotnet#22268) CR feedback Not poisoning restricted types known to be stack only. Typo Error messsage should mention C# 7.0 not C# 7 (dotnet#22255) Package descriptions for CSharp and VB should include summary blurb (dotnet#22166) Fix a trivial issue in how IsLifted is calculated for C# compound assignment operator Do not zero out "With" statement target expression locals referenced within a lambda. (dotnet#22223) Fix unit test failure from merge Mark "private protected", "ref readonly" and other C# 7.2 features as merged (dotnet#22209) Add unit tests for IAwaitExpression and make the API public again more test fixes due to merge conflicts ...
* dotnet/master: (309 commits) Avoid scheduling an unnecessary dispose task for EmptyAsyncToken Support for ref-readonly locals (dotnet#22269) ported tests from dotnet#21263 (dotnet#22296) NormalizeWhitespace should not add a space in nullable type (dotnet#22234) Fix the work item number associated with the unit test Add unit test Terminate a sentence Add instructions for upgrading PowerShell Unsafe local should require /unsafe flag on compilation (dotnet#22268) CR feedback Not poisoning restricted types known to be stack only. Typo Error messsage should mention C# 7.0 not C# 7 (dotnet#22255) Package descriptions for CSharp and VB should include summary blurb (dotnet#22166) Fix a trivial issue in how IsLifted is calculated for C# compound assignment operator Do not zero out "With" statement target expression locals referenced within a lambda. (dotnet#22223) Fix unit test failure from merge Mark "private protected", "ref readonly" and other C# 7.2 features as merged (dotnet#22209) Add unit tests for IAwaitExpression and make the API public again more test fixes due to merge conflicts ...
Customer scenario
Write an analyzer that modifies syntax and makes a nullable type, and calls
NormalizeWhitespaceAPI. Forint?, it introduces a space (int ?).Bugs this fixes:
Fixes #21231
Risk
Performance impact
Low. The method that determines whether a separator is needed between two tokens now has an extra check (among many)
Is this a regression from a previous update?
No.
How was the bug found?
Customer.
Test documentation updated?
Not applicable.