"Add null check" inserts single line statements if preferred by MaStr11 · Pull Request #52462 · dotnet/roslyn

MaStr11 · 2021-04-07T10:28:43Z

Respect csharp_prefer_braces
Respect csharp_style_allow_embedded_statements_on_same_line_experimental
string.IsNullOrEmpty is handled also
What about Support single-line null checks for 'Add null check' #52385 (comment)

…inations

MaStr11 · 2021-04-07T10:40:35Z

It seems, csharp_prefer_braces is already respected in CodeAction.PostProcessChangesAsync. Braces are removed by Simplifier.ReduceAsync here:

roslyn/src/Workspaces/Core/Portable/CodeActions/CodeAction.cs

Lines 283 to 297 in 9dad013

    
           if (document.SupportsSyntaxTree) 
        
           { 
        
               document = await ImportAdder.AddImportsFromSymbolAnnotationAsync( 
        
                   document, Simplifier.AddImportsAnnotation, cancellationToken: cancellationToken).ConfigureAwait(false); 
        
               document = await Simplifier.ReduceAsync(document, Simplifier.Annotation, cancellationToken: cancellationToken).ConfigureAwait(false); 
        
               // format any node with explicit formatter annotation 
        
               document = await Formatter.FormatAsync(document, Formatter.Annotation, cancellationToken: cancellationToken).ConfigureAwait(false); 
        
               // format any elastic whitespace 
        
               document = await Formatter.FormatAsync(document, SyntaxAnnotation.ElasticAnnotation, cancellationToken: cancellationToken).ConfigureAwait(false); 
        
               document = await CaseCorrector.CaseCorrectAsync(document, CaseCorrector.Annotation, cancellationToken).ConfigureAwait(false); 
        
           }

Afterwards the document looks like this (syntaxRoot.ToFullString()):

using System;

class C
{
    public C(object o)
    {
if(oisnull)thrownew ArgumentNullException(nameof(o));    }
}

The next change happens in Formatter.FormatAsync(SyntaxAnnotation.ElasticAnnotation) afterwards it looks like this:

using System;

class C
{
    public C(object o)
    {
        if (o is null)
            throw new ArgumentNullException(nameof(o));
    }
}

It seems csharp_style_allow_embedded_statements_on_same_line_experimental is not respected by Formatter.FormatAsync(document, SyntaxAnnotation.ElasticAnnotation).

The propable cause is the IndentBlockFormattingRule.AddEmbeddedStatementsIndentationOperation

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

