Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

Lint for SizedBox.shrink(...) and SizedBox.expand(...) #3039

Merged
merged 21 commits into from
Nov 2, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions example/all.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ linter:
- recursive_getters
- require_trailing_commas
- sized_box_for_whitespace
- sized_box_shrink_expand
- slash_for_doc_comments
- sort_child_properties_last
- sort_constructors_first
Expand Down
2 changes: 2 additions & 0 deletions lib/src/rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ import 'rules/public_member_api_docs.dart';
import 'rules/recursive_getters.dart';
import 'rules/require_trailing_commas.dart';
import 'rules/sized_box_for_whitespace.dart';
import 'rules/sized_box_shrink_expand.dart';
import 'rules/slash_for_doc_comments.dart';
import 'rules/sort_child_properties_last.dart';
import 'rules/sort_constructors_first.dart';
Expand Down Expand Up @@ -348,6 +349,7 @@ void registerLintRules({bool inTestMode = false}) {
..register(RecursiveGetters())
..register(RequireTrailingCommas())
..register(SizedBoxForWhitespace())
..register(SizedBoxShrinkExpand())
..register(SlashForDocComments())
..register(SortChildPropertiesLast())
..register(SortConstructorsFirst())
Expand Down
25 changes: 12 additions & 13 deletions lib/src/rules/sized_box_for_whitespace.dart
Original file line number Diff line number Diff line change
Expand Up @@ -74,26 +74,20 @@ class _Visitor extends SimpleAstVisitor {
return;
}

var visitor = _WidthOrHeightArgumentVisitor();
node.visitChildren(visitor);
if (visitor.seenIncompatibleParams) {
var data = _ArgumentData(node.argumentList);

if (data.seenIncompatibleParams) {
return;
}
if (visitor.seenChild && (visitor.seenWidth || visitor.seenHeight) ||
visitor.seenWidth && visitor.seenHeight) {
if (data.seenChild && (data.seenWidth || data.seenHeight) ||
data.seenWidth && data.seenHeight) {
rule.reportLint(node.constructorName);
}
}
}

class _WidthOrHeightArgumentVisitor extends SimpleAstVisitor<void> {
var seenWidth = false;
var seenHeight = false;
var seenChild = false;
var seenIncompatibleParams = false;

@override
void visitArgumentList(ArgumentList node) {
class _ArgumentData {
_ArgumentData(ArgumentList node) {
for (var name in node.arguments
.cast<NamedExpression>()
.map((arg) => arg.name.label.name)) {
Expand All @@ -110,4 +104,9 @@ class _WidthOrHeightArgumentVisitor extends SimpleAstVisitor<void> {
}
}
}

var seenWidth = false;
var seenHeight = false;
var seenChild = false;
var seenIncompatibleParams = false;
}
147 changes: 147 additions & 0 deletions lib/src/rules/sized_box_shrink_expand.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
// 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.

import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';

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

const _sizedBoxShrinkDescription =
r'Use the SizedBox.shrink(...) named constructor.';

const _sizedBoxExpandDescription =
r'Use the SizedBox.expand(...) named constructor';

const _details =
r'''Use SizedBox.shrink and SizedBox.expand constructors appropriately.

The `SizedBox.shrink(...)` and `SizedBox.expand(...)` constructors should be used
instead of the more general `SizedBox(...)` constructor for specific use cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider defining / describing the use cases for which this lint applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took a first swing, unsure how I feel about it tho

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something that suggests this is for readability purposes?

/fyi @goderbauer @Hixie who might have opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe replace the generic "for specific use cases" with "to improve readability"?


**Examples**

**BAD:**
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding the dart language tag to these code blocks.

=>

```dart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Widget buildLogo() {
return SizedBox(
height: 0,
width: 0,
child:const MyLogo(),
);
}
```
```
Widget buildLogo() {
return SizedBox(
height: double.infinity,
width: double.infinity,
child:const MyLogo(),
);
}
```

**GOOD:**
```
Widget buildLogo() {
return SizedBox.shrink(
child:const MyLogo(),
);
}
```
```
Widget buildLogo() {
return SizedBox.expand(
child:const MyLogo(),
);
}
```
''';

class SizedBoxShrinkExpand extends LintRule {
SizedBoxShrinkExpand()
: super(
name: 'sized_box_shrink_expand',
description: '',
details: _details,
group: Group.style) {
lintCode = LintCode(name, 'Unused');
}

@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
var visitor = _Visitor(this);

registry.addInstanceCreationExpression(this, visitor);
}

@override
late LintCode lintCode;
}

class _Visitor extends SimpleAstVisitor {
final SizedBoxShrinkExpand rule;

_Visitor(this.rule);

@override
void visitInstanceCreationExpression(InstanceCreationExpression node) {
// Only interested in the default constructor for the SizedBox widget
if (!isExactWidgetTypeSizedBox(node.staticType) ||
node.constructorName.name != null) {
return;
}

var data = _ArgumentData(node.argumentList);
if (data.width == 0 && data.height == 0) {
rule.lintCode = LintCode(rule.name, _sizedBoxShrinkDescription);
rule.reportLint(node.constructorName);
} else if (data.width == double.infinity &&
data.height == double.infinity) {
rule.lintCode = LintCode(rule.name, _sizedBoxExpandDescription);
rule.reportLint(node.constructorName);
}
}
}

class _ArgumentData {
_ArgumentData(ArgumentList node) {
for (var argument in node.arguments.cast<NamedExpression>()) {
if (argument.name.label.name == 'width') {
var argumentVisitor = _ArgumentVisitor();
argument.expression.visitChildren(argumentVisitor);
width = argumentVisitor.argument;
} else if (argument.name.label.name == 'height') {
var argumentVisitor = _ArgumentVisitor();
argument.expression.visitChildren(argumentVisitor);
height = argumentVisitor.argument;
}
}
}

double? width;
double? height;
}

class _ArgumentVisitor extends SimpleAstVisitor {
double? argument;

@override
visitIntegerLiteral(IntegerLiteral node) {
argument = node.value!.toDouble();
}

@override
visitDoubleLiteral(DoubleLiteral node) {
argument = node.value;
}

@override
visitSimpleIdentifier(SimpleIdentifier node) {
if (node.name.toLowerCase() == 'infinity') {
argument = double.infinity;
}
}
}
10 changes: 10 additions & 0 deletions lib/src/util/flutter_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ bool isExactWidget(ClassElement element) => _flutter.isExactWidget(element);
bool isExactWidgetTypeContainer(DartType? type) =>
_flutter.isExactWidgetTypeContainer(type);

bool isExactWidgetTypeSizedBox(DartType? type) =>
_flutter.isExactWidgetTypeSizedBox(type);

bool isKDebugMode(Element? element) => _flutter.isKDebugMode(element);

bool isStatefulWidget(ClassElement? element) =>
Expand All @@ -56,16 +59,19 @@ class _Flutter {
static const _nameStatefulWidget = 'StatefulWidget';
static const _nameWidget = 'Widget';
static const _nameContainer = 'Container';
static const _nameSizedBox = 'SizedBox';

final String packageName;
final String widgetsUri;

final Uri _uriBasic;
final Uri _uriContainer;
final Uri _uriFramework;
final Uri _uriFoundation;

_Flutter(this.packageName, String uriPrefix)
: widgetsUri = '$uriPrefix/widgets.dart',
_uriBasic = Uri.parse('$uriPrefix/src/widgets/basic.dart'),
_uriContainer = Uri.parse('$uriPrefix/src/widgets/container.dart'),
_uriFramework = Uri.parse('$uriPrefix/src/widgets/framework.dart'),
_uriFoundation = Uri.parse('$uriPrefix/src/foundation/constants.dart');
Expand Down Expand Up @@ -99,6 +105,10 @@ class _Flutter {
type is InterfaceType &&
_isExactWidget(type.element, _nameContainer, _uriContainer);

bool isExactWidgetTypeSizedBox(DartType? type) =>
type is InterfaceType &&
_isExactWidget(type.element, _nameSizedBox, _uriBasic);

bool isKDebugMode(Element? element) =>
element != null &&
element.name == 'kDebugMode' &&
Expand Down
8 changes: 8 additions & 0 deletions test_data/mock_packages/flutter/lib/src/widgets/basic.dart
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ class Row extends Flex {
});
}

class SizedBox extends SingleChildRenderObjectWidget {
const SizedBox({Key? key, double? width, double? height, Widget? child});
const SizedBox.expand({Key? key, Widget? child});
const SizedBox.shrink({Key? key, Widget? child});
SizedBox.fromSize({Key? key, Widget? child, Size? size});
const SizedBox.square({Key? key, Widget? child, double? dimension});
}

class Transform extends SingleChildRenderObjectWidget {
const Transform({
Key key,
Expand Down
85 changes: 85 additions & 0 deletions test_data/rules/sized_box_shrink_expand.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
// 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.

// test w/ `dart test -N sized_box_shrink_expand`

import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';

Widget sizedBoxWithZeroWidthZeroHeight() {
return SizedBox(
// LINT
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the 'LINT' marker needs to be on the same line as that on which the lint will be reported. The lint is currently (correctly IMO) reported on the name of the class on line 11, so I think this marker needs to be moved onto line 11. Similarly below. This probably isn't what's causing the current test failure, but I suspect it will cause a different form of test failure after the current failure is fixed.

The 'OK' markers won't cause that problem, but should probably be moved up to the preceding line for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that the dart format tooling is automatically moving the // LINT comment to the next line. I'll edit it with vi =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even with this change, I'm still seeing a failure.

$ dart test -N sized_box_shrink_expand
00:05 +0: test/rule_test.dart: rule dart sized_box_shrink_expand                                                                 

/Users/brettmorgan/Documents/GitHub/linter/test_data/rules/sized_box_shrink_expand.dart 11:10 [lint] Use the `SizedBox.shrink` constructor.
  return SizedBox( // LINT
         ^^^^^^^^

null files analyzed, 1 issue found, in null ms.

00:05 +0 -1: test/rule_test.dart: rule dart sized_box_shrink_expand [E]                                                          
  Expected: matches [Annotation:<[LINT]: "null" (line: 11) - [null:null]>, Annotation:<[LINT]: "null" (line: 19) - [null:null]>] unordered
    Actual: [
              Annotation:[LINT]: "Use the `SizedBox.shrink` constructor." (line: 11) - [10:8]
            ]
     Which: has too few elements (1 < 2)
  
  package:test_api           expect
  test/rule_test.dart 254:7  testRule.<fn>
  
00:05 +0 -1: loading test/rule_test.dart                                                                                         
Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'dart test --chain-stack-traces'.
00:05 +0 -1: Some tests failed.                                                                                                  

Copy link
Contributor

Choose a reason for hiding this comment

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

I consider the formatter's behavior to be a bug, so I opened dart-lang/dart_style#1070.

But I'm not surprised that it's still failing. I mentioned the placement of the comments because I suspect that the test code first verifies that the correct number of lints were produced, and then verifies that each of the lints was in the expected location. With the markers in the wrong places, fixing the number of lints would cause the second failure to occur.

It appears that the missing lint is the one on line 19 (the double.infinity case). I don't see any obvious reason when I look at the code, though my guess would be that the bug is in the code that's computing the value of the argument. I'd try setting a breakpoint in that method (after commenting out all the other cases so there's less noise) and debugging the lint.

height: 0,
width: 0,
child: Container(),
);
}

Widget sizedBoxWithInfiniteWidthInfiniteHeight() {
return SizedBox(
// LINT
height: double.INFINITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

With @scheglov's updates to the mock SDK, I think you'll want to bump these to:

double.infinity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With @scheglov's updates to the mock SDK, I think you'll want to bump these to:

double.infinity

Any idea when this will land? Or should I commit this file with a TODO?

Copy link
Contributor

@scheglov scheglov Oct 27, 2021

Choose a reason for hiding this comment

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

Should be already available in analyzer 2.7.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

width: double.INFINITY,
child: Container(),
);
}

Widget sizedBoxWithInfiniteWidthzeroHeight() {
return SizedBox(
// OK
height: 0,
width: double.INFINITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

And here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

child: Container(),
);
}

Widget sizedBoxWithZeroWidthInfiniteHeight() {
return SizedBox(
// OK
height: double.INFINITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

And here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

width: 0,
child: Container(),
);
}

Widget sizedBoxWithMixedWidthsAndHeights() {
return SizedBox(
// OK
height: 26,
width: 42,
child: Container(),
);
}

Widget sizedBoxWithZeroWidth() {
return SizedBox(
// OK
width: 0,
child: Container(),
);
}

Widget sizedBoxWithInfiniteWidth() {
return SizedBox(
// OK
width: double.INFINITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

child: Container(),
);
}

Widget sizedBoxWithZeroHeight() {
return SizedBox(
// OK
height: 0,
child: Container(),
);
}

Widget sizedBoxWithInfiniteHeight() {
return SizedBox(
// OK
height: double.INFINITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

child: Container(),
);
}