Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add formatting analyzer test for param nullchecking #58936

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

RikkiGibson
Copy link
Contributor

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
@RikkiGibson RikkiGibson added the Test Test failures in roslyn-CI label Jan 19, 2022
Copy link
Member

@Youssef1313 Youssef1313 left a comment

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
    {
    }
}

@RikkiGibson
Copy link
Contributor Author

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

[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.

@CyrusNajmabadi
Copy link
Member

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
Contributor Author

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
* 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
Labels
Area-Analyzers Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Param-nullchecking doesn't work properly with formatting analyzer
4 participants