Do not discard results of methods that shouldn't be ignored #34098
Description
In .NET APIs, calling methods for side effects is the norm. Thus, it's generally OK to call a method and discard the result. For example, List<T>
s Remove()
method returns a Boolean, indicating whether anything was removed. Ignoring this is just fine.
However, there are cases where ignoring the return value is a 100% a bug. For example, ImmutableList<T>
's Add()
has no side effects and instead returns a new list with the item being added. If you're discarding the return value, then you're not actually doing anything but heating up your CPU and give work to the GC. And your code probably isn't working the way you thought it was.
We should add the ability for specific APIs to be marked as "do not ignore return value", and have an analyzer that flags callsites to these methods that don't capture the return value.
Proposal
Suggested severity: Info
Suggested category: Reliability
namespace System.Diagnostics.CodeAnalysis
{
[AttributeUsage(AttributeTargets.ReturnValue | AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
public sealed class DoNotIgnoreAttribute : Attribute
{
public DoNotIgnoreAttribute() {}
public string? Message { get; set; }
}
}
Methods to annotate
- System.Collections.Immutable members that used to have
[Pure]
, but were removed with Remove Pure attribute from System.Collections.Immutable #35118. - Stream.ReadX methods that return how many bytes were read
- Tuple.Create (Add Pure-attribute to Tuple types #64141)
- DateTime,DateOnly,DateTimeOffset,TimeSpan,TimeOnly AddX methods (DateOnly.AddX methods should be marked [Pure] #63570)
- string creation APIs (ToUpper, Replace, etc.)
- More as we identify them...
The general rule is that we only annotate APIs where there is a very high likelihood that your code is a mistake. If there are generally valid reasons for ignoring a result (like creating new objects can have side-effects, ignoring TryParse and use the default, etc.), we won't annotate the API with [DoNotIgnore]
.
Usage
If a method is annotated with [return: DoNotIgnore]
, discarding the return value should be a flagged:
var x = ImmutableArray<int>.Empty;
x.Add(42); // This should be flagged
string s = "Hello, World!";
s.Replace("e", "i"); // This should be flagged
using FileStream f = File.Open("readme.md");
byte[] buffer = new byte[100];
f.Read(buffer, 0, 100); // This should be flagged
void Foo([DoNotIgnore] out int a) { a = 5; }
Foo(out int myInt); // This should be flagged since myInt is not used
Annotating {Value}Task<T>
Methods marked with [return: DoNotIgnore]
that return Task<T>
or ValueTask<T>
will be handled to apply both to the Task that is returned, as well as its await
ed result.
[return: DoNotIgnore]
public Task<int> ReadAsync(...);
ReadAsync(...); // This should be flagged
await ReadAsync(...); // This should be flagged
await ReadAsync(...).ConfigureAwait(false); // This should be flagged
DoNotIgnore on parameters
DoNotIgnoreAttribute is only valid on ref
and out
parameters - values that are returned from a method. We should flag annotating normal, in
, and ref readonly
parameters with [DoNotIgnore]
.
Interaction with CA1806
The DoNotIgnoreAttribute
will use a new Rule ID
distinct from CA1806
. The reason for this is:
CA1806
is in the "Performance" category. It is concerned with doing unnecessary operations: ignoring new objects, unnecessary LINQ operations, etc.DoNotIgnoreAttribute
is about correctness and is in theReliability
category.
Of the rules for CA1806
:
new
objects- string creation APIs
- ignoring HResult
- Pure attribute
- TryParse
- LINQ
- User-defined
The only APIs that will also be marked [return: DoNotIgnore]
are the string creation APIs. The only valid scenario to ignore one of the string APIs results that I've found is to try-catch
around a string.Format
call, and catch FormatException
to do validation and throw some other exception. When the string creation APIs are annotated with [return: DoNotIgnore]
, the new Rule ID will be raised, and CA1806
won't fire. But for older TFMs where the new attribute doesn't exist, CA1806
will still be raised.