proposal: `avoid_unnecessary_gesture_detectors`

navaronbracke commented 8 months ago

avoid_unnecessary_gesture_detectors

I propose the following new lint rule, to avoid a pitfall with GestureDetectors.

Description

Do not create GestureDetectors without gesture handlers.

Details

Given a GestureDetector widget from package:flutter/widgets.dart, it is redundant if it has no handlers set at all.

Kind

It does guard against errors somewhat. I.e. placing redundant widgets, or when a GestureDetector that used to do something, now does nothing because its last handler was removed.

Bad Examples

GestureDetector(child: const SizedBox(width: 200, height: 200))

or even special casing the empty function (){}, if possible without side effects. Using a (){} is sometimes used as a placeholder so I'm not sure about that.

GestureDetector(
  onTap: () {},
  child: const SizedBox(width: 200, height: 200),
)

Good Examples

void _someVoidFunction() {}

GestureDetector(
  onTap: _someVoidFunction,
  child: const SizedBox(width: 200, height: 200),
)

Discussion

It somewhat overlaps with https://dart.dev/tools/linter-rules/avoid_unnecessary_containers but they target different widgets.

I originally filed a proposal here: https://github.com/flutter/flutter/issues/99686

A real-world use case would have been some code I wrote a while back. It had a GestureDetector wrapping around a very large child definition, which made me miss the fact that it had no handlers. I was under the assumption of "This widget has a GestureDetector, why isn't it doing anything?", and it took me a bit to figure it out (in part due to the large widget tree obscuring my view somewhat).

Discussion checklist

[x] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
[x] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
[x] If there's any prior art (e.g., in other linters), please add references here.
[x] If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
[x] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

bwilkerson commented 8 months ago

@goderbauer

goderbauer commented 8 months ago

It's true that an "empty" gesture detector is unnecessary. Since this is the second example of this pattern after avoid_unnecessary_containers I wonder if there is a more generic way to express this instead of creating specialist lints for each of these widgets?

bwilkerson commented 8 months ago

We could potentially add one or more annotations that would let us cover this case.

@pq was looking at a similar situation awhile back. We had a client that wanted to be able to express things like "if this argument is provided then this other argument also has to be provided", or "these two arguments can't both be provided". I believe that the effort was dropped.

It got a bit messy because of all the combinations they wanted to have support for, but we could think about whether it's enough to be able to say something like "at least one of these arguments must be provided".

srawlins commented 8 months ago

That would be https://github.com/dart-lang/linter/issues/3063 and https://github.com/dart-lang/sdk/issues/47712. I think each of those discussions ground to a halt. I think it's been pointed out that this or that proposal has a downside so it shouldn't be done. Lots of opinions. We should just pick something and implement it, IMO.

dart-lang / linter