Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add DoNotPassStructToArgumentNullExceptionThrowIfNullAnalyzer#6815

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
buyaa-n merged 11 commits intodotnet:mainfromCollinAlpert:issue_85154
Feb 23, 2024

Conversation

@CollinAlpert
Copy link

# Conflicts:#src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx#src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt
@CollinAlpertCollinAlpert requested a review froma team as acode ownerJuly 28, 2023 13:11
@codecov
Copy link

codecovbot commentedJul 28, 2023•
edited
Loading

Codecov Report

Attention:16 lines in your changes are missing coverage. Please review.

Comparison is base(ab13ac7) 96.45% compared to head(b3eebbb) 96.46%.

Additional details and impacted files
@@           Coverage Diff            @@##             main    #6815    +/-   ##========================================  Coverage   96.45%   96.46%            ========================================  Files        1422     1427     +5       Lines      340585   341582   +997       Branches    11230    11246    +16     ========================================+ Hits       328511   329503   +992+ Misses       9236     9234     -2- Partials     2838     2845     +7

@stephentoub
Copy link
Member

Thanks,@CollinAlpert.

I suggest we broaden this slightly. The reason structs don't make sense here is because they'll be boxed to objects and then will never be null, making the whole operation an expensive nop. That same reasoning applies to other misuse, as well, for example accidentally passing anameof(...) to ThrowIfNull (dotnet/runtime#89751 (comment)), or passingnew Class(). Basically, make this analyzer about catching non-sensical uses of ThrowIfNull where the argument is provably never null by the compiler. (It shouldnot rely on nullable annotations.) So, boxing structs, nameof,new Class(...), and anything else we can think of that's provably never null.

vartypeProvider=WellKnownTypeProvider.GetOrCreate(context.Compilation);
varthrowIfNullMethod=typeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemArgumentNullException)
?.GetMembers("ThrowIfNull")
.FirstOrDefault(m=>misIMethodSymbolmethod&&method.Parameters[0].Type.SpecialType==SpecialType.System_Object);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.FirstOrDefault(m=>misIMethodSymbolmethod&&method.Parameters[0].Type.SpecialType==SpecialType.System_Object);
.FirstOrDefault(m=>misIMethodSymbolmethod&&method.Parameters.Length>0&&method.Parameters[0].Type.SpecialType==SpecialType.System_Object);

Copy link

@sharwellsharwellAug 3, 2023•
edited
Loading

Choose a reason for hiding this comment

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

You could go further if you want:

m is IMethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_Object }, ..] }

# Conflicts:#src/NetAnalyzers/RulesMissingDocumentation.md
@CollinAlpert
Copy link
Author

@stephentoub@Youssef1313 Thank you for your feedback, I have addressed it and added appropriate tests.

@mpidash
Copy link

Seems likeCA1869 (andCA1870) got used so this analyzer has to move toCA1871.

<targetstate="new">Do not call 'ArgumentNullException.ThrowIfNull' with a struct</target>
<note />
</trans-unit>
<trans-unitid="DoNotPassNullableStructToArgumentNullExceptionThrowIfNullDescription">

Choose a reason for hiding this comment

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

❓ Do we really need this? SharpLab suggests that release builds will not box a non-null value in this case.

Copy link
Author

Choose a reason for hiding this comment

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

Can you share an example? I can see thebox instructionhere.

# Conflicts:#src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md#src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx#src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md#src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt
# Conflicts:#src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md#src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx#src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md#src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif#src/NetAnalyzers/RulesMissingDocumentation.md
Copy link

@buyaa-nbuyaa-n left a comment

Choose a reason for hiding this comment

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

Sorry for delayed review@CollinAlpert, left some comments/questions, mostly NITs. Overall the PR looks great, thank you!

Testing with runtime got1 valid hit, the analyzer/fixer works well in functional testing.

# Conflicts:#src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md#src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md#src/NetAnalyzers/RulesMissingDocumentation.md
Copy link

@buyaa-nbuyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thank you@CollinAlpert!

@buyaa-nbuyaa-n merged commitba8b7f2 intodotnet:mainFeb 23, 2024
@CollinAlpertCollinAlpert deleted the issue_85154 branchFebruary 23, 2024 06:28
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

3 more reviewers

@sharwellsharwellsharwell left review comments

@Youssef1313Youssef1313Youssef1313 left review comments

@buyaa-nbuyaa-nbuyaa-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Warning for ArgumentNullException.ThrowIfNull when passing a struct

6 participants

@CollinAlpert@stephentoub@mpidash@sharwell@Youssef1313@buyaa-n
[8]ページ先頭
©2009-2025 Movatter.jp