Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false positive use_decorated_box and use_colored_box #3271

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix false positive use_decorated_box and use_colored_box #3271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f88d53
Fix DecoratedBox
minhqdao Mar 5, 2022
7d74aca
Fix ColoredBox
minhqdao Mar 5, 2022
c40fb11
Remove default constructor
minhqdao Mar 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions lib/src/rules/use_colored_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';

import '../analyzer.dart';
import '../util/dart_type_utilities.dart';
import '../util/flutter_utils.dart';

const _desc = r'Use `ColoredBox`.';
Expand Down Expand Up @@ -49,30 +50,31 @@ class UseColoredBox extends LintRule {
@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
var visitor = _Visitor(this);
var visitor = _Visitor(this, context);

registry.addInstanceCreationExpression(this, visitor);
}
}

class _Visitor extends SimpleAstVisitor {
final LintRule rule;
_Visitor(this.rule, this.context);

_Visitor(this.rule);
final LintRule rule;
final LinterContext context;

@override
void visitInstanceCreationExpression(InstanceCreationExpression node) {
if (!isExactWidgetTypeContainer(node.staticType)) {
return;
}

var data = _ArgumentData(node.argumentList);
var data = _ArgumentData(node.argumentList, context);

if (data.additionalArgumentsFound || data.positionalArgumentsFound) {
return;
}

if (data.hasColor) {
if (data.hasNonNullColor) {
rule.reportLint(node.constructorName);
}
}
Expand All @@ -81,20 +83,25 @@ class _Visitor extends SimpleAstVisitor {
class _ArgumentData {
var positionalArgumentsFound = false;
var additionalArgumentsFound = false;
var hasColor = false;
var hasNonNullColor = false;

_ArgumentData(ArgumentList node) {
_ArgumentData(ArgumentList node, LinterContext context) {
for (var argument in node.arguments) {
if (argument is! NamedExpression) {
positionalArgumentsFound = true;
return;
}
var label = argument.name.label;
if (label.name == 'color') {
hasColor = true;
} else if (label.name == 'child') {
var name = argument.name.label.name;
if (name == 'color') {
// Not lint if color is null or nullable because a ColoredBox only
// accepts a non-null Color
if (DartTypeUtilities.isNonNullable(
context, argument.expression.staticType)) {
hasNonNullColor = true;
}
} else if (name == 'child') {
// Ignore child
} else if (label.name == 'key') {
} else if (name == 'key') {
// Ignore key
} else {
additionalArgumentsFound = true;
Expand Down
31 changes: 19 additions & 12 deletions lib/src/rules/use_decorated_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';

import '../analyzer.dart';
import '../util/dart_type_utilities.dart';
import '../util/flutter_utils.dart';

const _desc = r'Use `DecoratedBox`.';
Expand Down Expand Up @@ -59,30 +60,31 @@ class UseDecoratedBox extends LintRule {
@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
var visitor = _Visitor(this);
var visitor = _Visitor(this, context);

registry.addInstanceCreationExpression(this, visitor);
}
}

class _Visitor extends SimpleAstVisitor {
final LintRule rule;
_Visitor(this.rule, this.context);

_Visitor(this.rule);
final LintRule rule;
final LinterContext context;

@override
void visitInstanceCreationExpression(InstanceCreationExpression node) {
if (!isExactWidgetTypeContainer(node.staticType)) {
return;
}

var data = _ArgumentData(node.argumentList);
var data = _ArgumentData(node.argumentList, context);

if (data.additionalArgumentsFound || data.positionalArgumentsFound) {
return;
}

if (data.hasDecoration) {
if (data.hasNonNullDecoration) {
rule.reportLint(node.constructorName);
}
}
Expand All @@ -91,20 +93,25 @@ class _Visitor extends SimpleAstVisitor {
class _ArgumentData {
var positionalArgumentsFound = false;
var additionalArgumentsFound = false;
var hasDecoration = false;
var hasNonNullDecoration = false;

_ArgumentData(ArgumentList node) {
_ArgumentData(ArgumentList node, LinterContext context) {
for (var argument in node.arguments) {
if (argument is! NamedExpression) {
positionalArgumentsFound = true;
return;
}
var label = argument.name.label;
if (label.name == 'decoration') {
hasDecoration = true;
} else if (label.name == 'child') {
var name = argument.name.label.name;
if (name == 'decoration') {
// Not lint if decoration is null or nullable because a DecoratedBox
// only accepts a non-null Decoration
if (DartTypeUtilities.isNonNullable(
context, argument.expression.staticType)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

hasNonNullDecoration = true;
}
} else if (name == 'child') {
// Ignore child
} else if (label.name == 'key') {
} else if (name == 'key') {
// Ignore key
} else {
additionalArgumentsFound = true;
Expand Down
2 changes: 1 addition & 1 deletion test_data/mock_packages/flutter/lib/painting.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

export 'src/painting/box_decoration.dart';
export 'src/painting/decoration.dart';
2 changes: 1 addition & 1 deletion ...tter/lib/src/painting/box_decoration.dart → .../flutter/lib/src/painting/decoration.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

class BoxDecoration {}
class Decoration {}
50 changes: 41 additions & 9 deletions test_data/rules/use_colored_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';

class Color {
Color(int value);
}
class Color {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Widget containerWithoutArguments() {
return Container(); // OK
Expand All @@ -23,7 +21,7 @@ Widget containerWithKey() {

Widget containerWithColor() {
return Container( // LINT
color: Color(0xffffffff),
color: Color(),
);
}

Expand All @@ -43,21 +41,21 @@ Widget containerWithKeyAndChild() {
Widget containerWithKeyAndColor() {
return Container( // LINT
key: Key('abc'),
color: Color(0xffffffff),
color: Color(),
);
}

Widget containerWithColorAndChild() {
return Container( // LINT
color: Color(0xffffffff),
color: Color(),
child: SizedBox(),
);
}

Widget containerWithKeyAndColorAndChild() {
return Container( // LINT
key: Key('abc'),
color: Color(0xffffffff),
color: Color(),
child: SizedBox(),
);
}
Expand All @@ -70,15 +68,49 @@ Widget containerWithAnotherArgument() {

Widget containerWithColorAndAdditionalArgument() {
return Container( // OK
color: Color(0xffffffff),
color: Color(),
width: 20,
);
}

Widget containerWithColorAndAdditionalArgumentAndChild() {
return Container( // OK
color: Color(0xffffffff),
color: Color(),
width: 20,
child: SizedBox(),
);
}

Widget containerWithNullColor() {
return Container( // OK
color: null,
child: SizedBox(),
);
}

const Color? _nullableColor = null;

Widget containerWithNullableColor() {
return Container( // OK
color: _nullableColor,
child: SizedBox(),
);
}

final _nonNullColor = Color();

Widget containerWithNonNullColor() {
return Container( // LINT
color: _nonNullColor,
child: SizedBox(),
);
}

Color? _getColor() => null;

Widget nullableReturnValue() {
return Container( // OK
color: _getColor(),
child: SizedBox(),
);
}
46 changes: 40 additions & 6 deletions test_data/rules/use_decorated_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Widget containerWithKey() {

Widget containerWithDecoration() {
return Container( // LINT
decoration: BoxDecoration(),
decoration: Decoration(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
}

Expand All @@ -40,21 +40,21 @@ Widget containerWithKeyAndChild() {
Widget containerWithKeyAndDecoration() {
return Container( // LINT
key: Key('abc'),
decoration: BoxDecoration(),
decoration: Decoration(),
);
}

Widget containerWithDecorationAndChild() {
return Container( // LINT
decoration: BoxDecoration(),
decoration: Decoration(),
child: SizedBox(),
);
}

Widget containerWithKeyAndDecorationAndChild() {
return Container( // LINT
key: Key('abc'),
decoration: BoxDecoration(),
decoration: Decoration(),
child: SizedBox(),
);
}
Expand All @@ -67,15 +67,49 @@ Widget containerWithAnotherArgument() {

Widget containerWithDecorationAndAdditionalArgument() {
return Container( // OK
decoration: BoxDecoration(),
decoration: Decoration(),
width: 20,
);
}

Widget containerWithDecorationAndAdditionalArgumentAndChild() {
return Container( // OK
decoration: BoxDecoration(),
decoration: Decoration(),
width: 20,
child: SizedBox(),
);
}

Widget containerWithNullDecoration() {
return Container( // OK
decoration: null,
child: SizedBox(),
);
}

const Decoration? _nullableDecoration = null;

Widget containerWithNullableDecoration() {
return Container( // OK
decoration: _nullableDecoration,
child: SizedBox(),
);
}

final _nonNullDecoration = Decoration();

Widget containerWithNonNullDecoration() {
return Container( // LINT
decoration: _nonNullDecoration,
child: SizedBox(),
);
}

Decoration? _getDecoration() => null;

Widget nullableReturnValue() {
return Container( // OK
decoration: _getDecoration(),
child: SizedBox(),
);
}