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

Change argument null exceptions to use ArgumentNullException.ThrowIfNull #43482

Closed
JamesNK opened this issue Aug 23, 2022 · 8 comments · Fixed by #45954
Closed

Change argument null exceptions to use ArgumentNullException.ThrowIfNull #43482

JamesNK opened this issue Aug 23, 2022 · 8 comments · Fixed by #45954
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions clean-up This issue is internal clean-up and has no effect on public APIs or expected behaviors

Comments

@JamesNK
Copy link
Member

JamesNK commented Aug 23, 2022

ASP.NET Core checks and throws for null all over the place. e.g.

if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}
if (policy == null)
{
throw new ArgumentNullException(nameof(policy));
}

ArgumentNullException.ThrowIfNull is used in modern code, but we should go through existing code to use ArgumentNullException.ThrowIfNull.

Pros: less code, small potential perf gains from inlining (exception moved into another method), consistency.

@JamesNK
Copy link
Member Author

JamesNK commented Aug 23, 2022

@stephentoub You're the author of many of the PRs in dotnet/runtime that switch code to use ArgumentNullException.ThrowIfNull. Is there a way to automate this, or did you update it manually?

@stephentoub
Copy link
Member

When !! was a thing, there was an auto-fixer in VS that would use it. I employed that to roll out !! throughout dotnet/runtime. Then !! went away, and I primarily used regexes and other simple console-based tools to rewrite those !! into ThrowIfNull calls.

@JamesNK
Copy link
Member Author

JamesNK commented Aug 24, 2022

@stephentoub Do you know what happened to the fixer when !! was removed?

A fixer that converts from if (x == null) throw new ArgumentNullException(nameof(x)) to ArgumentNullException.ThrowIfNull seems like it would still be valuable.

@stephentoub
Copy link
Member

A fixer that converts from if (x == null) throw new ArgumentNullException(nameof(x)) to ArgumentNullException.ThrowIfNull seems like it would still be valuable.

It would be. dotnet/runtime#68326 tracks it. @RikkiGibson wrote the initial analyzer/fixer and I believe was looking into revamping it for ThrowIfNull.

@JamesNK
Copy link
Member Author

JamesNK commented Aug 24, 2022

Ok!

FYI @rafikiassumani-msft @mkArtakMSFT @adityamandaleeka This has the area-runtime label, but changes apply in web frameworks and blazor areas as well.

Could delay doing anything here until an updated analyzer+fixer is ready and then change all null tests automatically.

@adityamandaleeka
Copy link
Member

You mean I wrote a sweet regex for nothing? 😄

@adityamandaleeka adityamandaleeka removed their assignment Aug 24, 2022
@adityamandaleeka adityamandaleeka added the clean-up This issue is internal clean-up and has no effect on public APIs or expected behaviors label Aug 24, 2022
@adityamandaleeka
Copy link
Member

src/Servers was done in #43502

@danmoseley
Copy link
Member

We can use this analyzer dotnet/roslyn-analyzers#6293

@ghost ghost locked as resolved and limited conversation to collaborators Feb 9, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions clean-up This issue is internal clean-up and has no effect on public APIs or expected behaviors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants