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

[stephentoub]

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

[tannergooding]

Thanks, makes sense!

lots of work resulting in lots of suppressions to avoid false positives

Well, except this bit. I've never used suppressions here, just used the discard pattern _ = set.Add(x); and the IDE quick fix when first enabling it on new codebases.

[stephentoub]

I've never used suppressions here

That's a suppression, just a different form :)

[terrajobst]

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.

[carlossanlop]

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?

[stephentoub]

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?

[eerhardt]

I've updated the top post with the current proposal. Please take a look and provide feedback. Once we are happy with the proposal, I can mark this as api-ready-for-review.

[eerhardt]

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

1. 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.)
2. 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.

[stephentoub]

Ignoring the result of this method means the method call is unnecessary

That might be the case, but it might also just be another case of your code being buggy, i.e. the method call is necessary and the fact that you're ignoring the result means you have a bug.

[eerhardt]

That might be the case, but it might also just be another case of your code being buggy, i.e. the method call is necessary and the fact that you're ignoring the result means you have a bug.

Agreed, we can't really know the intent of a developer who writes:

void Foo()
{
    string.Copy("Hello");
}

Were you supposed to do something with that copied string? Or was it just a mistake?

The delineation I was trying to make is that there are some methods that it is critical you do something with the result. So critical that we should make it a warning, enable it by default, and categorize it as a "Reliability" issue. I don't think all of the current CA1806 violations fall into this category. Specifically:

• A new object is created but never used.,
• the LINQ methods
• string.Clone/Copy/Split/Format/Join

An option is we leave the current APIs being flagged as CA1806 as they are today, and we don't mark any of those APIs as DoNotIgnore. Then introduce a new Rule ID for the DoNotIgnore attribute, mark it as Reliability, warning, and enabled by default.

[stephentoub]

I don't think all of the current CA1806 violations fall into this category

Certainly not "a new object is created but never used", assuming that's not just about "System.Object"... it's not unheard of to create an instance in order to use it for the side-effects of its constructor. While possibly a perf issue, it's not necessarily a functional one.

But the others, I have trouble differentiating possible reliability impact of ignoring the result of string.Replace (which you've grouped into category 1) and ignoring the result of a LINQ query (which you've grouped into category 2). I don't know how we'd meaningfully decide for a bool like you suggested whether something should be a warning or an error; we'd have to be able to understand the impact on the code, in any of these cases, and that's not something the analyzer will be able to do.

[eerhardt]

assuming that's not just about "System.Object"...

Looking at the code, it applies to all object creations.

I don't know how we'd meaningfully decide for a bool like you suggested whether something should be a warning or an error; we'd have to be able to understand the impact on the code, in any of these cases, and that's not something the analyzer will be able to do.

My thinking was that the analyzer wouldn't do this. The API author would decide whether ignoring the result of the method being marked DoNotIgnore should be considered a "Reliability" problem or a "Performance" problem.

But if you don't think having this distinction in the attribute makes sense, I think we can drop it.

What do you think about the option to keep all existing methods as CA1806, introduce a new Rule ID for DotNotIgnore, and don't mark any existing methods (like LINQ and string creation) as DoNotIgnore?

[stephentoub]

But if you don't think having this distinction in the attribute makes sense, I think we can drop it.

I think we'd have a really hard time making that call for all of the APIs we produce.

What do you think about the option to keep all existing methods as CA1806, introduce a new Rule ID for DotNotIgnore, and don't mark any existing methods (like LINQ and string creation) as DoNotIgnore?

@mavasani, do you have an opinion here? Any sense for how common it is for CA1806 to be used? It looks like it defaults to Info level.

[mavasani]

I think we'd have a really hard time making that call for all of the APIs we produce.

I personally feel it is going to be valuable to have the new attribute take some parameter (a bool or an enum specifying the issue category if return value is ignored), otherwise we are going to soon run into APIs where it is really bad to ignore its return value, but not significant enough or always guaranteed to lead to a functional bug to break the build. Given the new rule will be build breaking by default (warning and error severities are both considered build breaking), the API author would be unable to mark it with the new attribute. I understand @stephentoub's concern that by adding this additional parameter to the attribute, we are pushing a difficult task to the API authors. It is a tradeoff between additional functionality and additional overhead.

