Is/should there a code-style for avoiding null on left side of a binary expression? #40561
Labels
Milestone
Comments
Dotnet-GitSync-Bot
added
area-Meta
untriaged
|
Note that we do have this for the C++ code styling guide already: https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/clr-jit-coding-conventions.md#1517-if-conditions I don't think Yoda conditions are really common enough of a pattern to see in the .NET world. Maybe that's why it's not specifically mentioned there... IMO, if we ever decide to add this in the rule then it should just be about the whole Yoda notation instead of just |
|
Searching by "Yoda", I'm seeing some PR reviews in the runtime/coreclr/corefx that asks people to avoid using Yoda conditions in C# code: #33603 (comment) #38161 (comment) #38001 (comment) dotnet/corefx#31827 (comment) so it probably was just missed out for not being common. |
|
Enabling SA1131 results in 222 warnings for Clr+Libs build. List |
|
@Gnbrkm41 would it make sense to do a PR to fix those warnings, enable the setting, and update the code style guide? I am happy to do this. |
|
Not sure, probably other folks can decide. After all I'm just a contributor >.> Note that some of the code are direct port of some other libraries in C/C++. |
|
@jkotas You have been involved in the code style guide previously. What are your thoughts on this? |
|
I do not have an opinion on this one. @danmosemsft ? |
|
I believe we have a defacto preference for avoiding Yoda's (preference, we have...). There is no safety advantage except for bools and it isn't how you read it in your head (if null is Foo vs if Foo is null) @bartonjs who normally is opinionated about style on the team? |
|
Plus consistency is good if nothing else and it sounds like there's an analyzer and a volunteer. |
Not always. For example, the analyzer complains about |
While I believe you're asking me who (else) is normally opinionated, without the question mark (and inserting a comma) it just becomes "Jeremy, who normally is opinionated", which holds, too 😄. When it comes to style preferences, I'm possibly the most opinionated (or just the most vocal), followed by @stephentoub. The rule, when I ask myself what my rule is, is "the most important thing goes on the left". But I'm having a hard time imagining a case where I'd write a left-constant. Certainly back in my early days of C# I was still a left-conster from my C days, but now I think I'm a right conster. (And even though I don't personally like Given that we already have some stylecop things turned on, I think I'd be OK with adding this to the list. This is |
Lol OK I think this is a tentative consensus.. You mentioned there's an analyzer, I'm not sure what our criteria are for the level of quality of analyzers we allow in our build process but at least it should help you find them for your effort even if we don't use in our build . |
|
@danmosemsft Sure. Do you want me to update the code style guide as well? What language do you want to use for the rule? |
|
I suggest just a code PR first so we can get an idea of what the scale of the changes are.
I don't follow...we're not suggesting you write an analyzer. We don't store them in this repo anyway. |
sorry, I meant the wording of the rule in the Code Style Guide. I'll get the PR sorted first :) |
|
Roslynator has a vs extension which allows refactoring. So it doesn't need to be included in the project. It only deals with nulls on the left side though vs the stylecop analyzer which finds all constants on the left hand side. https://github.com/JosefPihrt/Roslynator/blob/master/docs/analyzers/RCS1098.md |
|
runtime/eng/CodeAnalysis.ruleset Line 295 in 0d6d73a |
|
Is there a way to open src\libraries\Common\src as a solution? That would make it a lot easier. |
|
@elachlan no I don't think so. Can you enable the rule, build, and pipe the output through VS or VS Code or some other tool that makes the errors clickable? (I'm assuming it doesn't auto fix?) |
@elachlan not sure if this is what you are looking for but if you are trying to automatically detect/apply a rule/fix across multiple projects you can run this script https://github.com/dotnet/runtime/blob/master/src/libraries/GenerateLibrariesSln.ps1 which creates a solution with all the |
|
@jozkee thank you so much, this makes it so much easier! |
joperezr
added
question
Merged
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is there are code-style for avoiding null on left side of a binary expression? If not an explicit requirement, maybe a guideline?
https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/coding-style.md
https://github.com/JosefPihrt/Roslynator/blob/master/docs/analyzers/RCS1098.md
The text was updated successfully, but these errors were encountered: