Warn when returned value is not referenced.

[Korporal]

Can a warning be added to the compiler so that when a caller does not refer to the returned value from a function that is not void, a warning is issued?

Or if that's not deemed a good idea (because of possibly breaking existing clean builds) then consider adding a new compiler option to warn about unused return values, that would be entirely safe and allow developers who care about this to be able to detect it just by setting the option.

Thx

[CyrusNajmabadi]

@Korporal You can definitely create your own analyzer for precisely this purpose.

[jmagaram]

With a functional coding style and immutable data structures, it is pretty easy to forget to use a return value. How I used to do it...

var x = new List<string>();
x.Add("something");

How I do it now...

var x = ImmutableList<string>.Empty;
x.Add("something"); // I accidentally write this because of years of doing it the mutable way.
x = x.Add("something"); // What I should have written

So I agree it would be useful to be warned about this kind of thing. If the compiler/language was smart enough to identity pure functions, then it could even be an error.

[DavidArno]

@Korporal,

Introducing a warning for this would be a breaking change, as many folk set "treat warnings as errors", so this warning could cause code to stop compiling.

To clarify, I think this would be a good thing, but the language team really do not like breaking changes. The solution, as @CyrusNajmabadi says, is to write your own Roslyn analyzer that reports an error/warning in such cases.

[yaakov-h]

What about "warning waves", which have been mentioned a few times?

Thiugh this is more of a compiler issue than a language one, IMO.

[DavidArno]

@yaakov-h,

Adding it to a warning wave would make total sense to me. The question is, will those warning waves ever happen?

[yaakov-h]

@DavidArno ¯\(ツ)/¯

[Korporal]

@DavidArno - I agree that its a breaking change, that's why I also said making it a compiler option would address that concern. Only specifying the new option in the project settings would cause the warning to be applied.

[Korporal]

@jmagaram

With a functional coding style and immutable data structures, it is pretty easy to forget to use a return value. How I used to do it...

Exactly, I've always regarded this language "capability" which exists in C (and may even predate that language) as appalling laxity.

In fact in F# (if I'm not mistaken) it's not possible to do this and since C# is embracing more and more functional features this is one that's overdue, adding a compiler option for this would resolve the "breaking change" concern too.

[DavidArno]

@Korporal,

Only specifying the new option in the project settings would cause the warning to be applied.

That is the idea behind those "warning waves". They'll be new sets of warnings that help improve code quality, but which are off by default, and must be opted into, to prevent breaking existing code.

[hgleaves-ncuadmin]

@DavidArno - I see, well I've never heard of this concept of "warning waves" so can't really comment. Is this too, a proposed change?

[DavidArno]

@hgleaves-ncuadmin,

Please see roslyn #1580 for details. @gafter hasn't yet copied it to this repo as far as I know.

[svick]

@Korporal

In fact in F# (if I'm not mistaken) it's not possible to do this

It is, but it gives a warning, e.g.:

warning FS0020: This expression should have type 'unit', but has type 'int'. Use 'ignore' to discard the result of the expression, or 'let' to bind the result to a name.

As the warning indicates, the idiomatic way to ignore a return value in F# is to use something like f() |> ignore. I guess in C# 7 it would be _ = f();?

[CyrusNajmabadi]

I don't see this ever being in a warning wave unless also paired with some attribute for users to opt into this behavior. Warning waves are for things we think are certainly bugs, but which we can't turn on for back compat reasons. For example, having branching code with conditions that always evaluate to true/false.

In this case, there are lots of times when you may not care about the value of a return type, and so warning in those cases would be something that would actually block people from moving to this warning wave.

However, if this was paired with something like a "Pure" attribute, it would be a way of saying "actually, this method has no purpose except to give you a resultant value. If you don't observe that value, it's almost certainly the case that your code is buggy".

There is already a Pure attribute in the Contracts API i believe. So i would recommend just using that and writing an analyzer that checks for that and warns.

[DavidArno]

@CyrusNajmabadi,

Yes, except it should not be for the developer to mark a method with [Pure]. That sort of thing can be done by the compiler with a far higher degree of accuracy: it it determined that the method was semantically pure, it could auto-add such an attribute.

[CyrusNajmabadi]

That leads to spooky action from a distance. User changes the internals of some method. Now the the compiler errors some further place. We do not like code that does that.

Now, it would certainly be fine to have some sort of analyzer that recommended 'Pure' when it could be determined that that was the case. Users could then choose to apply that attribute and would get the checking.

[DavidArno]

Hmm, hadn't considered "action from a distance" problems. I agree, that would not be good. So yes, hints that method could be marked [Pure] and warnings that a method is incorrectly marked thus would be useful additions.

However, I'm not sure that analysers are the right place for such functionality. I may have got this wrong, but I thought semantic analysis like this is better handled by the compiler. I don't have any details, it just rings a bell as something I read recently.

[tannergooding]

@DavidArno, I think one of the issues with the compiler auto-magically marking a method with Pure (or a similar attribute) is that it would likely be a breaking change to modify the method such that it was no longer Pure. Where-as today, you can freely change the internals of your method and not need to worry about it.

However, I would think compiler checking that a method is Pure (if marked as such) would be reasonable. That, couple with an Analyzer to recommend it would probably provide a satisfactory implementation.

[DavidArno]

@tannergooding,

Yep, that's @CyrusNajmabadi's "spooky action from a distance" problem. Having a change to a method's internals break other code that uses that method clearly is a non starter, so that idea is well and truly dumped in the trash.

[AndreasHoffmann2]

Since nobody else mentioned it yet:
With the original proposal, most usages of StringBuilder.Append() would be treated as a warning then.

[DavidArno]

@Epaminaidos

_ = stringBuilder.Append("stuff");

Warning resolved.

Explicitly dealing with unused returns, such as from StringBuilder.Append() is the point to this proposal after all.

[Korporal]

@Epaminaidos - Yes, .Append() would do that, so we agree that enabling this warning would be a breaking change. My motive with the idea is to warn about "our" code, code that we write, this would enable us to state a design/coding policy of not ignoring returns in our own code, i.e. avoid writing methods that return stuff that could be disregarded.

F# of course - being functional - inherently discourages the practice (but as someone else pointed out there is a way to do it) so that run of the mill code pretty much can't accidentally, easily, ignore returned values.

Maybe there's a case for introducing an attribute - class level - that gets leveraged by the compiler, this would allow us to enable the feature on a per-class basis, a bit contrived though - does the compiler currently ever look at attributes when compiling source?

[CyrusNajmabadi]

@Korporal you could accomplish all of that with your own analyzer:-)

[FredyFerrari]

@Korporal or use JetBrains Resharper?

[CyrusNajmabadi]

I'm not certain any traction will be made in this repo. I would recommend moving this thread to Roslyn-analyzers.

[AndreasHoffmann2]

@DavidArno I don't really like the resulting code of this notation, since there are mulitple places where the return-value is ignored more often than not. Adding _ = does not improve the code IMO.
StringBuilder is just one example.
There are several more:
_ = hashSet.Add(1);
_ = list.Remove("a");
_ = sqlCommand.Parameter.AddWithValue("@foo", doo);

IMO, the more sensible approach would be the "PureAttribute" mentioned above.