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 UseExceptionThrowHelpers analyzer/fixer #6293

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

stephentoub
Copy link
Member

Flags patterns involving guarded construction of:

  • ArgumentNullException
  • ArgumentException
  • ArgumentOutOfRangeException
  • ObjectDisposedException

that can be replaced with:

  • ArgumentNullException.ThrowIfNull
  • ArgumentException.ThrowIfNullOrEmpty
  • ArgumentOutOfRangeException.ThrowIfZero
  • ArgumentOutOfRangeException.ThrowIfNegative
  • ArgumentOutOfRangeException.ThrowIfNegativeOrZero
  • ArgumentOutOfRangeException.ThrowIfGreaterThan
  • ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual
  • ArgumentOutOfRangeException.ThrowIfLessThan
  • ArgumentOutOfRangeException.ThrowIfLessThanOrEqual
  • ObjectDisposedException.ThrowIf

For now I have a different diagnostic ID per exception type, but we can either increase or decrease the number (e.g. one all-up, one per method, etc.) We can add additional throw helpers to the analyzer/fixer as we add more. The Argument{Null}Exception and ObjectDisposedException helpers shipped in previous .NET releases; the ArgumentOutOfRangeException helpers are being added in dotnet/runtime#78222 from @hrrrrustic.

I've also trended towards preferring false negatives over false positives, so for example if the analyzer detects any "meaningful" additional arguments (e.g. an inner exception) being passed in, it doesn't warn.

There's also a fixer included. It's capable of fixing all flagged call sites with the exception of some ObjectDisposedException uses. It'll fix new ObjectDisposedException(GetType().Name) for example, but it won't fix new ObjectDisposedException("something") as it won't know what Type/object to pass in as the thing being disposed of.

Fixes dotnet/runtime#68326
cc: @buyaa-n, @mavasani, @Youssef1313 for review
cc: @bartonjs, @terrajobst in case you have feedback/opinions on diagnostic ID usage

Flags patterns involving guarded construction of:
- ArgumentNullException
- ArgumentException
- ArgumentOutOfRangeException
- ObjectDisposedException

that can be replaced with:
- ArgumentNullException.ThrowIfNull
- ArgumentException.ThrowIfNullOrEmpty
- ArgumentOutOfRangeException.ThrowIfZero
- ArgumentOutOfRangeException.ThrowIfNegative
- ArgumentOutOfRangeException.ThrowIfNegativeOrZero
- ArgumentOutOfRangeException.ThrowIfGreaterThan
- ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual
- ArgumentOutOfRangeException.ThrowIfLessThan
- ArgumentOutOfRangeException.ThrowIfLessThanOrEqual
- ObjectDisposedException.ThrowIf

For now I have a different diagnostic ID per exception type, but we can either increase or decrease the number (e.g. one all-up, one per method, etc.)

I've also trended towards preferring false negatives over false positives, so for example if the analyzer detects any "meaningful" additional arguments (e.g. an inner exception) being passed in, it doesn't warn.

There's also a fixer included.  It's capable of fixing all flagged call sites with the exception of some ObjectDisposedException uses.  It'll fix `new ObjectDisposedException(GetType().Name)` for example, but it won't fix `new ObjectDisposedException("something")` as it won't know what Type/object to pass in as the thing being disposed of.
@stephentoub stephentoub requested a review from a team as a code owner November 29, 2022 22:40
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.

LGTM

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #6293 (26295b4) into main (9b58ec3) will increase coverage by 0.01%.
The diff coverage is 98.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6293      +/-   ##
==========================================
+ Coverage   96.06%   96.07%   +0.01%     
==========================================
  Files        1364     1367       +3     
  Lines      313717   315337    +1620     
  Branches    10125    10187      +62     
==========================================
+ Hits       301357   302961    +1604     
- Misses       9939     9943       +4     
- Partials     2421     2433      +12     

- Added a unique title per diagnostic ID
- Changed category to Maintainability rather than Performance.  They're a bit of both, but more on the former.  Thus also renumbered everything.
- Better handle guards with else blocks.
- Avoid flagging nullables and enums for ArgumentOutOfRangeException
Copy link
Contributor

@buyaa-n buyaa-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, thanks, 👍 for the code comments

@stephentoub
Copy link
Member Author

Thanks for the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Analyzer Proposal]: Convert argument null checks to ArgumentNullException.ThrowIfNull
5 participants