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

[terrajobst]

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.

[terrajobst]

One could argue that the collection initializer work isn't necessary because types like this will generally not have a constructor. ImmutableArray<T> is an exception which is handled by #34096.

[GrabYourPitchforks]

Related: FxCop rule CA1806 "Do not ignore method results"

[stephentoub]

We have a lot of methods with no side-effects that aren't annotated with Pure, which is part of the defunct System.Diagnostics.Contract. Is the intent of this that we start using it on all methods where it's relevant? Would we put this on string.Replace for example? Today it's pretty much only used with immutable collections, at least within .NET itself.

[terrajobst]

I don't think we need to add it on all methods that are side-effect free, but we should do it on those where mistakes are likely. Once the analyzer is there, we only need to apply the attribute, which is nice. Not sure string is likely these days, but for immutable collections it's common when people convert code to use it. Having it there would certainly help.

[TonyValenti]

Why not have the C# compiler automatically add [Pure] to methods it detects are pure? I can see this as a great enhancement that would actually allow the compiler to do further optimizations with common expressions.

[terrajobst]

Because it's generally hard to follow this. For example, a method that lazily initializes a cache isn't side effect free (it modifies a field) but it's basically not observable. I think trying to create Haskell out of C# won't fly.

[TonyValenti]

Except in that situation, the compiler that compiles the LazyInitializeCache function will know that it isn't pure and wouldn't decorate it as such.

[stephentoub]

we should do it on those where mistakes are likely

I'm fine with having an analyzer + attribute that we can use to mark methods whose return values shouldn't be ignored. My concern is using [Pure] for that, as a) it wasn't intended for this purpose, b) it's part of a defunct contracts system, c) it's not comprehensive enough, but most importantly d) there are cases where you don't want the return value to be ignored but your method isn't pure (e.g. Stream.Read isn't pure, but ignoring the return value is a recipe for corruption; File.ReadLines isn't pure, but ignoring its return value is just as bad as ignoring the return value of adding to an immutable collection, more so in fact). Midori had a very robust permissions system, much more expressive than [Pure], yet it still had a [DoNotIgnoreReturnValue] attribute that was separate. If we want such an analyzer, seems reasonable for it to also respect [Pure] as an aside, but I think it's focus should be on a dedicated attribute for the purpose. Such an attribute could also be used on out parameters, even ref parameters, which are just another form of return. We should also think about async methods, where the T in a Task shouldn't be ignored, but having [Pure] on such a method is unlikely to be true, nor is just ensuring that the task itself is consumed, but rather that its returned value is used.

[terrajobst]

Works for me

[ghost]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

[ghost]

This issue will now be closed since it had been marked no recent activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

[dellamonica]

I think this issue being closed automatically is a mistake. There is real benefit in having an attribute DoNotIgnoreReturnValue and code analysis that warns when the return is ignored.

(Such a bug just bit me a few hours ago with ImmutableList.Add, and it could have been trivially caught.)

[stephentoub]

Re-opening to track DoNotIgnoreReturnValue attribute + analyzer.

[tannergooding]

@stephentoub, how, roughly speaking, would this be different from IDE0058/IDE0059?

Just that it would be "on by default" and would help catch cases like ImmutableArray where the original value is not "up to date"?

[eerhardt]

Per this comment by @eerhardt, I'm moving this issue to 8.0:

We discussed it in the API review and decided to leave this off for now. If it is a common request, we can add it in the future.

That comment was in response to Would you consider extending that to allow types too? in this comment. It didn't apply to this whole feature.

[Eli-Black-Work]

@carlossanlop Can we move this back to 7.0.0, as per above comment?

[buyaa-n]

Estimates:

Analyzer: Large
Fixer: NA

[reduckted]

DateTime,DateOnly,DateTimeOffset,TimeSpan,TimeOnly AddX methods

Regarding the annotation of date and time methods, I've identified some additional methods in dotnet/roslyn-analyzers#6489 (I created that before I discovered this issue, so it seems redundant now).

DateTime and DateTimeOffset have Subtract(...), ToLocalTime() and ToUniversalTime().

TimeSpan has Divide(...), Multiply(...), Negate(...) and Subtract(...).

[jeffhandley]

@terrajobst / @eerhardt -- During PR review of the first part of this analyzer (ignoring return values), @stephentoub and I revisited the default level. We propose upgrading the level up to Warning since we're going to limit where the attribute gets applied to be places where it's almost certainly a lurking bug. Any objections?

[eerhardt]

No objection from me.

[jeffhandley]

As was noted in dotnet/roslyn-analyzers#6546 (comment), we had some offline discussions about this analyzer and its potential relationship to IDE0058: Remove unnecessary expression value - .NET | Microsoft Learn and CA1806: Do not ignore method results (code analysis) - .NET | Microsoft Learn. We have not had the capacity to move that design discussion forward enough to have confidence in landing in a good place during .NET 8.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

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.

Author:	terrajobst
Assignees:	jeffhandley
Labels:	api-needs-work, area-System.Runtime, code-analyzer
Milestone:	8.0.0

[glen-84]

Could this also allow for specifying a list of fully-qualified return types that must never be ignored?

For example, I have Result and Result<TValue> types that are returned from many different methods. It would be a lot easier to configure these types as "DoNotIgnore", as opposed to applying the DoNotIgnore attribute to every method.

---

See also dotnet/roslyn#47832.

[KalleOlaviNiemitalo]

@glen-84 , when you write "fully-qualified", do you mean you might want to warn about ignoring List<VeryImportant> but not warn about ignoring List<WhoCares>?

[glen-84]

@KalleOlaviNiemitalo I was referring mainly to the namespace. I haven't considered generic type arguments.

[mykolav]

Hi,

Sorry for a shameless self-promotion.

While the official analyzer is in works, you might consider checking out this one https://github.com/mykolav/must-use-ret-val-fs
It emits a diagnostic if it comes across code that ignores the value returned from a method marked with [MustUseReturnValue].

[timcassell]

Would you consider extending that to allow types too?

+1

I wrote a promise library where each promise object is required to either be awaited, or forgotten. Using discard instead of Forget is wrong. It looks like this proposal treats discards as "not ignored", but I would want it to treat it as "ignored". Could that be configurable?

[julealgon]

Were there any discussions around alternatives to an attribute in this case? For example, what about using required before the return type to signal it must be consumed by the caller?

public required Task<int> ReadAsync(...);

It feels more "first-class" to me to have it as a language keyword vs an attribute.

Additionally, it could also be used to signal similar semantics for other cases, such as if you want to enforce a given out parameter to not be discarded as well:

public void MyMethod(required out int value);