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

Replace null-forgiving operator uses with debug assertions #890

Merged
merged 4 commits into from
Nov 16, 2022

Conversation

atifaziz
Copy link
Member

@atifaziz atifaziz commented Nov 15, 2022

This PR introduces Debug.Assert that's defined similarly to System.Diagnostics.Debug.Assert to help eliminate the bulk uses of the null-forgiving operator (!). The assertion more explicitly states the illegal case, and if triggered, will also relay the offending condition in the message. It also eliminates needing to suppress CS8602 and CS8603 in targets earlier than .NET 6.

This PR also adds Assume.NotNull, that's a Debug.Assert in disguise, but which specifically asserts the not-null-reference case. The idea is to make these cases easily searchable in code.

@atifaziz atifaziz self-assigned this Nov 15, 2022
@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #890 (2163141) into master (d587416) will decrease coverage by 0.14%.
The diff coverage is 72.97%.

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
- Coverage   92.53%   92.38%   -0.15%     
==========================================
  Files         108      110       +2     
  Lines        3428     3441      +13     
  Branches     1020     1020              
==========================================
+ Hits         3172     3179       +7     
- Misses        194      200       +6     
  Partials       62       62              
Impacted Files Coverage Δ
MoreLinq/FallbackIfEmpty.cs 88.67% <ø> (ø)
MoreLinq/Pad.cs 100.00% <ø> (ø)
MoreLinq/Partition.cs 98.24% <ø> (ø)
MoreLinq/Lookup.cs 61.87% <33.33%> (-2.26%) ⬇️
MoreLinq/Assume.cs 100.00% <100.00%> (ø)
MoreLinq/Debug.cs 100.00% <100.00%> (ø)
MoreLinq/Fold.cs 98.07% <100.00%> (ø)
MoreLinq/PartialSort.cs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@viceroypenguin
Copy link
Contributor

While I am not opposed to Debug.Assert(bool condition), I would suggest adding a static T AssertNotNull<T>([NotNull, AllowNull] T? object) where T : class, which receives an object, throws if null, and returns the object unchanged otherwise. The nullability analyzer will treat the returned object as not-null, which means it can be used immediately.

Example use case:

var g = Debug.AssertNotNull(_lastGrouping);

@atifaziz
Copy link
Member Author

atifaziz commented Nov 16, 2022

While I am not opposed to Debug.Assert(bool condition), I would suggest adding a static T AssertNotNull<T>([NotNull, AllowNull] T? object) where T : class, which receives an object, throws if null, and returns the object unchanged otherwise. The nullability analyzer will treat the returned object as not-null, which means it can be used immediately.

Example use case:

var g = Debug.AssertNotNull(_lastGrouping);

You can't do that because such assertion methods are conditional methods that get compiled out of release builds and therefore they must return void. See also the remarks section of the ConditionalAttribute docs:

Applying ConditionalAttribute to a method indicates to compilers that a call to the method should not be compiled into Microsoft intermediate language (MSIL) unless the conditional compilation symbol that is associated with ConditionalAttribute is defined. _You will get a compilation error in Visual Studio if you apply this attribute to a method that does not return void.

@viceroypenguin
Copy link
Contributor

Ah, I missed the [Conditional]. I'll ponder this further.

Copy link
Contributor

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

Approved for now. I have specific recommendations for different improvements for several of these, but those can be done later.

@atifaziz
Copy link
Member Author

which receives an object, throws if null, and returns the object unchanged otherwise

I've added this idea with 7e6fa33 and updated the initial description with:

This PR also adds Assume.NotNull, that's a Debug.Assert in disguise, but which specifically asserts the not-null-reference case. The idea is to make these cases easily searchable in code.

@atifaziz atifaziz merged commit b31c7fd into morelinq:master Nov 16, 2022
@atifaziz atifaziz deleted the our-debug-assert branch November 16, 2022 22:04
@viceroypenguin
Copy link
Contributor

Finished product LGTM

atifaziz added a commit to atifaziz/MoreLINQ that referenced this pull request Jan 12, 2023
This adds to commit b31c7fd,
"Replace null-forgiving operator uses with debug assertions (morelinq#890)".
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.

2 participants