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 ArgumentNullException.ThrowIfNull #55594

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Conversation

stephentoub
Copy link
Member

Fixes #48573

We will wait to roll this out broadly across dotnet/runtime until the !! feature from C# is available. C# may be able to target this method now or in the future now that it exists, and we'll use it explicitly in places where we can't rely on the C# feature, e.g. where we need to do the checks in a different order from what the compiler would emit (but the 90% case should end up using the C# feature).

In this PR I've only replaced call sites that had their own ThrowIfNull/NotNull already being used.

cc: @jcouv, @bartonjs

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jcouv
Copy link
Member

jcouv commented Jul 14, 2021

@stephentoub Sorry if I'm missing some context. I'm confused by "We will wait to roll this out broadly across dotnet/runtime until the !! feature from C# is available. C# may be able to target this method now or in the future now that it exists [...]".

The notes I had from email thread were:

  • Roslyn injects an internal ThrowIfNull(object argument, string argumentName) helper into every assembly that uses the feature, similar to how it injects helpers into assemblies for other purposes, like switching on strings (SharpLab) or use of nullability annotations (SharpLab)
  • We are free to liberally use !! throughout dotnet/runtime to replace places we currently do if (arg == null) throw new ArgumentNullException(nameof(arg)); for argument checking.
  • We can choose to also introduce a public ThrowIfNull helper for devs to use directly, but it’s unrelated to !!.

So the compiler will not be targeting this method.
I would have expected dotnet/runtime to use this method before the parameter!! feature from C# becomes available, not after.
Tagging @jaredpar since he's the champion for parameter!!.

@stephentoub
Copy link
Member Author

stephentoub commented Jul 14, 2021

@jcouv, sorry for the confusion. What I meant was, I'm not going to first replace all of the thousands of throw sites with a manual call to ThrowIfNull and then again change all of them to use !!; that's way too much churn. Instead, we won't call ThrowIfNull for them, and we'll just use the !! support when it's available. If in the future !! targets this method, great, but the expectation is that for the foreseeable future it won't (doing so could harm R2R code due to lack of cross-module inlining, so we likely wouldn't want to consider having the compiler target this public ThrowIfNull until that's addressed in the future). Even with !!, though, we expect there will still be places we'll use ThrowIfNull directly, but it'll be easier to see where those places should be after we've rolled out use of !! (or as part of doing so). For example, places where we do a null check on some argument but we do so after we check and throw for other conditions, we might still want to explicitly use ThrowIfNull to maintain ordering / compat.

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

Successfully merging this pull request may close these issues.

Introduce static methods to allocate and throw key exception types.
6 participants