Should ArgumentNullThrowHelper.ThrowIfNull be used instead of ArgumentNullException within AuthenticationState?

[aligoren]

Hello, I hope the topic I mentioned is open in the right place.

In this file, last updated 2 years ago:

https://github.com/dotnet/aspnetcore/blob/main/src/Components/Authorization/src/AuthenticationState.cs#L19, there is the following statement:

User = user ?? throw new ArgumentNullException(nameof(user));

Can we use this instead?

ArgumentNullThrowHelper.ThrowIfNull(user);
User = user;

If this would be a good first contribution, I would like to send my very first MR :P

I hope I haven't disturbed here unnecessarily and for an irrelevant matter.

Best regards

[danmoseley]

Hi @aligoren you are in the right place and are welcome here. Many people contribute from all over the world and we would be happy for you to as well.

Yes we are interested in such changes. @divyeshio began a change but was not able to complete it : #50666

You are welcome to offer a PR. My suggestion is to start small such as a PR for just one assembly. Small PRs are much easier to review and merge quickly. You could start with part of the above PR.

[danmoseley]

@BrennanConroy any reason to believe that this change will need perf measurements? It should make the hot path shorter. Unless there is some odd interaction with inlining heuristics in some particular case.

[aligoren]

Hi @danmoseley I have opened a PR. Thank you very much for everything.

#51732

[BrennanConroy]

I don't think this needs any specific measurements. Like you said, this should make the 'hot path' shorter due to optimizations made when throw isn't in the main method body. But I don't think constructing AuthenticationState is on any hot path we're paying attention to right now 😄

[danmoseley]

Yes, haha, I was wondering more about the general case and what threshold we use for running TE measurements for example.

For cold paths it is arguably still a readability improvement.

[aligoren]

Hi @aligoren you are in the right place and are welcome here. Many people contribute from all over the world and we would be happy for you to as well.

Yes we are interested in such changes. @divyeshio began a change but was not able to complete it : #50666

You are welcome to offer a PR. My suggestion is to start small such as a PR for just one assembly. Small PRs are much easier to review and merge quickly. You could start with part of the above PR.

Hey! Thank you so much for being kind to me. It really made me happy, and I'm sure it can encourage many others who might be hesitant just like me.

If I understood correctly, you suggested that I continue with the merge request mentioned above.

I should continue with this issue again, shouldn't I?

#50665

[danmoseley]

Yes please (but cc'ing @divyeshio just so they're aware in case they are planning to start again)

As mentioned I suggest making a relatively small change and getting that merged first.