RFC: lint mode for unused results

[nikomatsakis]

Proposal:

We should have a lint mode that warns when the result of some expression is not used (unless that result is of type unit or bot). Furthermore, this lint more should be set to error by default (or perhaps warning, if people insist, but I generally feel like warnings should be errors by default, 'cause I'm strict like that).

Implications:

Unless the lint mode is disabled, you would have to ignore results explicitly, like this:

let _ = some_function_whose_return_value_I_do_not_want_to_use();

This subsumes #2965. @catamorphism had proposed that we discuss this idea in the other issue, but I chose to make a separate issue for this because I think the title of the other is insufficient, and I wanted the issue to get re-emailed to everyone for discussion.

[nikomatsakis]

Some arguments in favor: there are many cases where ignoring a value is a bug. In trans, for example, ignoring a returned block context is easy to do but almost always constitutes a bug. This in particular is a refactoring hazard: if some function used to return unit because it did not create control flow, but is modified to return a block context, it is easy to fail to find all uses of that function.

Other arguments: a common source of error is ignoring return values that indicate error. Given that we do not have exceptions, such return values will be common, whether they take the form of sentinel values or result<>/option<> types. We should not let them be easily ignored.

Final argument: It encourages the (good, in my opinion) style of having side-effecting functions return unit. In cases, like insert() for hashmaps, where a function is primarily intended for its side effect but also has a useful return, it encourages there to be two functions, one that returns unit and one that does not. This makes for clearer code. I find it very convenient to write if set.insert(...) in order to add to a set, but not especially clear.

[bblum]

strongly in favour.

I feel like warnings are enough - it's not like people don't see them. errors are for, well, things that don't make sense to compile at all. and let _ = is simple enough that it shouldn't be too much of a burden to go fix the warning instead of leaving it to rot.

[jruderman]

This would also keep me from writing a == 4; when i mean a = 4; :)

[jruderman]

Should the opposite also warn? (Writing let _ = foo(); when foo() actually returns unit)

[catamorphism]

Support.

[brson]

The roadmap discusses making unused results always an error, and suggests this will help us get to a place where we can make the tail-expression take an optional semi-colon. I think because we can then assume that the type of the last statement in a block is the type of the block.

[mitsuhiko]

Strongly disagree with this for general functions due to expressions having so many side effects, but I do agree that it would be nice to have a trait that requires you to not ignore the value. For instance Result types should never be ignored because that would mean you can suppress an error. Since there is now trait inheritance that could extend a NonIgnorable trait.

Why would you not make that the default? Because too many things have side effects in the sense that the expression result stays on the stack. Sometimes you might also want to have functions that return something the same instance for chaining and then it is again very annoying if you have to ignore the final return value.

[catamorphism]

I still think a lint mode is more modular than adding to the number of traits that have special meaning to the compiler. Finding that you have to ignore a lot of results explicitly is probably a sign that you've structured your code badly.

[mitsuhiko]

Finding that you have to ignore a lot of results explicitly is probably a sign that you've structured your code badly.

Just look at how many libraries are based on chaining things. That's especially true in C++. Very often functions also return something you might want to use for debug purposes but not under normal circumstances. Do you want to rule out that pattern altogether? Then what's the alternative?

[bblum]

i think it's wrong to attach whether or not to warn to particular types. how would that mix with polyrmorphism? what if a datatype contains a Result inside; do you automatically warn for that too? what if it didn't used to, but you go to put one in later, and suddenly a bunch of warnings appear in unrelated code?

i think this is definitely a job for lint mode attributes. i personally favour having them on by default (writing let _ = foo() is really not that bad, and worth the cost of making you think about your code more), but it would also not be opposed to having them be off by default.

this is going to be a really important feature given that our libraries have both functional and in-place versions of the same functions -- for example, vec::push and vec::append, which i myself have gotten bitten by mixing up in the past.

[nikomatsakis]

@bblum I suppose you're right that it would be complex to try and attach the lint to particular types, though my intention was not to make this a "formal safety" guarantee but rather just a useful one. Still, it'd probably be enough to go with the usual scope-based granularity. We could enforce it for all of trans, for example, since that is the primary place where dropping values frequently causes definite bugs. (Actually we could just enforce the requirement for the entire compiler, of course)

[bblum]

Not entirely sure what that says about the rule you've got in mind. I'm inagining the rule to be "values that are dropped with the semicolon", to supplement "unused variable". That way "foo();" warns, and "let _ = foo();" fixes it.

[thestinger]

I don't think this would be very useful without either a concept of external effects (unlikely, at least for v1.0) or an explicit way of marking a function as warning on an unused result (like gcc and clang implement).

It would be really annoying to force explicitly ignoring the return value of functions like insert and remove on maps.

[graydon]

This is closely related to the reasoning around semi. When we were deciding that rule (if anyone can even state it simply) we felt that the ocaml-experience of having to explicitly ignore non-unit types was clunky and undesirable.

Is this not a reversal of that decision?

[bblum]

A third option would be an annotation that disables (or enables) the warning caller-side, so the user could decide what style they like better. So #[warn_unused_result] fn foo() { let _ = bar(); } would be the stricter version of fn foo() { bar(); } for example. Being able to put that directive at the crate level would be useful for enforcing style guidelines across a whole project.

Not sure where my personal preference lies, as long as there's some way to do it. :P

[pnkfelix]

visited for triage, 16 july 2013. Not sure where I fall in terms of what the default for this should be; I suspect I would prefer it to be "opt-in", in the manner described in bblum's comment.

But the first step would be to actually implement the warning itself, before we worry about whether lint turns it on or off by default, right? Let us get some concrete experience with it?

[metajack]

Since this was mass-assigned a milestone, I'm renominating because I think we can de-milestone this.

[graydon]

revisited, decided to keep on the milestone list. several champions for the feature in 'some form'

[graydon]

accepted for feature-complete milestone

[catamorphism]

Visiting for triage. I would love to implement this in between rustpkg bugs...

[pnkfelix]

nominating for discussion when pcwalton present

[nikomatsakis]

Much as I love this idea, I think at this point we should implement a lint mode that is off by default, and then just turn it on for rustc etc

[pnkfelix]

@alexcrichton landed this or a close approximation thereof in #11754