Lines 254 to 261 in 9dad013

    
           private static void AddEmbeddedStatementsIndentationOperation(List<IndentBlockOperation> list, SyntaxNode node) 
        
           { 
        
               // increase indentation - embedded statement cases 
        
               if (node is IfStatementSyntax ifStatement && ifStatement.Statement != null && !(ifStatement.Statement is BlockSyntax)) 
        
               { 
        
                   AddEmbeddedStatementsIndentationOperation(list, ifStatement.Statement); 
        
                   return; 
        
               }

It does not respect csharp_style_allow_embedded_statements_on_same_line_experimental. This option is also not available in IndentBlockFormattingRule.CachedOptions

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

Lines 333 to 340 in 9dad013

    
           private readonly struct CachedOptions : IEquatable<CachedOptions> 
        
           { 
        
               public readonly LabelPositionOptions LabelPositioning; 
        
               public readonly bool IndentBlock; 
        
               public readonly bool IndentSwitchCaseSection; 
        
               public readonly bool IndentSwitchCaseSectionWhenBlock; 
        
               public readonly bool IndentSwitchSection;

@CyrusNajmabadi Am I on the right track here. Should IndentBlockFormattingRule be changed to respect csharp_style_allow_embedded_statements_on_same_line_experimental?

CyrusNajmabadi · 2021-04-07T14:31:01Z

csharp_style_allow_embedded_statements_on_same_line_experimental is not respected by Formatter.FormatAsync(document,

'Allow' does not mean 'require'. It just means it is on if the user does that.

The fix is to check for both of these and not have elastic trivia after the close paren or before the throw keyword. Thanks!

MaStr11 · 2021-04-07T15:37:20Z

'Allow' does not mean 'require'. It just means it is on if the user does that.

But shouldn't Formatter.FormatAsync(SyntaxAnnotation.ElasticAnnotation) respect csharp_style_allow_embedded_statements_on_same_line_experimental?

The fix is to check for both of these and not have elastic trivia after the close paren or before the throw keyword. Thanks!

I tried to find the SyntaxAnnotation.ElasticAnnotation on any of the descendants nodes and tokens of the generated if statement but couldn't find any. So I think the Formatter.FormatAsync(SyntaxAnnotation.ElasticAnnotation) needs to be touched, or do I miss something?

MaStr11 · 2021-04-07T15:59:19Z

Okay. I found the ElasticAnnotations in the trivia. Overall there are 49 such annotations (empty whitespace with the elastic marker). Do I really need to remove it? The problem is that the statement syntax tree is generated in a language neutral manner, so I can't influence the ElasticAnnotation placement there:

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

Lines 376 to 377 in 95bcec2

    
           private static TStatementSyntax CreateNullCheckStatement(SemanticModel semanticModel, SyntaxGenerator generator, IParameterSymbol parameter) 
        
               => (TStatementSyntax)generator.CreateNullCheckAndThrowStatement(semanticModel, parameter);

CyrusNajmabadi · 2021-04-07T16:03:07Z

But shouldn't Formatter.FormatAsync(SyntaxAnnotation.ElasticAnnotation) respect csharp_style_allow_embedded_statements_on_same_line_experimental?

No. Because the general pattern in the ecosystem is to still place them on multiple lines. This is jsut a way for clients to say if it's should be considered illegal to be on the same line. Not that it is preferred that they be on the same line. They're very different concepts.

CyrusNajmabadi · 2021-04-07T16:04:33Z

Do I really need to remove it?

Yes. i would not include the trivia between those two tokens. :) I wouldn't remove them after the fact, i woudl just not add them in the first place.

…etween ) and throw

MaStr11 · 2021-04-07T17:33:38Z

Yes. i would not include the trivia between those two tokens. :) I wouldn't remove them after the fact, i woudl just not add them in the first place.

I finally got this approach working. 0f157af shows that the close brace and the throw keyword need both trivia handling to avoid the line break/indentation. I think the way forward here is to move away from SyntaxGenerator and use abstract methods in AbstractAddParameterCheckCodeRefactoringProvider instead. Am I right, or should we add overloads for trivia handling?

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs

CyrusNajmabadi · 2021-04-07T18:24:49Z

I think the way forward here is to move away from SyntaxGenerator and use abstract methods in AbstractAddParameterCheckCodeRefactoringProvider instead. Am I right, or should we add overloads for trivia handling?

This seems reasonable.

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs

MaStr11 · 2021-04-08T10:48:29Z

@CyrusNajmabadi and @sharwell Ready for review.

sharwell

The implementation seems fine. I would like to see new tests added for the "Fix All" case before merging.

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

sharwell · 2021-04-08T15:56:07Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

                {
-                    { CSharpCodeStyleOptions.PreferBraces, new CodeStyleOption2<PreferBracesPreference>((PreferBracesPreference)preferBraces, NotificationOption2.Silent) }
+                    { CSharpCodeStyleOptions.PreferBraces, new CodeStyleOption2<PreferBracesPreference>((PreferBracesPreference)preferBraces, NotificationOption2.Silent) },
+                    { CSharpCodeStyleOptions.AllowEmbeddedStatementsOnSameLine, false },


💭 Considering this test was previously showing the behavior for a defaulted value of AllowEmbeddedStatementsOnSameLine, it might be preferable to change FixedCode instead of changing this line.

It makes me wonder if we should have used FalseWithSilentEnforcement as the default for AllowEmbeddedStatementsOnSameLine, but if that changes it can be a separate change.

Done in 7205621

sharwell · 2021-04-08T15:59:45Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
+        [WorkItem(52385, "https://github.com/dotnet/roslyn/issues/52385")]
+        public async Task SingleLineStatement_NullCheck_BracesNone_SameLineFalse()


💡 Can you also add some tests for using code action equivalence key Add_null_checks_for_all_parameters (e.g. based on TestMultiNullableObjects)

Done in 0318ce5

sharwell · 2021-04-08T16:03:03Z

...ualBasic/Portable/InitializeParameter/VisualBasicAddParameterCheckCodeRefactoringProvider.vb

        End Function
+
+        Protected Overrides Function CreateParameterCheckIfStatement(options As DocumentOptionSet, condition As ExpressionSyntax, ifTrueStatement As StatementSyntax) As StatementSyntax
+            Return SyntaxFactory.MultiLineIfBlock(


💭 Technically we could do the same thing here, but I'm not sure it's common enough to need to. I'd be fine leaving it as a follow-up, especially considering AllowEmbeddedStatementsOnSameLine would need to be moved to the shared options first.

agreed.

fwiw, we don't check AllowEmbeddedStatementsOnSameLine in vb at all. it was really intended for the C# case (and may just be a c# option currently). we have very little use of this pattern in vb.

…CheckTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

CyrusNajmabadi · 2021-04-08T17:13:47Z

...ualBasic/Portable/InitializeParameter/VisualBasicAddParameterCheckCodeRefactoringProvider.vb

+                SyntaxFactory.IfStatement(SyntaxFactory.Token(SyntaxKind.IfKeyword), condition, SyntaxFactory.Token(SyntaxKind.ThenKeyword)),
+                New SyntaxList(Of StatementSyntax)(ifTrueStatement),
+                Nothing,
+                Nothing)


nit: named parameters plz.

Done in a557a4a

CyrusNajmabadi

Thanks! @jnm2 will appreciate this a lot :)

