Skip to content

Add formatting analyzer test for param nullchecking#58936

Merged
RikkiGibson merged 1 commit intodotnet:mainfrom
RikkiGibson:pnc-add-formatting-test
Jan 20, 2022
Merged

Add formatting analyzer test for param nullchecking#58936
RikkiGibson merged 1 commit intodotnet:mainfrom
RikkiGibson:pnc-add-formatting-test

Conversation

@RikkiGibson
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson commented Jan 19, 2022

Closes #58798.

Adding a test to demonstrate that the formatting analyzer itself isn't at fault here. Just the fact that we are using an old version of it in Roslyn.

@RikkiGibson RikkiGibson requested a review from a team as a code owner January 19, 2022 00:17
@ghost ghost added the Area-Analyzers label Jan 19, 2022
@RikkiGibson RikkiGibson added the Test Test failures in roslyn-CI label Jan 19, 2022
Youssef1313
Youssef1313 reviewed Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test where the !! isn't properly formatted:

class MyClass
{
    void MyMethod(string s !!) // <-- Extra space should be reported and fixed
    {
    }
}

This was referenced Jan 19, 2022
Closed
Closed
@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Jan 19, 2022

We have such a test for the formatter itself. It wouldn't hurt to have it for the formatting analyzer too.

roslyn/src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

Lines 5779 to 5799 in 259a9e2

[Fact]
[Trait(Traits.Feature, Traits.Features.Formatting)]
public async Task SpacingInNullCheckedParameter()
{
var code =
@"class C
{
static object F(string s !!)
{
}
}";
var expectedCode =
@"class C
{
static object F(string s!!)
{
}
}";
await AssertFormatAsync(expectedCode, code);
}

I did find that we don't do the right thing in the following scenario:

static void M1(string x
    !!)
{
}

I expected !! to always be moved to be right up against the parameter name. It seems like the leading trivia owned by !! should be moved to the next token, for consistency with the following (which doesn't prompt any formatting errors)

static void M1(string x
    )
{
}

All that said, I'd prefer to back-burner this work and possibly make it "help wanted". The important thing I wanted to confirm with this PR is that "correctly formatted" code using the feature doesn't produce formatting warnings.

@RikkiGibson RikkiGibson mentioned this pull request Jan 19, 2022
Closed
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jan 19, 2022

I expected !! to always be moved to be right up against the parameter name.

You'll likely need to force-space to 0, and set that without the flag that says only do that when on the same lien.

@RikkiGibson RikkiGibson merged commit fc0638b into dotnet:main Jan 20, 2022
@ghost ghost added this to the Next milestone Jan 20, 2022
@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Jan 20, 2022

Thanks for the review @davidwengier!

@RikkiGibson RikkiGibson deleted the pnc-add-formatting-test branch January 20, 2022 06:05
333fred added a commit to 333fred/roslyn that referenced this pull request Jan 21, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into Issue57742
2ee1ce6 
* upstream/main: (1035 commits)
  Add missing header
  Mark IVSTypeScriptFormattingServiceImplementation as optional, but require it in the constructor
  Fix Go To Base for a symbol with a single metadata location (dotnet#58965)
  [EnC] Store entire spans instead of line deltas (dotnet#58831)
  Delete CodeAnalysisRules.ruleset (dotnet#58968)
  Allow xml docs to still be created when 'emit metadata only' is on. (dotnet#57667)
  Fix ParseVBErrorOrWarning (dotnet#47833)
  Update parameter nullability to match implementation
  Ensure CSharpUseParameterNullCheckingDiagnosticAnalyzer works with nullable parameters
  Add tests for issues fixed by previous PR (dotnet#58764)
  Update src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs
  Disallow null checking discard parameters (dotnet#58952)
  Add extension method
  Escape type arguments
  Few fixes
  Update tests.
  Add Analyzers layer to CODEOWNERS
  Add formatting analyzer test for param nullchecking (dotnet#58936)
  Move reading HideAdvancedMembers option up (dotnet#58747)
  List patterns: Slice value is assumed to be never null (dotnet#57457)
  ...
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidwengier davidwengier davidwengier approved these changes

+1 more reviewer

@Youssef1313 Youssef1313 Youssef1313 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Analyzers Test Test failures in roslyn-CI

Projects

None yet

Milestone

17.2.P1

Development

Successfully merging this pull request may close these issues.

Param-nullchecking doesn't work properly with formatting analyzer

4 participants

@RikkiGibson @CyrusNajmabadi @davidwengier @Youssef1313