Add new lint avoid_unused_functions.

[davidmorgan]

It is a weaker version of the existing unnecessary_statements lint; it only fails when the expression is a function. This specifically catches cases where the fix is to add parentheses to call the function.

Note: I created this PR by forking the unnecessary_statements lint code and test; it may make sense to refactor to remove the code duplication. But, I thought it would first make sense to see if you agree on the idea of the lint.

When looking at unnecessary_statements we found false positives related to getters with side effects (called to trigger caching, loading, or updating) or getters that might throw (called to check if they throw). The main bug that unnecessary_statements did catch was missing parenthesis. So, this lint is targeted at that precise case.

Thanks!

[a14n]

An other solution could be to extract an avoid_unused_getter lint. unnecessary_statements assumes that getter have no side effects but it's perhaps a little controversal. Thus unnecessary_statements will not handle getters and users will have to opt-in to also check getters. WDYT?

[pq]

@davidmorgan : I'm all for this one.

@bwilkerson: thoughts?

[pq]

I'm also not terribly worried about false positives in the esoteric cases.

On the other hand, the side-effecting getters case that this is accommodating seems a little fishy. I recognize that folks are doing this but I have to ask should they? At least on the surface, that seems like a code smell.

For example, this related idiom in Java drives me personally a little crazy:

    // Initialize Foo.  
    FooMananger.getInstance();

and would much prefer something like:

    FooMananger.initialize();

Sorry for the potential derail!

[bwilkerson]

thoughts?

Almost always :-)

It's often a difficult trade-off between having a single lint that checks a variety of cases and having multiple lints that each check one case. It's easier to find and enable one lint than five, but harder to get the behavior you want if you can't configure lints and you don't like the way one works. The more lints you support, the more difficult it is to configure the system.

This is an interesting example of that problem. It might have been better to have defined multiple lints that each check a different kind of unused statement, but I tend to think we made the right choice in this case.

But I'm convinced that configuration is much harder when you have lints that overlap each other, so I would really like to avoid having a "same as foo except" style of lint.

I'm also not terribly worried about false positives in the esoteric cases.

On the other hand, the side-effecting getters case that this is accommodating seems a little fishy.

I tend to be fairly concerned about false positives, and in this case it's those false positives that are causing us to question whether to create a separate lint rule.

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. That should eliminate the false positives and cover the use case that this lint is trying to cover. If we can later figure out how to catch the use of getters that do not have side-effects, then we can add them back in.

[a14n]

If we can later figure out how to catch the use of getters that do not have side-effects, then we can add them back in.

Or we could add now a new lint unnecessary_getter_access for developers that consider getters should not have side-effects ;)

[bwilkerson]

Yes, that would also work. The two lints would be non-overlapping, which is good. They would be strongly related but separate, which is less than ideal, but possibly the best option.

We can partially mitigate the pain caused by having fine grained lints via documentation. For example, each of the rules could suggest that users consider also enabling the other.

[pq]

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.

I agree. Opened dart-lang/sdk#57718 to track. @davidmorgan: would that work for you?

[davidmorgan]

Thanks everyone!

Sounds good to me. To be clear, we have three classes of error caught by the lint currently:

1. Tear-offs: list.clear; should be list.clear();
2. Getters: list.length; should be doSomething(list.length);
3. Constant expressions; 1 + 1 should be doSomething(1 + 1)

Originally I was proposing to extract #1 into a new lint. Now it sounds like we want to remove #2 from the existing lint. This leaves a difference that we would continue to flag #3.

I actually see this a plus, we want to flag #3. (Although I think it's a less important case).

Did I get that right?

Thanks!

[bwilkerson]

Yes, I believe that's what we've decided.

[davidmorgan]

Thanks; took a shot at implementing, commented over on the issue. Will close this PR and open a new one.