Update coding guidelines to avoid using as keyword for type casting #5309
Conversation
zivkan
force-pushed
the
dev-zivkan-coding-guidelines-as-casting
branch
from
9947fd0 to
0190255
Compare
| Exceptions are allowed when multiple type names coming from different namespaces are available. | ||
| Exceptions are allowed when multiple type names coming from different namespaces are available. | ||
|
|
||
| 1. Do not use `as` to cast types |
There was a problem hiding this comment.
While I agree that objectively, as casting is probably not necessary, this almost feels like one of those things that's too opinionated. I don't mind accepting it, but that was my first instict.
There was a problem hiding this comment.
Just about everything in the coding guidelines is opinionated!
In cases where nulls are silently ignored, it can be a correctness issue. I'm thinking of something like var item = CurrentSelection as Package; if (item is not null) { DoSomething(item); } to handle the case when a button is clicked when no package is selected. If the type is wrong, customers will have selected an item, but nothing happens and the customer doesn't know why, and there are no errors to help us understand.
In cases of var item = CurrentSelection as Package; var name = item.Name; then a NullReferenceException is much harder to debug from telemetry compared to an InvalidCastException that tells use "Type1 cannot be cast to Type2".
So, I don't understand why this guideline would be less appropriate than most of the other opinionated guidelines in this document. Many others are nothing more than stylistic, this one actually reduces debugging complexity, hence increases productivity.
There was a problem hiding this comment.
This guideline makes sense to me. My ask is that we keep an eye where this guideline has an unintended consequence and revisit the guideline if needed.
Then handle it, like in my second "this is correct" snippet. How is a NullReferenceException any better? NREs are harder to debug, since they provide less information.
Yes, enabling nullable would tell the author that the code they're writing is not null safe. But in cases like |
| This is incorrect: | ||
|
|
||
| ```cs | ||
| ListItem item = selectedItem as ListItem; |
There was a problem hiding this comment.
Roslyn has an analyzer, Remove unnecessary cast (IDE0004) that flags instances when a cast is unnecessary. Perhaps the first step we should take is to change the severity of this analyzer to 'error.' I suspect that we are performing type casting as a safety measure, even in cases where these casts are implicit from the compiler's perspective.
|
Team Review: This is approved to be merged, but please update the PR description to show that this is a step we are taking before we turn on nullable checks on the codebase. |
| ``` | ||
|
|
||
| ```cs | ||
| if (obj is Type1 t1) |
There was a problem hiding this comment.
My understanding is this return/return/throw pattern is just an example, not a prescriptive pattern for all scenarios. With that understanding, I think it's worth trying and hopefully will help readibility.
Bug
Fixes: N/A
Description
Update coding guidelines to avoid using
askeyword for type casting, as it causesNullReferenceExceptionto be thrown instead ofInvalidCastException, making debugging much harder.In the future, if we can enable nullable referene types throughout the repo, then the compiler will tell us that the code is not null safe. But enabling nullable everywhere takes a lot of effort to fix all the existing issues and annotating APIs. So, this coding guideline is a reminder for us to write better null safe code until we can have the compiler automate null safety for us.
PR Checklist
PR has a meaningful titleno product changePR has a linked issue.
Described changes
Tests
Documentation