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

Use null coalescing in many more places #71361

Merged
merged 3 commits into from
Jun 28, 2022

Conversation

stephentoub
Copy link
Member

I previously enabled all of the IDExx rules around null coalescing, but that still left a large number of manual checks in the codebase, so here I used regexes to find and fix up many more occurences. @mavasani, is there an analyzer/fixer for this, or if not, have we considered one? For the most part, this is style, cleaning up around 5K lines, but a non-trivial number do result in more efficient accesses.

@ghost
Copy link

ghost commented Jun 28, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

I previously enabled all of the IDExx rules around null coalescing, but that still left a large number of manual checks in the codebase, so here I used regexes to find and fix up many more occurences. @mavasani, is there an analyzer/fixer for this, or if not, have we considered one? For the most part, this is style, cleaning up around 5K lines, but a non-trivial number do result in more efficient accesses.

Author: stephentoub
Assignees: stephentoub
Labels:

area-Meta

Milestone: -

@mavasani
Copy link

mavasani commented Jun 28, 2022

@stephentoub We don't support such an analyzer/code fix/refactoring, but we do have a tracking issue for this feature request: dotnet/roslyn#32985.

@CyrusNajmabadi @mikadumont do you know if we have received requests for this feature in the past?

@CyrusNajmabadi
Copy link
Member

The only reason we didn't do this was because we weren't certain the semantics were identical, and we were worried about power. E.g. the code before only assigns in the case of null. The code after always assigns.

If ST is ok with this though, then that's a strong indication this is fine and we should just do this :-)

@stephentoub
Copy link
Member Author

The code after always assigns.

Wait, what?

_obj ??= new object();

is not the same as:

_obj = _obj ?? new object();

I'd object if it were the latter, but it's actually:

if (_obj is null)
{
    _obj = new object();
}

@CyrusNajmabadi
Copy link
Member

That's fascinating. I missed that in the spec:

Which follows the existing semantic rules for compound assignment operators (§11.18.3), except that we elide the assignment if the left-hand side is non-null.

Thanks for making me aware of that. We'll def add this now!

@CyrusNajmabadi
Copy link
Member

Updated analyzer/fixer here: dotnet/roslyn#62191

@stephentoub stephentoub merged commit e55c908 into dotnet:main Jun 28, 2022
@stephentoub stephentoub deleted the morecoalesce branch June 28, 2022 18:47
modifier_spec = new List<IModifierSpec>();
modifier_spec.Add(md);
}
private void AddModifier(IModifierSpec md) => modifier_spec ??= new List<IModifierSpec>();
Copy link
Contributor

Choose a reason for hiding this comment

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

@stephentoub was it intentional that you also removed the Add call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely not. Sorry. I'll fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants