IDE0058 (csharp_style_unused_value_expression_statement_preference) should offer granular (type-specific, method-specific) suppression

[daiplusplus]

Analyzer

Diagnostic ID: IDE0058

Describe the improvement

I've found the warnings from IDE0058 to be very helpful in catching cases where return values should be used/inspected or where the call-site should explicitly discard the return value, however there are many types built-in to the .NET Framework / .NET Core / etc (and our own projects and third-party libraries) that have methods that return an insignificant value (i.e. it should be explicitly discarded) but these are APIs where adding an explicit discard would add visual noise.

Examples include StringBuilder.Append(..,) (which returns itself) and many fluent interfaces.

Here's a screenshot of VS2019 complaining about me ignoring the return value from StringBuilder.Append. I'm sure you'll agree it's far too squiggly! - but I don't want to blindly suppress IDE0058 (either globally or using #pragma warning disable inside each relevant class/method/scope) because I want to be informed of other unintentionally ignored return values on other methods.

(I note that I'd prefer it if methods didn't return this to allow for method-chaining: that's abusing language semantics for the sake of slightly cleaner syntax - C# should have first-class support for syntax like chaining but without requiring return this (and [return: ] attributes to indicate if a return value actually is this or something else) - but I digress...

Describe suggestions on how to achieve the rule

I think the safest approach would be to allow us to manually specify in .editorconfig every particular method overload where the return-value should be ignored. This may result in a lot of entries being added to .editorconfig though. To provide for a simpler experience there should be a secondary ability to specify a list of method-owner-types (which can include subclasses with inherited methods) for which IDE0058 will be automatically ignored or suppressed.

Therefore I'm proposing the addition of these new entries to .editorconfig for IDE0058:

• csharp_style_unused_value_assignment_ignore_types
  • Comma-separated list of type-names where all call-sites for all of their methods should be ignored by IDE0058.
• csharp_style_unused_value_assignment_ignore_types_namespaces
  • Comma-separated list of fully-qualified C# namespaces to search when resolving type-names specified in these new editorconfig properties.
• csharp_style_unused_value_assignment_ignore_methods
  • Comma-separated list of fully-qualified method overloads (ideally using C# dot syntax, but using alternative syntaxes such as the XML Documentation cref syntax or the .NET fully-qualified type-name (incl generic type backticks and nested-type + path characters) work just as well.
  • The entry should also allow for specifying just the containing type-name and the method-name (i.e. without specifying the exact overload's parameters) - in this case all matching overloads will be ignored by IDE0058 (e.g. StringBuilder.Append would ignore all overloads of Append whereas StringBuilder.Append(String) would only ignore that specific overload.

Example .editorconfig file:

csharp_style_unused_value_assignment_preference = discard_variable:warning
csharp_style_unused_value_expression_statement_preference = discard_variable:warning

# List the namespaces needed to resolve the type-names specified below (e.g. StringBuilder)
csharp_style_unused_value_assignment_ignore_types_namespaces = System, System.Globalization, System.Text, MyProject

# Ignore IDE0058 on all StringBuilder method call-sites:
csharp_style_unused_value_assignment_ignore_types = StringBuilder    

# Ignore IDE0058 on all overloads of StringBuilder.Append and AppendFormat, and only this specific overload of StringBuilder.Insert:
csharp_style_unused_value_assignment_ignore_methods = StringBuilder.Append, StringBuilder.AppendFormat, StringBuilder.Insert(Int32,Byte)

Additional context

Assuming this is possible, the Code Fixes menu should have new Suppression options to suppress either the call-site's specific method overload, all methods with the same name, or all methods on the containing type.

Questions

• Should it be possible to specify specific generic parameter arguments?
  • e.g. if you want to ignore HashSet<String>.Add but not HashSet<Int32>.Add.
• Given that the syntax for specifying a method overload will include commas , to separate method parameters - but commas are already used to separate values in comma-separated lists in .editorconfig - so how should those commas be disambiguated?
  • One option is to require method-overload specifiers that contain commas to be wrapped in double-quotes (but have double-quotes be optional for specifiers without commas). Is there any alternative?

[StingyJack]

Since the issue I had opened on devcomm was closed as a dupe of this, I would like to offer the more direct alternative of having the analysis rule just not report the methods on StringBuilder (or any well known "Builder" type) as a violation. Having an exclusions list for non-built-in types can be valuable, but we should not have to maintain any built in types in this list and I think that the direct bypassing of StringBuilder by the analysis rule can be turned around much quicker than adding support for an exclusions list.

[daiplusplus]

@StingyJack

I would like to offer the more direct alternative of having the analysis rule just not report the methods on StringBuilder (or any well known "Builder" type) as a violation.

I agree with having a useful set of default exclusions of all known in-box return this methods, yes.

Having an exclusions list for non-built-in types can be valuable

I need it for my own projects anyway.

but we should not have to maintain any built in types in this list

The problem with any built-in hardcoded list is that the list will go stale very quickly and updating that list probably won't be a priority of the .NET team.

The list cannot be automatically-generated either, because .NET assembly metadata does not currently provide a way to indicate which methods return this and which don't. There are also plenty of methods that don't return this but will return a zero-information return value.

Unfortunately using CIL/MSIL bytecode analysis won't help either because the Reference Assemblies don't contain that information at all.

and I think that the direct bypassing of StringBuilder by the analysis rule can be turned around much quicker than adding support for an exclusions list.

That still leaves out dozens of classes (and hundreds... to thousands?) of methods in the .NET BCL that are just as annoying and people don't like it when tools behave inconsistently or give certain built-in types/features/components special status compared to everybody else.

Also, a blanket-list of all StringBuilder methods would be incorrect as it excludes ToString(), EnsureCapacity() and Equals().

---

I just thought about it more and I realize that even if static-analysis could get a list of all return this methods, it would cause false-negatives (which arguably worse than false-positives when writing fail-safe software) when the method that return this is in an object created by an earlier method call, for example:

public class VeryImportantObject
{
    public VeryImportantObject()
    {
    }

    public VeryImportantObject SomeChainableMethod()
    {
        return this;
    }
}

public VeryImportantObject CreateVeryImportantObject()
{
    return new VeryImportantObject();
}

public void DoThing_1()
{
    CreateVeryImportantObject(); // This triggers IDE0058, good!
    _ = CreateVeryImportantObject(); // This resolves IDE0058 and signifies intent to discard an important object.
}

public void DoThing_2()
{
    // If all `return this` methods were automagically excluded then the code below wouldn't trigger IDE0058, even though arguably it should be raised as the reference to the returned `VeryImportantObject` is lost.
    CreateVeryImportantObject() 
        .SomeChainableMethod();
}

So for the analyzer to handle this correctly it would need to either:

1. Perform more NP-hard static flow-analysis to determine when significant objects or values are lost unintentionally.
   • So kiss your sweet sub-second build-times goodbye.
   • This assumes that it's even possible to always be able to tell when a method will always return this, even with manually-annotated [return: AlwaysReturnsSelf].
2. Need a manually-maintained exclusion-list which, as I said, will go stale.

The best alternative is to allow us to maintain our own exclusion lists.

[StingyJack]

The problem with any built-in hardcoded list is that the list will go stale very quickly and updating that list probably won't be a priority of the .NET team.

Really I just want it to stop reporting StringBuilder, because this CA is very noisy for it, I dont want to have to update the editor.config in 1600 repos every time an exclusion needs to be added. I think our chances of getting this narrower scoped item addressed are better than getting the wider scope.

[smaillet]

I'd recommend following the C++ model here and support a configuration that ONLY triggers this warning/error if the return attribute NoDiscardAttribute (Or equivalent new attrib) isn't present. That way method implementers can clearly state that it's perfectly OK to ignore the return type.

I generally find this warning so obnoxious in the "boy that cried wolf" fashion I just disable it outright.

[uecasm]

As an alternative attribute-based proposal (since I dislike C++'s nodiscard choice), I suggest introducing an attribute [Discardable] (or perhaps [IgnorableReturn] or other bikeshedding) that can be applied either to a method or directly to a return-value to indicate that IDE0058 should not be reported for this return value in particular even when otherwise enabled -- but unadorned methods should by default trigger IDE0058 (when enabled).

This should then be applied to the fluent-builder methods and several other places such as collection Add/Remove methods where the return value can be interesting but is not always so. This is usually fairly obvious on a method-by-method basis, when it still makes sense to use the method only for its side effect and not its return. This will take a while to filter down through the library ecosystem once introduced, so the earlier introduced the better, and other suppression techniques can be used in the interim.

[glen-84]

As a first step, would it not make sense to update the analyzer to ignore all fluent methods (methods that return the same type as the type on which they were invoked)?

Examples:

• IApplicationBuilder.Use*
• IServiceCollection.Add*
• IWebHostBuilder.Use*

Are there any reasons to flag fluent methods?

As a second step, allow specific fully-qualified methods to be ignored through configuration.

This rule is completely useless at the moment, which is unfortunate.

---

See also #34098.

[daiplusplus]

@glen-84

would it not make sense to update the analyzer to ignore all fluent methods (methods that return the same type as the type on which they were invoked)?

That wouldn't work for immutable builder methods, as they often do return the same type, Linq's IQueryable<T> comes to mind.

[glen-84]

Ah, true.

It seems that this issue is about different ways of marking return values as ignorable, whereas #34098 is the opposite, marking values as "do not ignore". I wonder what the best approach is.

[detzt]

I'd say showing the warning unless the return value is marked as can be ignored is way more useful because initially nothing is categorized and you can't expect all libraries that you use to apply do not ignore.
If you care about unused values, it's better to get too many warnings (false positives) than too few (false negatives).
If you don't care about them, you can disable the analyzer rule altogether.

Moreover, ignorable return values like those in fluent APIs tend to be grouped together, which makes it easier to mark all of them as can be ignored than the other way around.