Consider adding unnecessary_statements to recommended

[natebosch]

This lint does tend to help catch mistakes, but it has a small false positive rate because there are legitimate cases to call a getter for it's side effects.

We had some discussion about an annotation to mark getters as having side effects to suppress this lint, but nothing has landed.
dart-lang/sdk#35513

It seems like the current practice is to use // ignore: comments, and we've so far avoided adding lints with known false positives that can't be suppressed in a nicer way than an ignore.

[lrhn]

The lint is under-documented, so I have no idea what it actually triggers on.

tear-offs are known to not have side-effects. Anything that could be a constant expression too.
Everything else might have side-effects, unless the lint has special knowledge about specific types and methods.

(There are a lot of functions whose primary purpose is to create a new object. If that object isn't used, the call is useless, but we cannot tell that from method signatures. And the traditional iterable.map(...side-effects..).toList() would trigger, since toList itself is just creating an object.)

I need more documentation and specification before I'll consider the lint.

[natebosch]

tear-offs are known to not have side-effects. Anything that could be a constant expression too.
Everything else might have side-effects, unless the lint has special knowledge about specific types and methods.

Mistaken tear-offs are a big motivator for getting this lint into the recommended set. If we think we could define a more narrow lint without false positives, and recommend that one, I'd be in favor. I think we could also flag localVariable; as long as it's not late.

@pq WDYT about looking for a second no-false-positive version of this lint?

I think the original version is likely still useful for folks using it already. IIRC getters were a bigger motivator than tear-offs.

[pq]

If we can increase adoption, I'm up for a variation on this lint.

On the other hand, while this is true

Everything else might have side-effects

I wonder, in practice, how much of an issue this is. Depending on the side-effects of an (otherwise) apparently unnecessary statement seems like a code smell to me.

[natebosch]

Depending on the side-effects of an (otherwise) apparently unnecessary statement seems like a code smell to me.

Yes, I agree it's a code smell, and the existing lint is worthy for consideration in a "strict" set of rules. I don't think the current rule is feasible in a recommended set, but a more narrow rule shouldn't be controversial.

[lrhn]

I think there is room for two lints here: A "this code definitely does nothing" and a "this code looks like it does nothing".
Maybe the first should be "this operation itself does nothing, even though subexpressions might".

The first should have no false positives. It'd probably be something like an expression in a context where the value isn't used, which:

• Is a constant
• Is a method/function/constructor tear-off
• Reads a non-late local variable.
• Is a list/set/map literal containing only expressions which do nothing. (Even though map/set literals might call == and hashCode, let's assume those are not doing weird stuff.)

All of these create or reference a value, with no possible side effect, and then throws the value away again.

If we allow subexpressions to have effects, it would would include any map/set/list literal, even if elements have side-effects. The allocation itself is unnecessary.

The second lint would look at expected behavior for certain operations, and try to enforce that the syntax matches the expected semantics, not just the actual semantics. It would include:

• Any getter invocation.
• Any object construction.
• Possibly almost any unary or binary operator. Those are expected to create a new value, not mutate operands.

And we can include extra class-specific knowledge about methods with no (expected) side effects:

• Any operation on a num or BigInt.
• Calling the Object methods ==, hashCode or runtimeType, toString
• Calling any Iterable method, and any other non-mutating List/Set/Map/Queue operation.

Those are operations where some can be used for side-effects, but you shouldn't. Your code looks like it does nothing, so it should do nothing. If it does nothing, you should remove it. If it does something, you should rewrite it to look like it does something, so it's not misleading readers.

[devoncarew]

This lint will likely need more investigation. Generally, we would be in favor of a version of this lint with no false positives (flagging unnecessary statements where the code for the statement should generally be side-effect free).