Update 'use coalece assignment' fixer to support additional common use case.#62191
Update 'use coalece assignment' fixer to support additional common use case.#62191CyrusNajmabadi merged 4 commits intodotnet:mainfrom
Conversation
| else | ||
| { | ||
| var coalesce = coalesceOrIfStatement; | ||
| // changing from `x ?? (x = y)` to `x ??= y` can change the type. Specifically, |
There was a problem hiding this comment.
easier to read this with whitespace off.
| { | ||
| private static string s_goo; | ||
| private static string Goo => s_goo [||]?? (s_goo = new string('c', 42)); | ||
| private static string Goo => s_goo [|??|] (s_goo = new string('c', 42)); |
There was a problem hiding this comment.
switched to new test harness. the new one is stricter about the test inputs, so a lot of existing tests are updated to be correct in that regard.
| { | ||
| private static string s_goo; | ||
| private static string Goo => s_goo [||]?? s_goo = new string('c', 42); | ||
| private static string Goo => {|CS0131:s_goo ?? s_goo|} = new string('c', 42); |
There was a problem hiding this comment.
Why do you need to specify the diagnostic in certain cases but not others?
There was a problem hiding this comment.
because the original code is invalid C#. the test is testing that. the new test harness requires you to be explicit htat you know it's invalid code by putting in these markers. it helps prevent tests that are accidentally testing bogus code unintentionally.
| { | ||
| // Before | ||
| o ??= new C(); // After | ||
| } |
| ? assignmentTemp | ||
| : null; | ||
|
|
||
| return whenTrue != null && assignment != null; |
There was a problem hiding this comment.
Why not just return assignment != null?
There was a problem hiding this comment.
i liked the clarity :)
| // System.String. Even though `==` is overloaded, it has the same semantics as ReferenceEquals(null) so | ||
| // it's safe to convert. | ||
| var symbol = semanticModel.GetSymbolInfo(binaryExpression, cancellationToken).Symbol; | ||
| if (symbol is null || !symbol.IsUserDefinedOperator() || symbol.ContainingType.SpecialType == SpecialType.System_String) |
There was a problem hiding this comment.
null check looks unnecessary? The extension method will return false if symbol is null:
There was a problem hiding this comment.
it's not an && it's an ||.
There was a problem hiding this comment.
If the null checked is removed and symbol is null, then symbol.IsUserDefinedOperator will return false, then the ! will make it true and hence the whole expression will be true, which matches the current behavior. The null check just makes it more clear, I think?
There was a problem hiding this comment.
fair point :) i'm ok either way.
Fixes #32985
Fixes #54974