Adding Null Checks For All Parameters

[stcahlon]

Work in progress

#20986

[dnfclas]

All CLA requirements met.

[CyrusNajmabadi]

Please include me when this is ready to move getting eyes. Thanks.

@CyrusNajmabadi Hi. This would be a good time. Thanks!

[genlu]

namespace Microsoft.CodeAnalysis.GenerateMember.GenerateVariable

No actual change in this file, please revert to exclude it from commit.

---

Refers to: src/Features/Core/Portable/GenerateMember/GenerateVariable/AbstractGenerateVariableService.GenerateParameterCodeAction.cs:8 in 51cf39c. [](commit_id = 51cf39c, deletion_comment = False)

[genlu]

    protected abstract Task<ImmutableArray<CodeAction>> GetRefactoringsAsync(

Consider renaming this to something like GetRefactoringsForSingleParameterAsync, to make it purpose clearer.

---

Refers to: src/Features/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs:40 in 51cf39c. [](commit_id = 51cf39c, deletion_comment = False)

[genlu]

Some of the checks on selection to decide weather to offer refactoring do not apply to "add for all parameter" option. For example, do we want to offer "add for all" when selection exactly selects a parameter name? We need to figure out exactly when to offer this new refactoring.

[genlu]

Also, these available checks don't determine whether a parameter is "valid" for refactoring, I believe we have logic in ParameterValidForNullCheck that does that, right? Seems to me that this loop is not needed, we can just pass entire parameter list to AddParameterCheck provider instead.

---

In reply to: 300806544 [](ancestors = 300806544)

[stcahlon]

True. but some checks are also made here. For example It checks the method symbol of each parameter, and whether it or its name are null. They don't determine whether a parameter is "valid" for refactoring - but they help determine which parameters are NOT valid - and those ones don't make it into the new list (which is now renamed to avoid confusion into "listOfPotentiallyValidParametersNodes". Let me know what you think - I could be wrong.

[petrroll]

I'm currently working on making refactorings offerings more consistent #35525 and:

1. We have a set of helpers living in IRefactoringHelpersService that you can ask "here's my selection/location and I'm interested in this type (ParameterSyntax / ParameterListSyntax in your case), give me the SyntaxNode if refactoring for it should be offered / null if not". Example usage can be seen here:

   roslyn/src/Features/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs

   Line 50 in 6dda102

   var parameterNode = await refactoringHelperService.TryGetSelectedNodeAsync<TParameterSyntax>(document, context.Span, cancellationToken).ConfigureAwait(false);

   The AbstractInitializeParameterCodeRefactoring has actually been moved to that helper (not sure why it doesn't show in diffs, it's already merged in master). It would be great if we could work together so that it keeps using the helper (if there're any unsatisfied requirements we can extend it) and doesn't regress to custom handling of context.Span.

2. Generally our guidelines that would be applicable we arrived to (happy to revise them) were:

   • If user hasn't selected anything and is anywhere inside the header (in any argument) -> show Add check for all parameters (we consider the caret being in a header of ArgumentList).
   • If user selected less then one/one parameter -> show Add check only for that parameter (they selected -> they want that one specifically)
   • If user selected the whole parameter list -> show Add check to all (obvious)
   • Also, since argument initializers can be only primitive values, we came to a conclusion that the initializer should be part of the header (and thus applicable caret location to invoke the refactoring).

[petrroll]

3. As for concrete implementation, I'm slightly confused (because the diff here doesn't seem to be up to date) but I think it could look something like this:

var refactoringHelperService = document.GetLanguageService<IRefactoringHelpersService>();
var parameterNode = await refactoringHelperService.TryGetSelectedNodeAsync<TParameterSyntax>(document, context.Span, cancellationToken).ConfigureAwait(false);

// A concrete parameter node can be identified & it was a selection of one parameter (-> intentional)
if (parameterNode != null && !context.Span.IsEmpty)
{
   // Initialize just for that one parameterNode
}

var parameterListNode = parameterNode?.Parent ?? await refactoringHelperService.TryGetSelectedNodeAsync<TParameterListSyntax>(document, context.Span, cancellationToken).ConfigureAwait(false);

if (parameterListNode == null)
{
    return;
}

// Offer initialize (add null checks) for all parameters in parameterListNode 

[genlu]

LGTM. Good job!

[CyrusNajmabadi]

please don't do this. we don't need to loop N times over making N documents and having to go re-find parameters after making edits. this is a very very brittle way to do things and should def be avoided.

[genlu]

Will try to address this in a follow-up PR

[CyrusNajmabadi]

still don't understand this. this should be an abstract method.

[stcahlon]

It is abstract in the base class

[CyrusNajmabadi]

do you need this helper? this should all fall out from using TryGetRelevantNodeAsync<TParameterSyntax>() right?

[stcahlon]

As long as we update the document in the loop we will need this (otherwise we'll get another syntax tree). This can be replaced once document is only updated at the end.

[genlu]

Will try to address this in a follow-up PR

[CyrusNajmabadi]

So, i would not have a problem with this going into a feature branch for now. But there's a pretty core concern with that inner loop that i'm def worried about. Note: if it helps, i can contirbute the fixes there, but it would have to happen after monday.

[CyrusNajmabadi]

Good job!

Indeed! This is really nice, and I think this will be great for customers. I think this coudl go in, but i would recommend some followup PRs.

[stcahlon]

@CyrusNajmabadi Addressed your comments - thanks!
regarding the inner loop - it would be very helpful if you can get to it as I'll be away for a while after today.

[CyrusNajmabadi]

Will be happy to help next week! This is really great already, is just like to tweak one thing :-)

…

On Thu, Aug 15, 2019, 12:35 PM S. T. Zach-Cahlon ***@***.***> wrote: @CyrusNajmabadi <https://github.com/CyrusNajmabadi> Addressed your comments - thanks! regarding the inner loop - it would be very helpful if you can get to it as I'll be away for a while after today. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36931?email_source=notifications&email_token=ABC2MY3XILG5OZRPFE5TZMDQEWVY3A5CNFSM4H46QHYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4MY5FQ#issuecomment-521768598>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABC2MY52EDAIEJW7HLSJPG3QEWVY3ANCNFSM4H46QHYA> .

[genlu]

I merged this into the wrong branch. Should have targeted 16.4. Will revert and retarget