Add explicit cast to UseCoalesceExpressionForIfNullStatementCheckCodeFixProvider #74471
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
the
Community
Orachor
force-pushed
the
coalesce-explicit-cast
branch
from
c99fb82
to
2bcf6b4
Compare
Orachor
force-pushed
the
coalesce-explicit-cast
branch
from
2bcf6b4
to
59782ec
Compare
|
Hi @CyrusNajmabadi think you could take a look, please? :) |
| if (ShouldAddExplicitCast(syntaxFacts, semanticModel, expressionToCoalesce, | ||
| whenTrueStatement, out var castTo, cancellationToken)) |
There was a problem hiding this comment.
i'd have this just return the castTo type as the return value. and this can then just check that for null, and cast if not.
|
|
||
| Protected Overrides Function ShouldAddExplicitCast(syntaxFacts As ISyntaxFactsService, semanticModel As SemanticModel, expressionToCoalesce As SyntaxNode, whenTrueStatement As SyntaxNode, <NotNullWhen(True)> ByRef castTo As ITypeSymbol, cancellationToken As CancellationToken) As Boolean | ||
| Return False | ||
| End Function |
There was a problem hiding this comment.
i'm also ok with this being virtual and returning false, so that the VB side doesn't need this.
There was a problem hiding this comment.
or, if you keep this, doc why cast is not needed.
There was a problem hiding this comment.
Changed method to virtual with default returning null.
...CoalesceExpression/VisualBasicUseCoalesceExpressionForIfNullStatementCheckCodeFixProvider.vb
Outdated
Show resolved
Hide resolved
| @@ -33,10 +26,17 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context) | |||
| return Task.CompletedTask; | |||
| } | |||
|
|
|||
| protected override Task FixAllAsync( | |||
| protected abstract bool ShouldAddExplicitCast( | |||
| ISyntaxFactsService syntaxFacts, SemanticModel semanticModel, | |||
There was a problem hiding this comment.
don't need to pass syntaxfacts in. since the subclasses are already lang specific, they can directly do whatever they need.
There was a problem hiding this comment.
I changed the implementation so SyntaxFacts are not passed in, now I am wondering whether I should do the same for the SemanticModel and have the language specific stuff return just true/false for whether the cast can be added?
| { | ||
| castTo = null; | ||
|
|
||
| if (!syntaxFacts.IsSimpleAssignmentStatement(whenTrueStatement)) |
There was a problem hiding this comment.
doc why this is only for the asignment case.
There was a problem hiding this comment.
doc the basic intuition here, possible with an example.
|
@Orachor Great work! Love the tests. Some minor tweaks/suggestions, esp. about having hte code doc what's going on. Thanks! <3 |
|
Looks great. Thanks! |
This adds an explicit cast to
UseCoalesceExpressionForIfNullStatementCheckCodeFixProvider(if necessary) to make sure the code fix does not produce broken code.Whether or not the cast is added depends on the types that are being coalesce-d
As for VisualBasic from what I can tell this cast is completely unnecessary, due to the coalesce expression behaving differently, however I am not an expert so please check if the tests I added are correct.
Closes #74460