Update unnecessary_statements to report tear-offs but not getters

[pq]

In dart-archive/linter#1014, @bwilkerson offered:

Honestly, I think the right solution is to update unnecessary_statements to stop reporting the use of getters and to report the narrower case of tear-offs.

and I agree.

[davidmorgan]

Thanks.

I took a shot at implementing, but I'm not too happy with the result

https://github.com/davidmorgan/linter/blob/tweak-unnecessary-statements/lib/src/rules/unnecessary_statements.dart

I added code for three cases: PrefixExpression, PrefixedIdentifier and SimpleIdentifier. To guesstimate if something is a tearoff I just check if bestType is FunctionType. But this has two problems:

It causes false positives for getters than return functions; I can live with that.
It causes false negatives for nodes of these types that are not accessing getters at all. e.g. myVar where myVar is a local variable; or foo.bar where foo is a class and bar is a field; for foo.bar where foo is an import prefix and bar is a top level field.
Any suggestions for how to improve this, please?

Thanks!

[bwilkerson]

We should be able to improve this by making the conditions more specific. It sounds like we just need to include a condition to see whether the element associated with the node is a getter.

[MichaelRFairhurst]

I'm late to the party but have a few thoughts to add:

This discussion is almost exactly like another one: constructors. I personally don't think this is good code:

new Foo();
return sideEffectOfConstructor;

but it happens more than you'd think, especially in tests when people are asserting that constructors can throw. This reminds me a lot of the reasons why we're removing getters! If we make an unecessary_getters lint, it'd be great to get an unecessary_constructors lint too.

I will throw out one wart here. It's not a problem as currently written but it could maybe? become one.

Currently the lint is careful to lint different branches of a ternary and short-circuited operators:

  x
  ? y
  : null; // lint

It's not necessarily invalid to switch that around to lint a ternary only when both branches are themselves unnecessary.

true ? 1 : 2; // definitely invalid
x ? 1 : 2; // almost certainly invalid
x ? y : 1; // currently invalid, but I could see some people pushing back on this since 1 is realistically a place-holder

Especially when you consider that there are a number of branching operators in dart (.?, ??, &&, ||, ?:), I have a vague feeling that we may get requests for unnecessary_branching, or something like that.

This would become an open doorway to the lints we break out of unecessary_statements starting to interact with each other.

Currently it will be fine:

x // ok
  ? y // lint: unnecessary_getters
  : 1; // lint: unnecessary_statements

but if we wanted to say, make a new lint named unecessary_ternaries, for maybe linting x when y and 1 are unnecessary, or only linting y and 1 when unnecessary_ternaries is turned on, then that new lint would have to know whether unnecessary_getters and unnecessary_statements are both turned on. Or vice versa. There would be interaction between lints which I am assuming is complexity we neither support nor want to get into the business of supporting.

I don't think it will happen. But I think its an interesting risk to splitting apart unecessary_statements too much.