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 DoesNotReturnIf to Assert.IsTrue/Assert.IsFalse and DoesNotReturn to Assert.Fail #1005

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Add DoesNotReturnIf to Assert.IsTrue/Assert.IsFalse and DoesNotReturn to Assert.Fail #1005

merged 1 commit into from
Nov 2, 2021

Conversation

dferretti
Copy link
Contributor

@dferretti dferretti commented Nov 2, 2021

Fixes #1004

When adding these attributes, I did hesitate on the IsTrue/IsFalse overloads accepting nullable bools, because it does look odd to see
[DoesNotReturnIf(true)] bool? condition
right next to a check like
if (condition == true || condition == null)
knowing that for both true and null the method will not return - but only for true will the nullability analysis kick in. Passing null will result in the call site still not knowing if the method returns or not so the user will still have to perform their own additional check.

But my thought process was that the attribute is still accurate and still helps. [DoesNotReturnIf(true)] means just that - the method does not return if the parameter exactly matches true. It does not say whether or not the method returns if false or null are passed in.
Even though in this case we know that IsFalse(null) will not return, there is not a way to represent that with the current DoesNotReturnIf attribute.

@ghost
Copy link

ghost commented Nov 2, 2021

CLA assistant check
All CLA requirements met.

@dferretti
Copy link
Contributor Author

cc @Haplois

@Haplois
Copy link
Contributor

Haplois commented Nov 2, 2021

Thank you for the contribution @dferretti, merging!

@Haplois Haplois merged commit 4173747 into microsoft:main Nov 2, 2021
@dferretti dferretti deleted the additional-null-aware branch November 2, 2021 21:46
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.

Additional Nullable Reference Type-aware support
2 participants