Fix false positive use_decorated_box and use_colored_box
#3271
Conversation
| // Not lint if decoration is null or nullable because a DecoratedBox | ||
| // only accepts a non-null Decoration | ||
| if (DartTypeUtilities.isNonNullable( | ||
| context, argument.expression.staticType)) { |
There was a problem hiding this comment.
Checking for nullability is good, but we're missing the more important test to ensure that the type of the expression is assignable to BoxDecoration.
If you wouldn't mind fixing that while you're at it, then also add a test to ensure that we don't lint when the decoration argument is some other kind of decoration (maybe a ShapeDecoration).
There was a problem hiding this comment.
A Container accepts a Decoration?, which looks like a deliberate design decision to me. This also doesn't throw an error:
child: Container(decoration: const ShapeDecoration(shape: CircleBorder()))If a Container is supposed to only accept a BoxDecoration, why not just allow the constructor to accept a BoxDecoration instead of a Decoration?
| class Color { | ||
| Color(int value); | ||
| } | ||
| class Color {} |
There was a problem hiding this comment.
You can't do so in this PR because the change has to be made to the analyzer package, but this class really ought to be part of the mock SDK we use for testing. When we do that we'll make the class match the published version of Color, including making the constructor take an argument.
Once it's been added, then this test will need to be converted to using the definition from the mock SDK, and that will require restoring all of the arguments. It would ease that future conversion if you could revert the change to the definition of Color and the invocations of the constructor.
| @@ -20,7 +20,7 @@ Widget containerWithKey() { | |||
|
|
|||
| Widget containerWithDecoration() { | |||
| return Container( // LINT | |||
| decoration: BoxDecoration(), | |||
| decoration: Decoration(), | |||
There was a problem hiding this comment.
I believe that these changes (from BoxDecoration to Decoration) also need to be reverted. It's a false positive to lint here because this code can't use DecoratedBox.
|
Sorry for the year+ delay, @minhqdao . @bwilkerson left some comments, are you able to review them and upload another commit? Thanks! |
srawlins
added
the
needs-info
github-actions
bot
removed
the
needs-info
This PR alters
use_decorated_boxso it doesn't advise switching from aContainerto aDecoratedBoxwhen a nullableDecoration?expression is used because aDecoratedBoxonly accepts a non-nullDecoration.The same is done for
use_colored_box.See discussion: #3239