Skip to content

Include type name in Assumes.Null/NotNull failures#216

Merged
AArnott merged 2 commits intomicrosoft:mainfrom
drewnoakes:assumes-null-failure-message
Oct 10, 2023
Merged

Include type name in Assumes.Null/NotNull failures#216
AArnott merged 2 commits intomicrosoft:mainfrom
drewnoakes:assumes-null-failure-message

Conversation

@drewnoakes
Copy link
Member

Recently I've seen a few errors from customers with error text "value is null" (the caller's expression), which isn't particularly helpful. This change attempts to include the name of the type in the message, which will help with diagnosis in many cases. With this change the message would read something like "Unexpected null value of type 'String'.".

I've attempted to preserve the inlineability and tiny code size of the original methods by moving the formatting into a separate method. That method is generic (to avoid the caller having code to produce a type name) which could increase generated code size, however I wouldn't expect that to be a huge concern. It's worth calling out for review though.

Recently I've seen a few errors from customers with error text "value is null" (the caller's expression), which isn't particularly helpful. This change attempts to include the name of the type in the message, which will help with diagnosis in many cases. With this change the message would read something like "Unexpected null value of type 'String'.".

I've attempted to preserve the inlineability and tiny code size of the original methods by moving the formatting into a separate method. That method is generic (to avoid the caller having code to produce a type name) which could increase generated code size, however I wouldn't expect that to be a huge concern. It's worth calling out for review though.
@drewnoakes
Copy link
Member Author

A motivating example for this change is https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1880988

Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

I like it. Just a test change please.

This ensures tests run correctly, regardless of the environment's culture.
Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks.

@AArnott AArnott merged commit 5272898 into microsoft:main Oct 10, 2023
@drewnoakes drewnoakes deleted the assumes-null-failure-message branch October 12, 2023 01:26
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