Add DeclareAsNullable refactoring#26661
Add DeclareAsNullable refactoring#26661jcouv wants to merge 1 commit intodotnet:features/NullableReferenceTypesfrom
Conversation
| } | ||
|
|
||
| [Fact] | ||
| public async Task TestAlreadyNullableRefType() |
There was a problem hiding this comment.
nit: "ref" sounds misleading (I thought of RefTypeSyntax), consider using ReferenceType #Pending
| static string M2() => throw null; | ||
| }"; | ||
|
|
||
| await TestMissingInRegularAndScriptAsync(code, parameters: s_nullableFeature); |
There was a problem hiding this comment.
consider supporting this by splitting the declaration #WontFix
| return; | ||
| } | ||
| } | ||
| static T M2<T>() => throw null; |
There was a problem hiding this comment.
question: is T? not supported? #ByDesign
| await TestMissingInRegularAndScriptAsync(code, parameters: s_nullableFeature); | ||
| } | ||
|
|
||
| private Task TestMissingInRegularAndScriptAsync(string initialMarkup, IDictionary<OptionKey, object> options) |
There was a problem hiding this comment.
it looks like this isn't used anywhere #Pending
| <data name="Use_explicit_type_instead_of_var" xml:space="preserve"> | ||
| <value>Use explicit type instead of 'var'</value> | ||
| </data> | ||
| <data name="Declare as nullable" xml:space="preserve"> |
There was a problem hiding this comment.
missing underscores #Pending
| { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
consider supporting is null and possibly even (object)x == null and ReferenceEquals #Pending
| var declaration = (VariableDeclarationSyntax)declarator.Parent; | ||
| return declaration.Variables.Count == 1 ? declaration.Type : null; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
what about things like a foreach block that doesn't have a VarDecl? #ByDesign
There was a problem hiding this comment.
I don't see a good scenario in a foreach where a null test would indicate that the iteration variable should be declared as nullable.
In reply to: 186297577 [](ancestors = 186297577)
| return property.Type; | ||
|
|
||
| case MethodDeclarationSyntax method: | ||
| if (method.Modifiers.Any(SyntaxKind.PartialKeyword)) |
| { | ||
| return null; | ||
| } | ||
| return method.ReturnType; |
There was a problem hiding this comment.
blank line after } #Pending
| return null; | ||
| } | ||
|
|
||
| private async Task<ISymbol> TryGetSymbolToFix(SyntaxNode root, TextSpan textSpan, Document document, CancellationToken cancellationToken) |
There was a problem hiding this comment.
returns Task, add 'Async' suffix. #Pending
There was a problem hiding this comment.
order of args is a bit off from normal patterns. usually we go 'wider to narrower'. so Document/Root/Span. #Pending
| return null; | ||
| } | ||
|
|
||
| private async Task<ISymbol> TryGetSymbolToFix(SyntaxNode root, TextSpan textSpan, Document document, CancellationToken cancellationToken) |
| } | ||
| else if (token.Parent.IsKind(SyntaxKind.NullLiteralExpression) && token.Parent.IsParentKind(SyntaxKind.EqualsExpression, SyntaxKind.NotEqualsExpression)) | ||
| { | ||
| equals = (BinaryExpressionSyntax)token.Parent.Parent; |
There was a problem hiding this comment.
this could be simplified by first checking for null, and walking to the parent. Then just checking == and != once.
| type = method.ReturnType; | ||
| break; | ||
| case IFieldSymbol field: | ||
| type = field.Type; |
There was a problem hiding this comment.
nit: consider extracting helper to get the type, if such a helper doesn't already exist somewhere #WontFix
| private static async Task<Document> UpdateDocumentAsync(Document document, TypeSyntax typeToFix, CancellationToken cancellationToken) | ||
| { | ||
| var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
| var editor = new SyntaxEditor(root, document.Project.Solution.Workspace); |
There was a problem hiding this comment.
an editor is somewhat overkill here since you ca njust do root.ReplaceNode. Editors are good when you have a lot of work to do. #Pending
| public const string CodeActionsUseExpressionBody = "CodeActions.UseExpressionBody"; | ||
| public const string CodeActionsUseImplicitType = "CodeActions.UseImplicitType"; | ||
| public const string CodeActionsUseExplicitType = "CodeActions.UseExplicitType"; | ||
| public const string CodeActionsDeclaredAsNullable = "CodeActions.DeclaredAsNullable"; |
There was a problem hiding this comment.
inconsistency. "Declared" here, but "Declare" elsewhere. #Pending
|
|
||
| private static async Task<Document> UpdateDocumentAsync(Document document, TypeSyntax typeToFix, CancellationToken cancellationToken) | ||
| { | ||
| var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
this is already done inside ComputeRefactoringsAsync #ByDesign
| private static async Task<Document> UpdateDocumentAsync(Document document, TypeSyntax typeToFix, CancellationToken cancellationToken) | ||
| { | ||
| var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
| var editor = new SyntaxEditor(root, document.Project.Solution.Workspace); |
There was a problem hiding this comment.
is it worth it to use the syntax editor for just 1 change? #Pending
|
From discussion with Chuck, I will switch this to be an analyzer/fixer. We'd offer a suggestion on |
|
How about a fix-all, but with an actual model dialog warning the user needs to confirm? #ByDesign |
|
@CyrusNajmabadi Are you suggesting that the user would have to individually confirm every instance that gets fixed? As a side note, I also wanted to ask: |
No. There would be a single notification. #Resolved |
| /// For example: `nonNull == null`, `nonNull?.Property` | ||
| /// </summary> | ||
| [ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = PredefinedCodeRefactoringProviderNames.DeclareAsNullable), Shared] | ||
| internal class DeclareAsNullableCodeRefactoringProvider : CodeRefactoringProvider |
There was a problem hiding this comment.
Why a refactoring? I think fix-all is most preferable for this. I'd expect such analyzer look for all null literals and null checks (including ?., ??, ??=, etc) and offer to change the target type. Now, the said type could come from anywhere like a local declaration (if not var), parameter, return type, field, property, class base, or a type argument. The fix-all could be a little more involved since we probably should infer nullability when you pass a value to a nullable parameter (need to guard for recursions). I think it'd make more sense to get this info from the compiler (#30110?) #Closed
There was a problem hiding this comment.
Checking for null is a weak indication that a variable might allow null. I expect many APIs to declare non-nullable parameters, but check those parameters.
it may be possible to distinguish two cases: if null then throw, and, if null do something else.
The first scenario should most likely leave the parameter as non-nullable. #Closed
|
Any thoughts on allowing this to also (optionally?) modify an interface implementation? Otherwise this will trigger a further warning: interface INulls
{
public void M(string s);
}class Nulls : INulls
{
public void M(string? s) // Signature after applying code fix
{
if (s == null) Console.WriteLine("Oh boy it's null");
}
}Which you could argue is a good thing - perhaps seeing that fixing the warning involves a contract change makes you consider how not to introduce a |
The purpose for this refactoring is to assist in migrating a project to use the nullable feature.
When we see a comparison of a non-nullable with
null(such asnonNullable == nullornonNullable != null), there is a good chance thatnonNullableshould in fact be declared as nullable instead. This refactoring helps do that with one click.Note this is only a good chance, because such null checks could also be defensive programming, especially when it comes to public APIs (you declare them as non-nullable, but check anyways).
From discussion with Chuck, I will switch this to an analyzer/fixer so that it's more discoverable. But I won't offer a FixAll.