Add tests for PreferBraces and AllowEmbeddedStatementsOnSameLine comb…

b0a9b93

…inations

MaStr11 requested a review from a team as a code owner April 7, 2021 10:28

ghost added the Area-IDE label Apr 7, 2021

WIP: Show trivia replacements needed to avoid newline and indention b…

0f157af

…etween ) and throw

MaStr11 commented Apr 7, 2021

View reviewed changes

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs Outdated Show resolved Hide resolved

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 7, 2021

jinujoseph assigned CyrusNajmabadi Apr 7, 2021

sharwell reviewed Apr 7, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 7, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 7, 2021

View reviewed changes

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 7, 2021

View reviewed changes

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs Outdated Show resolved Hide resolved

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs Outdated Show resolved Hide resolved

MaStr11 added 5 commits April 7, 2021 22:16

Add space if single line allowed and no braces.

c1e85dc

Fix TestPreferNoBlock test

9d55f67

Fix missing "Then" keyword in VB if statement

3b86a1b

Add some code comments in C# CreateParameterCheckIfStatement

3d21795

Replace cursor marker in tests

12a09a5

MaStr11 changed the title ~~"Add null check" inserts single line statements if prefferd~~ "Add null check" inserts single line statements if preferred Apr 8, 2021

MaStr11 added 2 commits April 8, 2021 09:50

Re-factor C# CreateParameterCheckIfStatement

856b0c9

Improve code comment.

2042787

sharwell suggested changes Apr 8, 2021

View reviewed changes

Change TestPreferNoBlock

7205621

MaStr11 and others added 2 commits April 8, 2021 17:41

Add SingleLineStatement_NullCheck_AllParameters

0318ce5

Update src/EditorFeatures/CSharpTest/InitializeParameter/AddParameter…

cdd735b

…CheckTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

CyrusNajmabadi reviewed Apr 8, 2021

View reviewed changes

CyrusNajmabadi approved these changes Apr 8, 2021

View reviewed changes

Use named parameters for SyntaxFactory.MultiLineIfBlock in VB

a557a4a

sharwell approved these changes Apr 8, 2021

View reviewed changes

sharwell enabled auto-merge April 8, 2021 18:57

sharwell merged commit e5fa846 into dotnet:main Apr 8, 2021

ghost added this to the Next milestone Apr 8, 2021

MaStr11 deleted the SingleLineAddNullCheck branch April 9, 2021 06:43

dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021

Conversation

MaStr11 commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaStr11 commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 7, 2021

Uh oh!

MaStr11 commented Apr 7, 2021

Uh oh!

MaStr11 commented Apr 7, 2021

Uh oh!

CyrusNajmabadi commented Apr 7, 2021

Uh oh!

CyrusNajmabadi commented Apr 7, 2021

Uh oh!

MaStr11 commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaStr11 commented Apr 8, 2021

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

MaStr11 Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

MaStr11 Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

MaStr11 Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MaStr11 commented Apr 7, 2021 •

edited

Loading

MaStr11 commented Apr 7, 2021 •

edited

Loading

MaStr11 commented Apr 7, 2021 •

edited

Loading

MaStr11 Apr 8, 2021 •

edited

Loading