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

domesticmouse · 2021-10-21T06:01:14Z

Hey @pq,

This is the start of a lint for SizedBox.shrink(...) and SizedBox.expand(...). When finished, this will fix dart-lang/sdk#58152. I'm using #2049 as the starting point.

However, I'm hitting an issue with double.infinity and I can't see how to patch it into the mock_packages.

$ dart test -N sized_box_shrink_expand
00:03 +0 -1: test/rule_test.dart: rule dart sized_box_shrink_expand [E]                                                                                                        
  Unexpected diagnostics:
  
  test_data/rules/sized_box_shrink_expand.dart 55:20 The getter 'infinity' isn't defined for the type 'double'.
  test_data/rules/sized_box_shrink_expand.dart 23:19 The getter 'infinity' isn't defined for the type 'double'.
  test_data/rules/sized_box_shrink_expand.dart 39:19 The getter 'infinity' isn't defined for the type 'double'.
  test_data/rules/sized_box_shrink_expand.dart 22:20 The getter 'infinity' isn't defined for the type 'double'.
  package:test_api           fail
  test/rule_test.dart 249:7  testRule.<fn>
  
00:03 +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'.

Halp?

coveralls · 2021-10-21T06:04:33Z

Coverage decreased (-0.009%) to 94.149% when pulling bd4a9d4 on domesticmouse:sized_box_shrink_expand into 7058a80 on dart-lang:master.

bwilkerson

Looks like you're headed in the right direction. Hope you don't mind the early review.

lib/src/rules/sized_box_shrink_expand.dart

bwilkerson · 2021-10-21T15:47:17Z

lib/src/rules/sized_box_shrink_expand.dart

