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

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 #35118.
Stream.ReadX methods that return how many bytes were read
Tuple.Create (#64141)
DateTime,DateOnly,DateTimeOffset,TimeSpan,TimeOnly AddX methods (#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 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:

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:

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.

About this issue

Original URL
State: open
Created 4 years ago
Reactions: 36
Comments: 54 (44 by maintainers)

Most upvoted comments

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<T> 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.

+10

stephentoub on Mar 26, 2020

In general, all the LINQ methods are not proposed to be marked with DoNotIgnore attributes because there are valid cases for calling .Select and other methods that can be side-effecting. If there are one-off ones that make sense to be marked, we can consider adding them.

LINQ methods can generally be split into two categories: those that force enumeration (e.g. Count, Max, Min, Aggregate, etc.) and those that create new lazy enumerables (e.g. Select, Where, Append, Concat, etc.). We can’t attribute the former category as DoNotIgnore, since as you say there are legitimate uses for calling such methods primarily to force evaluation of the query in order to exercise any side effects of those operations. However, it seems we should be able to annotate methods in the latter category; even if the execution of the query operator has side effects (e.g. invoking the delegate passed to Select), the operator is lazy and won’t do any work until the enumerable it returns is enumerated… if that enumerable is ignored, then the operator won’t have done anything observable (other than argument validation).

stephentoub on Jun 14, 2022

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<T>.Add, and it could have been trivially caught.)

dellamonica on Nov 9, 2021

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.

eerhardt on Jul 6, 2022

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

There are many methods for which those are noise because it’s perfectly fine in many situations to ignore the return value, e.g. HashSet<T>.Add, Dictionary<TKey, TValue>.Remove, etc., so much so that it’s often overwhelming to try to enable them in a codebase (lots of work resulting in lots of suppressions to avoid false positives). But there are a much smaller number of methods where not using the result is almost always a bug, e.g. Stream.Read, string.Replace, etc.

If we want to teach IDE0058/59 about the new attribute, e.g. with a setting that causes them to focus only on such members, rather than introducing an additional analyzer / diagnostic code, I’d be fine with that. But there needs to be some way to opt-in the list of members to examine, and an attribute is a good way to do that.

stephentoub on Jan 10, 2022

Related: FxCop rule CA1806 “Do not ignore method results”

GrabYourPitchforks on Mar 25, 2020

@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?

jeffhandley on Mar 26, 2023

We changed Inherited true to Inherited false
We think that the analyzer should only look at the specific method declaration, and not try to infer the behavior by walking the virtual hierarchy
For return values, discard assignment (_ = obj.Method()) should count as “not ignoring”
For out values, discard assignment (out _) should count as “not ignoring”.
The proposed members seem like a good place to start, and once the attribute is merged in send an announcement to have area owners apply it to their areas as they see fit.

namespace System.Diagnostics.CodeAnalysis
{
    [AttributeUsage(AttributeTargets.ReturnValue | AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class DoNotIgnoreAttribute : Attribute
    {
        public DoNotIgnoreAttribute() {}

        public string? Message { get; set; }
    }
}

bartonjs on Jun 9, 2022

I don’t think either category would be sufficient justification for it to be an error/build break

@eerhardt and I spoke earlier today and this is effectively the same conclusion we reached. He has more details, but strawman is essentially a default level of info, reliability category, attributing everything where there’s a very good chance it indicates a mistake but not things where there are legitimate reasons you might want to ignore the return value, e.g. there’s basically no reason to call string.Replace and ignore the return value, but there are valid reasons you might want to call Enumerable.Single and ignore the return value.

stephentoub on May 25, 2022

Agreed. Also, in my opinion it does very little to improve the readability of the code and arguably makes it worse.

However, I do see value in adding this to API where not consuming the return value is a bug. Immutable APIs are great candidates.

Should we close #34096 in favor of this? It seems we’d prefer this design over a specialized analyzer.

terrajobst on Jan 10, 2022

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.

glen-84 on Dec 12, 2023

As was noted in https://github.com/dotnet/roslyn-analyzers/pull/6546#issuecomment-1546212215, 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.

jeffhandley on May 12, 2023

DateTime,DateOnly,DateTimeOffset,TimeSpan,TimeOnly AddX methods

Regarding the annotation of date and time methods, I’ve identified some additional methods in https://github.com/dotnet/roslyn-analyzers/issues/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(...).

reduckted on Feb 18, 2023

I just made the mistake of .Append(str) on a List<string> (instead of .Add(str)).

Did we conclude that we would mark something like LINQ’s Append(..) such that it’s flagged by this analyzer?

danmoseley on Jun 13, 2022

Would you consider extending that to allow types too?

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.

eerhardt on Jun 9, 2022

and it would be nice to have the opposite of DoNotIgnoreAttribute — something like MayIgnoreAttribute

I would suggest to open a new issue for that scenario. These two scenarios, while related, aren’t dependent on each other so I wouldn’t group it into this proposal.

eerhardt on May 31, 2022

One issue I have with the current design is that I see methods falling into 2 categories:

Ignoring the result of this method is basically guaranteed to be a bug in your code - i.e. Reliability. (Stream.Read, ImmutableCollection.Add, string.Replace/Remove, etc.)
Ignoring the result of this method means the method call is unnecessary - consider removing the method call - i.e. Performance. (LINQ methods, string.Clone/Copy/Split, etc)

I am wondering if it would be valuable to add a bool parameter to DoNotIgnoreAttribute’s ctor, similar to ObsoleteAttribute’s bool error. This would allow us to keep using CA1806 Rule ID for the performance cases, and a new Rule ID for the Reliability cases.

eerhardt on May 20, 2022

Is [Conditional(“CONTRACTS_FULL”)] needed?

No. It’s part of the legacy contracts system. This should not be included.

System.Diagnostics.Contracts

It should be in System.Diagnostics.CodeAnalysis.

It should detect async methods that return Task.

Can you clarify what this is intended to mean? Consider a method like:

[DoNotIgnoreReturnValue]
public Task<int> ReadAsync(...);

What does the “do not ignore” refer to? Will it warn for:

ReadAsync(...);

where the returned task is ignored? Will it warn for:

await ReadAsync(...);

where the Task<int> isn’t ignored but the int extracted from the Task<int> and returned from the await is ignored? What about if the API returned ValueTask<T> instead of Task<T>, or some other awaitable?

stephentoub on May 13, 2022

So from this comment, there are 2 work items to tackle with this issue:

Work items

Add the new attribute [DoNotIgnoreReturnValue]
Add an analyzer:
- It should detect methods decorated with the new [DoNotIgnoreReturnValue] attribute.
- It should detect async methods that return Task.
- It does not have a code fixer. Is up to the user to decide what to do with the return value.

The [Pure] attribute is ignored.

Suggested category: Reliability Suggested severity: warning

API proposal

namespace System.Diagnostics.Contracts
{
    [Conditional("CONTRACTS_FULL")]
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public sealed class DoNotIgnoreReturnValueAttribute : Attribute
    {
    }
}

Question: Is [Conditional("CONTRACTS_FULL")] needed? It can be found it on top of PureAttribute. The documentation says that ConditionalAttribute Indicates to compilers that a method call or attribute should be ignored unless a specified conditional compilation symbol is defined. but I am not sure what CONTRACTS_FULL is for.

@stephentoub @tannergooding @terrajobst @GrabYourPitchforks thoughts on this proposal?

carlossanlop on May 12, 2022