proposal: `move_color_to_decoration`

minhqdao commented 2 years ago

`move_color_to_decoration`

Description

Don't provide Container with both non-null color and decoration. Place color inside decoration instead.

Details

When a Container is provided with both color and decoration (both non-null), an error is thrown at runtime, and there is currently nothing that warns you about that beforehand. This rule prevents you from running into the error by prompting to move color into decoration.

Kind

Guard against errors.

Good Examples

Widget buildArea() {
  return Container(
    decoration: BoxDecoration(
      border: Border.all(color: Colors.white),
      color: Colors.black,
    ),
  );
}

Bad Examples

Widget buildArea() {
  return Container(
    color: Colors.black,
    decoration: BoxDecoration(
      border: Border.all(color: Colors.white),
    ),
  );
}

Discussion

Those who already ran into the error will know, but those who haven't will certainly do, and that's simply annoying. No one wants to dig through unexpected error messages, even if this one is rather easy to identify.

Discussion checklist

[x] List any existing rules this proposal complements, overlaps or conflict with. Slightly complementing: https://github.com/dart-lang/linter/issues/3060 https://github.com/dart-lang/linter/pull/3061
[x] List any relevant issues (reported here, the SDK Tracker, or elsewhere). Not found.
[x] If there's any prior art (e.g., in other linters), please add references here. None.
[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.

pq commented 2 years ago

/fyi @Hixie @goderbauer @HansMuller

goderbauer commented 2 years ago

This seems useful to inform people of an illegal configuration at code write time instead of compile time.

HansMuller commented 2 years ago

I agree, if we can flag this problem early it's a win for developers.

pq commented 2 years ago

This is terrific and addresses a problem that really trips folks up. I looked at this a while ago and recall there are at least a handful of other Flutter classes w/ similar constraints. Doing a quick scan now I see similar patterns in at least: CupertinoSearchTextField, Ink, TextStyle, and AnimatedContainer. Given that, it'd be great to revisit a more general and robust solution.

Using an annotation, for example, we could capture this constraint at the API level.

~Details~

EDIT: removed annotation proposal in favor of centralizing that discussion in https://github.com/dart-lang/sdk/issues/47712

/fyi @bwilkerson @InMatrix @leafpetersen @munificent who were in on the original conversation about annotating exclusive params vs. a language solution like overloading.

/fyi also @natebosch @jakemac53 @lrhn @eernstg for more language team perspective

bwilkerson commented 2 years ago

In terms of using an annotation for this, I think the right next step is to create a new issue (in the sdk repo) to discuss the kinds of information we'd need to capture for the use cases we know about, and then the right set of annotations with which to capture that information. It might well be that OnlyOneOf would meet all of the known needs, but I'd like to know that before we make a decision.

pq commented 2 years ago

@bwilkerson, agreed.

I created https://github.com/dart-lang/sdk/issues/47712 and updated my proposal above to link out to it.

dart-lang / linter