+  @override
+  void visitArgumentList(ArgumentList node) {
+    for (var name in node.arguments
+        .cast<NamedExpression>()


It isn't safe to assume that all of the arguments are named expressions. The lints are run over code that's invalid, so you'll need to handle the case where the user didn't provide a name (SizedBox(0, child: child)).

You probably want to treat such cases as 'incompatible' parameters to keep the lint from firing on invalid code.

test_data/rules/sized_box_shrink_expand.dart

lib/src/rules/sized_box_shrink_expand.dart

pq · 2021-10-21T18:38:24Z

However, I'm hitting an issue with double.infinity and I can't see how to patch it into the mock_packages.

This will have to be updated in the analyzer package (which has our mocked SDK). I'll tackle that but it may be a bit before a new analyzer is published.

In the meantime, maybe you can get by with double.INFINITY? (That does seem to be there.)

See: dart-archive/linter#3039 Change-Id: If0a2a02591a0a46c0c463ff2e79f35a1209dff35 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/217700 Commit-Queue: Konstantin Shcheglov <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>

domesticmouse · 2021-10-22T01:41:56Z

In the meantime, maybe you can get by with double.INFINITY? (That does seem to be there.)

Ok, with that work around it looks like I'm now getting to the point where the tests are failing due to the lint not being correctly implemented =)

lib/src/rules/sized_box_for_whitespace.dart

lib/src/rules/sized_box_shrink_expand.dart

lib/src/rules/sized_box_for_whitespace.dart

bwilkerson · 2021-10-25T15:26:08Z

lib/src/rules/sized_box_shrink_expand.dart

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


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

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

Maybe something that suggests this is for readability purposes?

/fyi @goderbauer @Hixie who might have opinions.

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

lib/src/rules/sized_box_shrink_expand.dart

pq

Sorry for the slow replies!

pq · 2021-10-21T18:33:03Z

lib/src/rules/sized_box_shrink_expand.dart

+**Examples**
+
+**BAD:**
+```


Consider adding the dart language tag to these code blocks.

=>

```dart

lib/src/rules/sized_box_shrink_expand.dart

pq · 2021-10-27T13:26:17Z

test_data/rules/sized_box_shrink_expand.dart

+Widget sizedBoxWithInfiniteWidthInfiniteHeight() {
+  return SizedBox(
+    // LINT
+    height: double.INFINITY,


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

double.infinity

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?

Should be already available in analyzer 2.7.0

pq · 2021-10-27T13:26:29Z

test_data/rules/sized_box_shrink_expand.dart

+  return SizedBox(
+    // OK
+    height: 0,
+    width: double.INFINITY,


pq · 2021-10-27T13:26:35Z

test_data/rules/sized_box_shrink_expand.dart

+Widget sizedBoxWithZeroWidthInfiniteHeight() {
+  return SizedBox(
+    // OK
+    height: double.INFINITY,


pq · 2021-10-27T13:26:50Z

test_data/rules/sized_box_shrink_expand.dart

+Widget sizedBoxWithInfiniteWidth() {
+  return SizedBox(
+    // OK
+    width: double.INFINITY,


pq · 2021-10-27T13:26:58Z

test_data/rules/sized_box_shrink_expand.dart

+Widget sizedBoxWithInfiniteHeight() {
+  return SizedBox(
+    // OK
+    height: double.INFINITY,


pq · 2021-10-27T13:28:33Z

lib/src/rules/sized_box_shrink_expand.dart

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


Maybe something that suggests this is for readability purposes?

/fyi @goderbauer @Hixie who might have opinions.

bwilkerson

It's a little hard to follow the conversation at this point. Are you still seeing test failures?

bwilkerson · 2021-10-27T15:39:16Z

lib/src/rules/sized_box_shrink_expand.dart

+        (argument.propertyName.name == 'infinity' ||
+            argument.propertyName.name == 'INFINITY')) {
+      var target = argument.target;
+      if (target is SimpleIdentifier && target.name == 'double') {


It's a corner case that will probably never come up, and a bug that came from me, but if a user imports dart:core using a prefix, then the target will be a PrefixedIdentifier. If you care about catching this case you can add an else if for it.

Added a condition to the if for PrefixedIdentifier version of double.

bwilkerson · 2021-10-27T15:47:46Z

test_data/rules/sized_box_shrink_expand.dart

+
+Widget sizedBoxWithZeroWidthZeroHeight() {
+  return SizedBox(
+    // LINT


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.

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

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.

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.

…ed_box_shrink_expand

domesticmouse · 2021-10-29T04:22:58Z

It's a little hard to follow the conversation at this point. Are you still seeing test failures?

Tests are green now

domesticmouse · 2021-10-30T23:19:40Z

Now that I kinda understand how lints work, I'm curious how I can add dart fix fixes for these two lints?

bwilkerson · 2021-10-31T04:05:12Z

The doc https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/doc/tutorial/quick_fix.md should have most of the information you need. Please let me know if there's information that you would have found useful that's missing from the current state of the doc.

pq

LGTM! Unless @bwilkerson had any unresolved concerns?

bwilkerson

Happy to see it land as is.

bwilkerson · 2021-11-01T17:49:58Z

lib/src/rules/sized_box_for_whitespace.dart

+        positionalArgumentFound = true;
+        return;
+      }
+      if (argument.name.label.name == 'width') {


The code is fine as is, but I'll point out that there's a lot of repetition of argument.name.label.name, so it might be a little cleaner to assign it to a local variable (here and below).

pq · 2021-11-02T13:39:53Z

Thanks all for the thoughtful back and forth. So great to see this land!

🦅

…ive/linter#3039) * Start of a lint for `SizedBox.shrink(...)` and `SizedBox.expand(...)` * Copyright year * Add test cases * double.infinity -> double.INFINITY * next step * next step * Add single backquotes to diagnostic messages * Add description * use static LintCodes * tidyup * Check for positional arguments * Define use case for SizedBox.shrink and SizedBox.expand * Drop the SimpleAstVisitor * Dart language tags and backticks * Add in PrefixedIdentifier for double * Move `// LINT` comment to the right line * double.INFINITY -> double.infinity * Updated to reflect reality * Introduce local var for argument.name.label

Start of a lint for SizedBox.shrink(...) and SizedBox.expand(...)

fa50b93

domesticmouse marked this pull request as draft October 21, 2021 06:01

google-cla bot added the cla: yes label Oct 21, 2021

bwilkerson reviewed Oct 21, 2021

View reviewed changes

domesticmouse added 3 commits October 22, 2021 12:09

Copyright year

7783d72

Add test cases

18fbbee

double.infinity -> double.INFINITY

3c4b01e

domesticmouse added 2 commits October 25, 2021 13:21

next step

ad2cd87

next step

cb49098

domesticmouse commented Oct 25, 2021

View reviewed changes

lib/src/rules/sized_box_for_whitespace.dart Show resolved Hide resolved

domesticmouse commented Oct 25, 2021

View reviewed changes

lib/src/rules/sized_box_shrink_expand.dart Outdated Show resolved Hide resolved

domesticmouse requested a review from bwilkerson October 25, 2021 05:12

This comment has been minimized.

Sign in to view

bwilkerson reviewed Oct 25, 2021

View reviewed changes

domesticmouse added 7 commits October 27, 2021 13:29

Add single backquotes to diagnostic messages

7949bac

Add description

a23896f

use static LintCodes

52afbec

tidyup

33878df

Check for positional arguments

33e2474

Define use case for SizedBox.shrink and SizedBox.expand

0b7f972

Drop the SimpleAstVisitor

cbced5f

This comment has been minimized.

Sign in to view

pq reviewed Oct 27, 2021

View reviewed changes

bwilkerson reviewed Oct 27, 2021

View reviewed changes

domesticmouse added 2 commits October 28, 2021 07:10

Dart language tags and backticks

a100c3e

Add in PrefixedIdentifier for double

975f8f4

domesticmouse added 3 commits October 28, 2021 07:49

Move // LINT comment to the right line

03ee685

Merge branch 'master' of https://github.com/dart-lang/linter into siz…

bd3582c

…ed_box_shrink_expand

double.INFINITY -> double.infinity

e308807

domesticmouse marked this pull request as ready for review October 28, 2021 03:55

domesticmouse added 2 commits October 29, 2021 13:33

Merge branch 'master' of https://github.com/dart-lang/linter into siz…

48fa245

…ed_box_shrink_expand

Updated to reflect reality

e4f0b0b

domesticmouse changed the title ~~Start of a lint for SizedBox.shrink(...) and SizedBox.expand(...)~~ Lint for SizedBox.shrink(...) and SizedBox.expand(...) Nov 1, 2021

pq approved these changes Nov 1, 2021

View reviewed changes

bwilkerson approved these changes Nov 1, 2021

View reviewed changes

Introduce local var for argument.name.label

bd4a9d4

pq merged commit 2e1800e into dart-archive:master Nov 2, 2021

domesticmouse deleted the sized_box_shrink_expand branch November 3, 2021 11:07

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

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

Uh oh!

Conversation

domesticmouse commented Oct 21, 2021

Uh oh!

coveralls commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwilkerson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pq commented Oct 21, 2021

Uh oh!

domesticmouse commented Oct 22, 2021

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

pq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheglov Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

coveralls commented Oct 21, 2021 •

edited

Loading

scheglov Oct 27, 2021 •

edited

Loading