Skip to content

Do not discard results of methods that shouldn't be ignored #34098

Open
@terrajobst

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

  1. System.Collections.Immutable members that used to have [Pure], but were removed with Remove Pure attribute from System.Collections.Immutable #35118.
  2. Stream.ReadX methods that return how many bytes were read
  3. Tuple.Create (Add Pure-attribute to Tuple types #64141)
  4. DateTime,DateOnly,DateTimeOffset,TimeSpan,TimeOnly AddX methods (DateOnly.AddX methods should be marked [Pure] #63570)
  5. string creation APIs (ToUpper, Replace, etc.)
  6. 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 awaited 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:

  1. 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 the Reliability category.

Of the rules for CA1806:

  1. new objects
  2. string creation APIs
  3. ignoring HResult
  4. Pure attribute
  5. TryParse
  6. LINQ
  7. 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.

Metadata

Assignees

Labels

api-needs-workAPI needs work before it is approved, it is NOT ready for implementationarea-System.Runtimecode-analyzerMarks an issue that suggests a Roslyn analyzer

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions