dotnet / docs

This repository contains .NET Documentation.
https://learn.microsoft.com/dotnet
Creative Commons Attribution 4.0 International
4.22k stars 5.87k forks source link

CA1806's doc fails to mention IDE0058, it's wrong about Linq, and should recommend the discard operator #24435

Open daiplusplus opened 3 years ago

daiplusplus commented 3 years ago

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1806

1: No mention of IDE0058

It feels to me like both analysis rules were written by independent teams completely ignorant of each other's work.

2: The doc is wrong about Linq:

This statement is untrue and potentially harmful:

LINQ methods are known to not have side effects, and the result should not be ignored.

Linq methods can and do have side-effects depending on the background Linq provider, such as Linq-to-Entities (and Linq-to-SQL) where materialization of a Linq query can happen unintentionally even without a final ToList/ToDictionary/ToArray call. Another risk is that a Linq expression invokes a database procedure or trigger (as some databases, like Informix, do support SELECT-triggers).

3: It should recommend using the discard operator:

The article does not mention the C# discard operator at all, despite it being available since C# 7.0 and because the CA1806 rule does recognize the operator:

When to suppress warnings

Do not suppress a warning from this rule unless the act of creating the object serves some purpose.

Instead it should read:

When to suppress warnings

Instead of suppressing the rule when a method's return value serves no purpose, use the discard operator to signify intent that the return value is unneeded:

_ = MethodWithIgnoredReturnValue();

I would also suggest an example including StringBuilder. For example, this code triggers CA1806:

StringBuilder sb = new StringBuilder("foo");
sb.Append("bar");
sb.Append("baz");

This does not:

StringBuilder sb = new StringBuilder("foo");
_ = sb.Append("bar");
_ = sb.Append("baz");

(Can we customize CA1806 (or IDE0058) to ignore StringBuilder.Append yet?)


I did start writing changes in my own fork of the docs but I realised my changes would be too extensive for a simple PR and would likely be rejected - hence me filing this issue first.


Target framework

[Edited below by gewarren]


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

Youssef1313 commented 3 years ago

I'd say that both should be consolidated into one, either as an IDExxxx or a CAxxxx

@mavasani What do you think?