Any sense for how common it is for CA1806 to be used? It looks like it defaults to Info level.

As you have identified in this issue, CA1806 (performance) as well IDE0058 (code style/refactoring aid) are half baked rules, based on heuristics/hard coded APIs to flag. Former is too restrictive and misses lot of cases, while latter is too permissive and hence we could not even make it an IDE code style suggestion, it is hidden (refactoring) by default. We have always had lot of requests to be able to mark and flag custom APIs whose return value is ignored, and we ended up adding an editorconfig option for users to specify custom APIs to be flagged by the CA1806 analyzer. So the most requested use case for CA1806 is already being served by the new attribute/analyzer being proposed in this issue, which is great. I feel it is fine to either fold the existing cases handled by CA1806 into the new analyzer rules (if we are planning to add a separate parameter to the attribute to flag non-critical non-build breaking cases) and then deprecate CA1806 OR retain existing cases for CA1806, and have the new analyzer rules only for cases not handled by CA1806.

Note that we do have some precedent here for parallel rules that differ just by the issue category, but have identical analysis: CA1307: Specify StringComparison for clarity and CA1310: Specify StringComparison for correctness. CA1307 is disabled by default, while CA1310 is hidden by default (https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/Core/AnalyzerReleases.Shipped.md).

[terrajobst]

Personally, I don't think either category would be sufficient justification for it to be an error/build break.

• Existing code might have a bug, but if it was undetected the world hasn't ended yet. Failing it now seems unnecessary draconian.
• For new code, the user is still writing that code. Failing the build that early might be a productivity hit. Sure, I might not pay attention to warnings because I have already a ton of them, but at least for me the squiggles in the code I just wrote generally get my attention.

[stephentoub]

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.

[GrabYourPitchforks]

The API is still marked as a suggestion, not ready for review. Would people feel more confident bringing this through review if we spent some time generating a more concrete list of methods to annotate?

I feel the API is fine as is, unless we needed the ability to add a message to it for some reason. e.g., calling Dictionary.TryGetValue and ignoring the out parameter: you should be using ContainsKey instead.

[eerhardt]

I've updated the top post with my current proposal. Notable changes are:

• Suggested severity: Info
• The Interaction with CA1806 section was rewritten
• I captured the general rule of when we will annotate a dotnet/runtime API
• Added an optional Message to the attribute

Would people feel more confident bringing this through review if we spent some time generating a more concrete list of methods to annotate?

Do you have any thoughts on how to generate this list? The only one I have is to go through all our APIs and apply the rule. The other thought is that we start with some in .NET 7, and we add more over time as people propose them.

[carlreinke]

I set IDE0058 to suggestion in most of my projects, and it would be nice to have the opposite of DoNotIgnoreAttribute — something like MayIgnoreAttribute. The bar for applying it would necessarily be set pretty high — something like "the return value is something you definitely already have". So, for example, MayIgnoreAttribute would be applied to StringBuilder.Append(...) but not to Stream.Seek(...).

[eerhardt]

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]

Marking as "ready for review"

[bash]

As a library author, I would love to have this feature.

I see that the proposed attribute can be applied to methods only. Would you consider extending that to allow types too? Applying the attribute to a type would mean that all methods that return that type would be treated as if they were annotated with [DoNotIgnoreAttribute]. This would be really useful for functional style result types (Either / Result / etc.).

(Rust has the very similar #[must_use] attribute which can be applied to both types and functions.)

[bartonjs]

• 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; }
    }
}

[eerhardt]

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.

[danmoseley]

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?

[TonyValenti]

It would be great if any Pure method opted into this as well.

[eerhardt]

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

This is already flagged by CA1806. https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1806. 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.

It would be great if any Pure method opted into this as well.

Pure methods are already covered by CA1806 as well - https://github.com/dotnet/roslyn-analyzers/blob/e52a3f1c0b45684ddaa041f26a980fa451ea9001/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/DoNotIgnoreMethodResults.cs#L346-L349.

However, no APIs in dotnet/runtime are marked with the Pure attribute anymore. The only ones that were are the System.Collections.Immutable APIs, and those Pure attributes were removed with https://github.com/dotnet/runtime/pull/35118.

[stephentoub]

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

[carlossanlop]

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.

[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 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(...).

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