[L] Change in dart/sdk[main]: [analysis_server] add a fix for `use_decorated

Ahmed Ashour (Gerrit)

unread,

Feb 1, 2023, 5:50:36 AM2/1/23

to rev...@dartlang.org

Ahmed Ashour has uploaded this change for review.

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart
10 files changed, 466 insertions(+), 15 deletions(-)

diff --git a/pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
new file mode 100644
index 0000000..538643b
--- /dev/null
+++ b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
@@ -0,0 +1,125 @@
+// Copyright (c) 2023, 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:analysis_server/src/services/correction/dart/abstract_producer.dart';
+import 'package:analysis_server/src/services/correction/fix.dart';
+import 'package:analysis_server/src/services/linter/lint_names.dart';
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
+import 'package:analyzer/error/error.dart';
+import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
+import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
+import 'package:analyzer_plugin/utilities/range_factory.dart';
+
+class ReplaceWithDecoratedBox extends CorrectionProducer {
+  @override
+  bool get canBeAppliedInBulk => true;
+
+  @override
+  bool get canBeAppliedToFile => true;
+
+  @override
+  FixKind get fixKind => DartFixKind.REPLACE_WITH_DECORATED_BOX;
+
+  @override
+  FixKind get multiFixKind => DartFixKind.REPLACE_WITH_DECORATED_BOX_MULTI;
+
+  @override
+  Future<void> compute(ChangeBuilder builder) async {
+    var node = this.node;
+    if (node is! SimpleIdentifier) return;
+    var instanceCreation =
+        node.thisOrAncestorOfType<InstanceCreationExpression>();
+    if (instanceCreation is! InstanceCreationExpression) return;
+
+    var parentInstanceCreation = instanceCreation.parent?.parent?.parent;
+    if (parentInstanceCreation is InstanceCreationExpression &&
+        _hasLint(parentInstanceCreation)) {
+      return;
+    }
+
+    var linterContext = getLinterContext();
+    var deletions = <Token>[];
+    var replacements = <AstNode, String>{};
+
+    // Returns `true` if the specified [expression] can be `const`, considering
+    // similar fixes, which would transform it to `const`.
+    bool canExpressionBeConst(Expression expression) {
+      var canBeConst = linterContext.canBeConst(expression);
+      if (!canBeConst && expression is InstanceCreationExpression) {
+        if (_hasLint(expression)) {
+          canBeConst = true;
+          var childrenConstMap = <InstanceCreationExpression, bool>{};
+          for (var argument in expression.argumentList.arguments) {
+            if (argument is NamedExpression) {
+              var child = argument.expression;
+              var canChildBeConst = canExpressionBeConst(child);
+              canBeConst &= canChildBeConst;
+              if (child is InstanceCreationExpression) {
+                childrenConstMap[child] = canChildBeConst;
+              }
+            } else {
+              // Should not happen, as Container doesn't have positional
+              // arguments.
+              canBeConst = false;
+            }
+          }
+
+          _replaceIfLint(replacements, expression, addConst: canBeConst);
+
+          for (var entry in childrenConstMap.entries) {
+            var child = entry.key;
+            var canChildBeConst = entry.value;
+            _replaceIfLint(replacements, child,
+                addConst: canChildBeConst && !canBeConst);
+            if (canBeConst) {
+              var keyword = child.keyword;
+              if (keyword?.type == Keyword.CONST) {
+                deletions.add(keyword!);
+              }
+            }
+          }
+        }
+      }
+      return canBeConst;
+    }
+
+    canExpressionBeConst(instanceCreation);
+
+    await builder.addDartFileEdit(file, (builder) {
+      for (var entry in replacements.entries) {
+        builder.addSimpleReplacement(range.node(entry.key), entry.value);
+      }
+      for (var token in deletions) {
+        builder.addDeletion(range.startStart(token, token.next!));
+      }
+    });
+  }
+
+  /// Return `true` if the specified [expression] has a lint fixed by this
+  /// producer.
+  bool _hasLint(InstanceCreationExpression expression) {
+    var constructorName = expression.constructorName;
+    return resolvedResult.errors.any((error) {
+      var errorCode = error.errorCode;
+      return errorCode.type == ErrorType.LINT &&
+          errorCode.name == LintNames.use_decorated_box &&
+          error.offset == constructorName.offset &&
+          error.length == constructorName.length;
+    });
+  }
+
+  /// Replace the expression if it is an [InstanceCreationExpression] and
+  /// [_hasLint].
+  void _replaceIfLint(Map<AstNode, String> replacements, Expression expression,
+      {required bool addConst}) {
+    if (expression is InstanceCreationExpression && _hasLint(expression)) {
+      var replacement = 'DecoratedBox';
+      if (addConst) {
+        replacement = 'const $replacement';
+      }
+      replacements[expression.constructorName] = replacement;
+    }
+  }
+}
diff --git a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
index 2dfd1fe..f042cc9 100644
--- a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
+++ b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
@@ -2080,7 +2080,7 @@
 LintCode.use_colored_box:
   status: needsEvaluation
 LintCode.use_decorated_box:
-  status: needsEvaluation
+  status: hasFix
 LintCode.use_enums:
   status: hasFix
 LintCode.use_full_hex_values_for_flutter_colors:
diff --git a/pkg/analysis_server/lib/src/services/correction/fix.dart b/pkg/analysis_server/lib/src/services/correction/fix.dart
index 2948fc1..3d53f78 100644
--- a/pkg/analysis_server/lib/src/services/correction/fix.dart
+++ b/pkg/analysis_server/lib/src/services/correction/fix.dart
@@ -1523,16 +1523,6 @@
     DartFixKindPriority.DEFAULT,
     "Replace 'var' with 'dynamic'",
   );
-  static const REPLACE_WITH_EIGHT_DIGIT_HEX = FixKind(
-    'dart.fix.replace.withEightDigitHex',
-    DartFixKindPriority.DEFAULT,
-    "Replace with '{0}'",
-  );
-  static const REPLACE_WITH_EIGHT_DIGIT_HEX_MULTI = FixKind(
-    'dart.fix.replace.withEightDigitHex.multi',
-    DartFixKindPriority.IN_FILE,
-    'Replace with hex digits everywhere in file',
-  );
   static const REPLACE_WITH_BRACKETS = FixKind(
     'dart.fix.replace.withBrackets',
     DartFixKindPriority.DEFAULT,
@@ -1553,6 +1543,26 @@
     DartFixKindPriority.IN_FILE,
     'Replace with ??= everywhere in file',
   );
+  static const REPLACE_WITH_DECORATED_BOX = FixKind(
+    'dart.fix.replace.withDecoratedBox',
+    DartFixKindPriority.DEFAULT,
+    "Replace with 'DecoratedBox'",
+  );
+  static const REPLACE_WITH_DECORATED_BOX_MULTI = FixKind(
+    'dart.fix.replace.withDecoratedBox.multi',
+    DartFixKindPriority.IN_FILE,
+    "Replace with 'DecoratedBox' everywhere in file",
+  );
+  static const REPLACE_WITH_EIGHT_DIGIT_HEX = FixKind(
+    'dart.fix.replace.withEightDigitHex',
+    DartFixKindPriority.DEFAULT,
+    "Replace with '{0}'",
+  );
+  static const REPLACE_WITH_EIGHT_DIGIT_HEX_MULTI = FixKind(
+    'dart.fix.replace.withEightDigitHex.multi',
+    DartFixKindPriority.IN_FILE,
+    'Replace with hex digits everywhere in file',
+  );
   static const REPLACE_WITH_EXTENSION_NAME = FixKind(
     'dart.fix.replace.withExtensionName',
     DartFixKindPriority.DEFAULT,
diff --git a/pkg/analysis_server/lib/src/services/correction/fix_internal.dart b/pkg/analysis_server/lib/src/services/correction/fix_internal.dart
index 16ce900..8534a0f 100644
--- a/pkg/analysis_server/lib/src/services/correction/fix_internal.dart
+++ b/pkg/analysis_server/lib/src/services/correction/fix_internal.dart
@@ -185,6 +185,7 @@
 import 'package:analysis_server/src/services/correction/dart/replace_var_with_dynamic.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_brackets.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_conditional_assignment.dart';
+import 'package:analysis_server/src/services/correction/dart/replace_with_decorated_box.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_eight_digit_hex.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_extension_name.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_identifier.dart';
@@ -714,6 +715,9 @@
     LintNames.unnecessary_this: [
       RemoveThisExpression.new,
     ],
+    LintNames.use_decorated_box: [
+      ReplaceWithDecoratedBox.new,
+    ],
     LintNames.use_enums: [
       ConvertClassToEnum.new,
     ],
diff --git a/pkg/analysis_server/lib/src/services/linter/lint_names.dart b/pkg/analysis_server/lib/src/services/linter/lint_names.dart
index f008de8..78a851d 100644
--- a/pkg/analysis_server/lib/src/services/linter/lint_names.dart
+++ b/pkg/analysis_server/lib/src/services/linter/lint_names.dart
@@ -164,6 +164,7 @@
   static const String unnecessary_string_interpolations =
       'unnecessary_string_interpolations';
   static const String unnecessary_this = 'unnecessary_this';
+  static const String use_decorated_box = 'use_decorated_box';
   static const String use_enums = 'use_enums';
   static const String use_full_hex_values_for_flutter_colors =
       'use_full_hex_values_for_flutter_colors';
diff --git a/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart b/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
index 76bde00..ec69d60 100644
--- a/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
+++ b/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
@@ -12,6 +12,10 @@
   const LocalKey() : super._();
 }
 
+class UniqueKey extends LocalKey {
+  UniqueKey();
+}
+
 class ValueKey<T> extends LocalKey {
   final T value;
 
diff --git a/pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart b/pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
index 7befd75..f9024d9 100644
--- a/pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
+++ b/pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
@@ -67,7 +67,7 @@
   /// This is ignored if [gradient] is non-null.
   ///
   /// The [color] is drawn under the [image].
-  final Color color;
+  final Color? color;
 
   /// A border to draw above the background [color], [gradient], or [image].
   ///
@@ -79,7 +79,7 @@
   /// Use [BoxBorder] objects to describe borders that should flip their left
   /// and right edges based on whether the text is being read left-to-right or
   /// right-to-left.
-  final BoxBorder border;
+  final BoxBorder? border;
 
   /// If non-null, the corners of this box are rounded by this [BorderRadius].
   ///
@@ -87,7 +87,7 @@
   /// [BoxShape.rectangle].
   ///
   /// {@macro flutter.painting.boxDecoration.clip}
-  final BorderRadiusGeometry borderRadius;
+  final BorderRadiusGeometry? borderRadius;
 
   /// The blend mode applied to the [color] or [gradient] background of the box.
   ///
@@ -95,7 +95,7 @@
   /// mode is used.
   ///
   /// If no [color] or [gradient] is provided then the blend mode has no impact.
-  final BlendMode backgroundBlendMode;
+  final BlendMode? backgroundBlendMode;
 
   /// The shape to fill the background [color], [gradient], and [image] into and
   /// to cast as the [boxShadow].
diff --git a/pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart b/pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
index 8bed913..9188737 100644
--- a/pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
+++ b/pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+export 'foundation.dart' show UniqueKey;
 export 'src/widgets/async.dart';
 export 'src/widgets/basic.dart';
 export 'src/widgets/container.dart';
diff --git a/pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart b/pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
new file mode 100644
index 0000000..60525de
--- /dev/null
+++ b/pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
@@ -0,0 +1,293 @@
+// Copyright (c) 2020, 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:analysis_server/src/services/correction/fix.dart';
+import 'package:analysis_server/src/services/linter/lint_names.dart';
+import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
+import 'package:test/expect.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import 'fix_processor.dart';
+
+void main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(ReplaceWithDecoratedBoxTest);
+    defineReflectiveTests(ReplaceWithDecoratedBoxInFileTest);
+    defineReflectiveTests(ReplaceWithDecoratedBoxBulkTest);
+  });
+}
+
+@reflectiveTest
+class ReplaceWithDecoratedBoxBulkTest extends BulkFixProcessorTest {
+  @override
+  String get lintCode => LintNames.use_decorated_box;
+
+  @override
+  void setUp() {
+    super.setUp();
+    writeTestPackageConfig(flutter: true);
+  }
+
+  Future<void> test_singleFile() async {
+    await resolveTestCode('''
+import 'package:flutter/material.dart';
+
+void f(Color color) {
+  Container(
+    decoration: BoxDecoration(),
+    child: Text(''),
+  );
+
+  Container(
+    decoration: BoxDecoration(color: color),
+    child: const Text(''),
+  );
+}
+''');
+    await assertHasFix('''
+import 'package:flutter/material.dart';
+
+void f(Color color) {
+  const DecoratedBox(
+    decoration: BoxDecoration(),
+    child: Text(''),
+  );
+
+  DecoratedBox(
+    decoration: BoxDecoration(color: color),
+    child: const Text(''),
+  );
+}
+''');
+  }
+}
+
+@reflectiveTest
+class ReplaceWithDecoratedBoxInFileTest extends FixInFileProcessorTest {
+  @override
+  void setUp() {
+    super.setUp();
+    writeTestPackageConfig(flutter: true);
+  }
+
+  Future<void> test_notDirectParent() async {
+    createAnalysisOptionsFile(lints: [LintNames.use_decorated_box]);
+    await resolveTestCode(r'''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: const BoxDecoration(),
+    child: Container(
+      color: Colors.white,
+      child: Container(
+        decoration: const BoxDecoration(),
+        child: Text(''),
+      ),
+    ),
+  );
+}
+''');
+    var fixes = await getFixesForFirstError();
+    expect(fixes, hasLength(1));
+    assertProduces(fixes.first, r'''
+import 'package:flutter/material.dart';
+
+void f() {
+  DecoratedBox(
+    decoration: const BoxDecoration(),
+    child: Container(
+      color: Colors.white,
+      child: const DecoratedBox(
+        decoration: BoxDecoration(),
+        child: Text(''),
+      ),
+    ),
+  );
+}
+''');
+  }
+
+  Future<void> test_parentConst_childConst() async {
+    createAnalysisOptionsFile(lints: [LintNames.use_decorated_box]);
+    await resolveTestCode(r'''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: const BoxDecoration(),
+    child: Container(
+      decoration: const BoxDecoration(),
+      child: const Text(''),
+    ),
+  );
+}
+''');
+    var fixes = await getFixesForFirstError();
+    expect(fixes, hasLength(1));
+    assertProduces(fixes.first, r'''
+import 'package:flutter/material.dart';
+
+void f() {
+  const DecoratedBox(
+    decoration: BoxDecoration(),
+    child: DecoratedBox(
+      decoration: BoxDecoration(),
+      child: Text(''),
+    ),
+  );
+}
+''');
+  }
+
+  Future<void> test_parentConst_childNotConst() async {
+    createAnalysisOptionsFile(lints: [LintNames.use_decorated_box]);
+    await resolveTestCode(r'''
+import 'package:flutter/material.dart';
+
+void f(int i) {
+  Container(
+    decoration: const BoxDecoration(),
+    child: Container(
+      decoration: const BoxDecoration(),
+      child: Text('$i'),
+    ),
+  );
+}
+''');
+    var fixes = await getFixesForFirstError();
+    expect(fixes, hasLength(1));
+    assertProduces(fixes.first, r'''
+import 'package:flutter/material.dart';
+
+void f(int i) {
+  DecoratedBox(
+    decoration: const BoxDecoration(),
+    child: DecoratedBox(
+      decoration: const BoxDecoration(),
+      child: Text('$i'),
+    ),
+  );
+}
+''');
+  }
+
+  Future<void> test_parentNotConst_childConst() async {
+    createAnalysisOptionsFile(lints: [LintNames.use_decorated_box]);
+    await resolveTestCode(r'''
+import 'package:flutter/material.dart';
+
+void f(Color color) {
+  Container(
+    decoration: BoxDecoration(color: color),
+    child: Container(
+      decoration: const BoxDecoration(),
+      child: Text(''),
+    ),
+  );
+}
+''');
+    var fixes = await getFixesForFirstError();
+    expect(fixes, hasLength(1));
+    assertProduces(fixes.first, r'''
+import 'package:flutter/material.dart';
+
+void f(Color color) {
+  DecoratedBox(
+    decoration: BoxDecoration(color: color),
+    child: const DecoratedBox(
+      decoration: BoxDecoration(),
+      child: Text(''),
+    ),
+  );
+}
+''');
+  }
+}
+
+@reflectiveTest
+class ReplaceWithDecoratedBoxTest extends FixProcessorLintTest {
+  @override
+  FixKind get kind => DartFixKind.REPLACE_WITH_DECORATED_BOX;
+
+  @override
+  String get lintCode => LintNames.use_decorated_box;
+
+  @override
+  void setUp() {
+    super.setUp();
+    writeTestPackageConfig(flutter: true);
+  }
+
+  Future<void> test_canBeConst() async {
+    await resolveTestCode('''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: BoxDecoration(),
+    child: Text(''),
+  );
+}
+''');
+    await assertHasFix('''
+import 'package:flutter/material.dart';
+
+void f() {
+  const DecoratedBox(
+    decoration: BoxDecoration(),
+    child: Text(''),
+  );
+}
+''');
+  }
+
+  Future<void> test_const() async {
+    await resolveTestCode('''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: const BoxDecoration(),
+    child: const Text(''),
+  );
+}
+''');
+    await assertHasFix('''
+import 'package:flutter/material.dart';
+
+void f() {
+  const DecoratedBox(
+    decoration: BoxDecoration(),
+    child: Text(''),
+  );
+}
+''');
+  }
+
+  Future<void> test_nonConst() async {
+    await resolveTestCode('''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    key: UniqueKey(),
+    decoration: const BoxDecoration(),
+    child: const Text(''),
+  );
+}
+''');
+    await assertHasFix('''
+import 'package:flutter/material.dart';
+
+void f() {
+  DecoratedBox(
+    key: UniqueKey(),
+    decoration: const BoxDecoration(),
+    child: const Text(''),
+  );
+}
+''');
+  }
+}
diff --git a/pkg/analysis_server/test/src/services/correction/fix/test_all.dart b/pkg/analysis_server/test/src/services/correction/fix/test_all.dart
index 743f280..3c49661 100644
--- a/pkg/analysis_server/test/src/services/correction/fix/test_all.dart
+++ b/pkg/analysis_server/test/src/services/correction/fix/test_all.dart
@@ -227,6 +227,7 @@
 import 'replace_with_brackets_test.dart' as replace_with_brackets;
 import 'replace_with_conditional_assignment_test.dart'
     as replace_with_conditional_assignment;
+import 'replace_with_decorated_box_test.dart' as replace_with_decorated_box;
 import 'replace_with_eight_digit_hex_test.dart' as replace_with_eight_digit_hex;
 import 'replace_with_extension_name_test.dart' as replace_with_extension_name;
 import 'replace_with_identifier_test.dart' as replace_with_identifier;
@@ -450,6 +451,7 @@
     replace_var_with_dynamic.main();
     replace_with_brackets.main();
     replace_with_conditional_assignment.main();
+    replace_with_decorated_box.main();
     replace_with_eight_digit_hex.main();
     replace_with_extension_name.main();
     replace_with_identifier.main();

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Brian Wilkerson (Gerrit)

unread,

Feb 1, 2023, 2:59:35 PM2/1/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Ahmed Ashour.

View Change

1 comment:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:
- Patch Set #1, Line 36: parentInstanceCreation
  This is too fragile. It assumes that the parent will be a `NamedExpression` in an `ArgumentList` in what might be an `InstanceCreationExpression`. If we need to change the AST structure for some reason in the future this could be testing the wrong node and the only indication would be that we offer the fix where we previously didn't. I'm not sure what the best way to fix it is, so we might have to leave it, but I wanted to mention it in case you had ideas.
  On the flip side, this is only checking the next level up in the hierarchy. What if the `parentInstanceCreation` is a non-`Container` widget wrapped inside a `Container` that should be converted? Should we still block suggesting the fix?
  I suspect the answer is yes because I suspect the reason for this code is to support the bulk fix case where there could be edit conflicts as a result of trying to clean up the `const` keywords.
  But, I question whether we want to prevent the fix from being offered for nested containers when we're not running in a bulk fix mode. There really isn't any reason why a user can't replace an inner container without replacing the outer container. Nor is it clear to me that a user would necessarily want to replace an inner container as part of replacing the outer container. That's kind of the whole motivation for the 'fix in file' and 'bulk fix' modes, to specify that you want more than the one occurrence to be fixed.
  And I'll just point out that for fix in file we might need this, but for bulk fix we'll iterate and fix the nested cases in a future pass, so it isn't clear that we even need this complexity for that case.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 2, 2023, 3:28:28 PM2/2/23

to rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Brian Wilkerson.

View Change

1 comment:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #1, Line 36: parentInstanceCreation

- This is too fragile. ...change the AST ... in the future this could be testing the wrong node
  I understand, and it is guarded by the tests. So, whenever the AST changes, a failing test would give a hint about an incorrect implementation at that time, so I guess it is fine.
  ... What if the `parentInstanceCreation` is a non-`Container` widget wrapped inside a `Container` that should be converted?
  
  I should have explained the motivation, earlier.
  Let's split the discussion into two things, the `const` addition or removal of the converted node and its children, and offering the fix in file and bulk modes.
  To support the `const` modification of a single occurrence, we ideally need to check the hierarchy and decide for each node accordingly, and I initially thought that we should not walk up to the parent of the `DecoratedBox` because there is no need for `const` changes there, as parent `Container` can not be constant. After a second thought, this is not ideal, because adding `const` to `DecoratedBox` can potentially necessitate having _another_ parent widget possibly `const`.
  I think now, it is better to have a utility `ConstModifier`, which is given a node, and have methods `addConst` and `removeConst`, so the caller doesn't bother about the details, and this utility class modifies the hierarchy accordingly. This should be used in all places where a fix needs to add or remove `const`. This should handle named and positional parameters.
  Regarding the fix in file, I understand it as supporting 'everywhere in file', and I don't see a reason why it shouldn't be there. As there is no point to modify a single place, without modifying others. I understand that it is usually not supported, when the needed complex implementation is not done, or other fixes can be offered for the same diagnostic.

- And I'll just point out that for fix in file we might need this, but for bulk fix we'll iterate and fix the nested cases in a future pass, so it isn't clear that we even need this complexity for that case.

- I think the complexity comes from replacing a container with a `const` widget, and modifying the hierarchy, even for a single occurrence.
  The bulk fix is used from the command line, and I think there is no harm to also allow it.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Brian Wilkerson (Gerrit)

unread,

Feb 2, 2023, 4:12:31 PM2/2/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Ahmed Ashour.

View Change

1 comment:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #1, Line 36: parentInstanceCreation

- > This is too fragile. ...change the AST ... in the future this could be testing the wrong node […]
  Regarding the addition or removal of `const`:
  I like the idea of adding a utility (probably to `DartFileEditBuilder`, maybe something like `addConst(Expression)`) to add the keyword `const` to an expression and remove any no-longer needed `const` modifiers on children. It isn't clear to me that we need a corresponding `removeConst`, at least not for this fix.
  I don't like the idea of unconditionally moving the `const` keyword higher in the widget tree. I think it would be surprising to users to have "non-local" (for some definition of "local") changes. That said, if the user has enabled `prefer_const_constructors`, then I think that moving the keyword higher would be the right choice.
  Moving the keyword higher could also be handled in the utility. I'd probably do so by walking up the parent chain to find the highest node that could be converted, then walking downward from that node to remove the keyword where it's no longer needed. I think that would be the easiest implementation to understand.
  Regarding the fix-in-file / bulk-fix modes:
  I didn't communicate clearly. I wasn't suggesting that we shouldn't have fix-in-file or bulk-fix support. I was commenting on the policy of not offering the fix if a parent `Container` creation also had the lint associated with it.
  If the user wants to apply the fix to a single `Container` constructor invocation (the single-fix mode), I think that should be allowed, even if that invocation is nested inside another `Container` constructor invocation. But I think line 38 will prevent that.
  For the fix-in-file mode we probably do need to find all of the `Container` constructor invocations in a given widget tree that need to be replaced, compute the set of ancestor nodes that need to have `const` added to them, and then use `addConst` on each of those ancestor nodes. That's different enough that we probably need to have two implementation paths, one for fix-in-file and one for both single-fix and bulk-fix.
  Hope that was a little more clear.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 2, 2023, 5:20:53 PM2/2/23

to rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Brian Wilkerson.

View Change

1 comment:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #1, Line 36: parentInstanceCreation

- Regarding the addition or removal of `const`: […]
  Oh! I have read the earlier message many times and couldn't figure out the main point. Now I see.
  I forgot to consider, that line 38 was added to prevent a change if the parent has the lint, because it causes conflict in fix-in-file mode.
  Now I see there an `applyingBulkFixes` flag in `CorrectionProducerContet`, but it is `true` for both fix-in-file and bulk. I wonder how to know if we are running in only fix-in-file mode.
  As always, thanks a lot for the guidance.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Brian Wilkerson (Gerrit)

unread,

Feb 3, 2023, 11:28:14 AM2/3/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Ahmed Ashour.

View Change

1 comment:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #1, Line 36: parentInstanceCreation

- Oh! I have read the earlier message many times and couldn't figure out the main point. Now I see. […]
  I don't think we can at the moment. It won't be terrible if we use the same approach in bulk-fix mode that we use in fix-in-file mode, so that's the approach I think we should use.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 3, 2023, 12:20:47 PM2/3/23

to rev...@dartlang.org

Attention is currently required from: Ahmed Ashour.

Ahmed Ashour uploaded patch set #2 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart

M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
M pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
12 files changed, 538 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 3, 2023, 12:39:00 PM2/3/23

to rev...@dartlang.org

Attention is currently required from: Ahmed Ashour.

Ahmed Ashour uploaded patch set #3 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart
M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
M pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart

12 files changed, 528 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 3, 2023, 12:43:22 PM2/3/23

to rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Brian Wilkerson.

View Change

1 comment:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #1, Line 36: parentInstanceCreation

- I don't think we can at the moment. […]
  Ok.
  Now, if in single-mode, we walk up the tree to find the top linted expression to change, and call the utility method, which adds const to it and handle const of the children recursively.
  If in fix-in-file or bulk mode, we check the direct parent, if it has the lint, we do nothing, because that means the parent has taken care of this node. Otherwise, that mean we are a top-node, and so the utility method is call to handle children recursively.
  PTAL.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 3, 2023, 12:50:44 PM2/3/23

to rev...@dartlang.org

Attention is currently required from: Brian Wilkerson.

Ahmed Ashour uploaded patch set #4 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart
M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
M pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
12 files changed, 528 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 3, 2023, 1:07:38 PM2/3/23

to rev...@dartlang.org

Attention is currently required from: Brian Wilkerson.

Ahmed Ashour uploaded patch set #5 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart
M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
M pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart

12 files changed, 527 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Brian Wilkerson (Gerrit)

unread,

Feb 3, 2023, 2:22:44 PM2/3/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Ahmed Ashour.

View Change

7 comments:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:
- Patch Set #4, Line 35: _hasLint(parentInstanceCreation)
  What if the parent doesn't have the lint, but one of it's parents does? I think the loop will stop and we won't notice that this diagnostic is nested.
File pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart:
- Patch Set #4, Line 32: test_singleFile
  The potential problem we need to guard against in bulk-fix mode tests is cases where we potentially have overlapping edits. Cases like the one below don't really have any potential for failure. I think the only case for this fix is when we have nested containers that are both being converted.
- Patch Set #4, Line 112: test_parentConst_childConst
  We ought to have a test that ensures that the fix for the second error is also valid (or that we don't offer a fix-in-file fix if that's the case).
File pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart:
- Patch Set #4, Line 1673: canChildBeConst
  It seems to me that if you find a child that can't be const, then you can conclude that `expression` can't be const either (and short-circuit looking at the remaining children), at least as far as computing `canBeConst` is concerned.
- Patch Set #4, Line 1689: if (canBeConst) {
  If the `expression` can't be const, then I think we shouldn't be removing any `const` keywords from any of the children, either any visited in this invocation of `canExpressionBeConst` or those visited in any recursive invocations of this function. But I think that by adding them to `deletions` here were causing them to be unconditionally removed even if a not-yet visited sibling of one of the parents causes a const to not be added somewhere higher up.
  In other words, I think we need to know whether we can add const to the top-level instance creation expression before we start looking for keywords that we can removed. Either that, or we need to not remove any keywords if the top-level constructor invocation can't be made const (though that doesn't allow us to add const to any children that could be made const).
  I suspect that the best way to implement this will be to return a "status" object from `canExpressionBeConst` that keeps track of whether `const` could be added, if so which keywords can be deleted, and if not which children (possibly multiple levels deep) could be made constant (as a list of status objects so that we can walk the tree of status objects to determine what edits to make).
- Patch Set #4, Line 1691: keyword?
  Stylistically, it would be better to write `keyword != null && keyword.type ...`. It would remove both the `?` and the `!` below, leaving the code easier to reason about.
File pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart:
- Patch Set #4, Line 392: replaceWithConstRecursively
  This isn't a general utility that I could imagine using in other contexts. When we were talking earlier I thought we might have a method that would add `const` and clean up any children. That's something I could imagine being able to use in other places. But that isn't what this does, so I'm going to suggest that we not add it here, but just make it a utility method in the correction producer.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 6, 2023, 5:36:08 PM2/6/23

to rev...@dartlang.org

Attention is currently required from: Ahmed Ashour.

Ahmed Ashour uploaded patch set #6 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart

M pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
A pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart


M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart

M pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
13 files changed, 585 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 6, 2023, 5:38:20 PM2/6/23

to rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Brian Wilkerson.

View Change

7 comments:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #4, Line 35: _hasLint(parentInstanceCreation)

- What if the parent doesn't have the lint, but one of it's parents does? I think the loop will stop a […]
  It was done on purposed, since I thought something like `FunctionExpression` should not be considered a 'widget' parent, so we stop whenever the direct parent doesn't have the lint.
  Anyhow, this behavior is changed, test case in `test_functionExpression`
File pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart:

- Patch Set #4, Line 32: test_singleFile

- The potential problem we need to guard against in bulk-fix mode tests is cases where we potentially […]
  Done, then it has the same purpose as the fix-in-file, to try to find issue in conflict. I recall seeing this not followed in other tests. Anyhow, this would be hopefully remembered, thanks.

- Patch Set #4, Line 112: test_parentConst_childConst

- We ought to have a test that ensures that the fix for the second error is also valid (or that we don […]
  To catch the case of the second error, we need something else other than `getFixesForFirstError()`, for example, `getFixForError(int index)`. Actually it was tried, but then another test was added, `test_hierarchy()` (lower) which tests that in all three layers, the same expectations are found.
File pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart:

- Patch Set #4, Line 1673: canChildBeConst

- It seems to me that if you find a child that can't be const, then you can conclude that `expression` […]
  Well, the loop has a side effect, even if a child is non-const, we need to proceed next children, in case they need replacement.

- Patch Set #4, Line 1689: if (canBeConst) {
  If the `expression` can't be const, then I think we shouldn't be removing any `const` keywords from any of the children, either any visited in this invocation of `canExpressionBeConst` or those visited in any recursive invocations of this function. But I think that by adding them to `deletions` here were causing them to be unconditionally removed even if a not-yet visited sibling of one of the parents causes a const to not be added somewhere higher up.

- Children `const` are not unconditionally removed.
  Let's talk about a specific expression: we iterate over its children, to determine first if this expression can be `const` or not.
  If the expression is `const`, then we are sure it is a `const`, and we add it in replacement as `const DecorationBox` (coming to this later). And because the expression is `const`, we are sure that `const` can be removed of its children, whether or this expression will remain `const` or not (because the grand-parent can be `const`).
  If the expression is non-const, then we don't remove the `const` from children.
  Coming back to the `const expression`, in the iteration of the grand parent:
  If the grand parent is `const`, then we overwrite the replacement of the expression to be non-`const`.
  If the grand parent is non-`const`, then we overwrite the replacement of the expression to be a `const`.
  The tests cover the scenarios of the parent non-const or const, and if the children is (non)-const.

- Patch Set #4, Line 1691: keyword?

- Stylistically, it would be better to write `keyword != null && keyword.type ...`. […]
  Done
File pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart:

- Patch Set #4, Line 392: replaceWithConstRecursively

- This isn't a general utility that I could imagine using in other contexts. […]
  True. I initially thought that we a general utility for only `const`, but it seems we need to have both replacement of `DecoratedBox` and `const` together.
  Another idea to consider, is to add/remove the AST nodes, not strings of source code, which could also have multiple fixes do concurrent changes to a specific `node`.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 6, 2023, 5:42:30 PM2/6/23

to rev...@dartlang.org

Attention is currently required from: Brian Wilkerson.

Ahmed Ashour uploaded patch set #7 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
A pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart

12 files changed, 584 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Brian Wilkerson (Gerrit)

unread,

Feb 7, 2023, 2:22:41 PM2/7/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Ahmed Ashour.

View Change

6 comments:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #4, Line 35: _hasLint(parentInstanceCreation)

- It was done on purposed, since I thought something like `FunctionExpression` should not be considered a 'widget' parent ...
  I agree. Any node other than an `InstanceCreationExpression` shouldn't be considered a 'widget' parent. But this code requires that parent be both an `InstanceCreationExpression` _and_ that it have the lint associated with it. My point was that we should continue to look up the tree even if we find an `InstanceCreationExpression` that doesn't have the lint associated with it.
  Using `thisOrAncestorOfType` (on line 35) doesn't solve the problem I had in mind, but it does introduce exactly the kind of problem you were correctly trying to avoid. If there's a widget creation inside a closure that's passed to another widget creation then the code will incorrectly assume that one widget is a child of the other, when that isn't the case.
  That said, the more I think about it the more I'm thinking that it might not matter because a closure isn't a constant expression, so that might counter the theoretic problem with using `thisOrAncestorOfType`.
File pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart:

- Patch Set #4, Line 32: test_singleFile

- Done, then it has the same purpose as the fix-in-file, to try to find issue in conflict. […]
  If you happen to find such tests again, please let us know. (Although there are cases where it isn't actually possible to create a test case in which the fixed regions are overlapping, so there isn't always a good test case. I don't know if that's what you were seeing or not.)

- Patch Set #4, Line 112: test_parentConst_childConst

- To catch the case of the second error, we need something else other than `getFixesForFirstError()`, […]
  Perfect. I missed noticing that aspect of `test_hierarchy`. Thanks!
File pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart:
- Patch Set #7, Line 71: createAnalysisOptionsFile
  Consider moving this line to `setUp`.
- Patch Set #7, Line 352: var offset in [54, 109, 174]
  If I'm understanding the test correctly, the single-location fix is offered in all three places, which is good, but selecting any one of those will fix all of the diagnostics in the enclosing widget tree. I don't think that's what the user expects; I think we should fix a single location and ignore other locations.
File pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart:
- Patch Set #4, Line 1689: if (canBeConst) {
  > If the `expression` can't be const, then I think we shouldn't be removing any `const` keywords fro […]
  Ok, I think I understand. Thanks.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 7, 2023, 3:14:09 PM2/7/23

to rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Brian Wilkerson.

View Change

2 comments:

File pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart:

- Patch Set #4, Line 35: _hasLint(parentInstanceCreation)

- > It was done on purposed, since I thought something like `FunctionExpression` should not be conside […]
  As you know, one of the benefits about `thisOrAncestorOfType`, is that it also handles positional parameters (which is rare in Flutter).
  I also think it is fine this way.
File pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart:
- Patch Set #7, Line 352: var offset in [54, 109, 174]
  If I'm understanding the test correctly, the single-location fix is offered in all three places, whi […]
  I was under the impression that in 'single-mode', there is no point to change a single location without changing the surroundings (children and parents).
  Tomorrow, the CL would be changed so that, the single mode will change only the expression, change `const` of itself and children (if needed), but won't change the `Container` children or parents of the expression selected.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 8, 2023, 7:07:32 AM2/8/23

to rev...@dartlang.org

Attention is currently required from: Brian Wilkerson.

Ahmed Ashour uploaded patch set #8 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
A pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart

12 files changed, 624 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 8, 2023, 7:08:03 AM2/8/23

to rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Brian Wilkerson.

View Change

2 comments:

File pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart:
- Patch Set #7, Line 71: createAnalysisOptionsFile

- Consider moving this line to `setUp`.

- Done

- Patch Set #7, Line 352: var offset in [54, 109, 174]

- I was under the impression that in 'single-mode', there is no point to change a single location with […]
  PTAL.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 8, 2023, 8:08:55 AM2/8/23

to rev...@dartlang.org

Attention is currently required from: Brian Wilkerson.

Ahmed Ashour uploaded patch set #9 to this change.

View Change

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94
---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
A pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart

12 files changed, 623 insertions(+), 15 deletions(-)

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Brian Wilkerson (Gerrit)

unread,

Feb 9, 2023, 3:56:24 PM2/9/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Attention is currently required from: Ahmed Ashour.

Patch set 9:Code-Review +1Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #9:
  Thanks! Sorry it took so long for me to figure out / communicate what the fix ought to do.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Ashour (Gerrit)

unread,

Feb 9, 2023, 3:59:22 PM2/9/23

to rev...@dartlang.org, Commit Queue, Brian Wilkerson

View Change

1 comment:

Patchset:

- Patch Set #9:
  Thanks! Sorry it took so long for me to figure out / communicate what the fix ought to do.

- No worries, discussion is always fruitful. Thanks a lot for your support.

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Commit Queue (Gerrit)

unread,

Feb 9, 2023, 5:12:18 PM2/9/23

to Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

Commit Queue submitted this change.

View Change

Approvals:
  Brian Wilkerson: Looks good to me, approved; Commit

[analysis_server] add a fix for `use_decorated_box`

Fixes #50817

Change-Id: I78309df7cda2dc74b62eab3bdb48e52a3df02f94

Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280165
Reviewed-by: Brian Wilkerson <brianwi...@google.com>
Commit-Queue: Brian Wilkerson <brianwi...@google.com>


---
A pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
M pkg/analysis_server/lib/src/services/correction/fix.dart
M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
M pkg/analysis_server/lib/src/services/linter/lint_names.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
A pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/src/painting/box_decoration.dart
M pkg/analysis_server/test/mock_packages/flutter/lib/widgets.dart
A pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
M pkg/analysis_server/test/src/services/correction/fix/test_all.dart

12 files changed, 626 insertions(+), 15 deletions(-)

diff --git a/pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
new file mode 100644

index 0000000..9fc1ae4
--- /dev/null
+++ b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_decorated_box.dart
@@ -0,0 +1,124 @@

+    var instanceCreation =
+        node.thisOrAncestorOfType<InstanceCreationExpression>();
+    if (instanceCreation is! InstanceCreationExpression) return;
+

+    if (applyingBulkFixes) {
+      var parent = instanceCreation.parent
+          ?.thisOrAncestorOfType<InstanceCreationExpression>();
+
+      while (parent != null) {
+        if (_hasLint(parent)) return;
+        parent =
+            parent.parent?.thisOrAncestorOfType<InstanceCreationExpression>();
+      }
+    }
+


+    var deletions = <Token>[];
+    var replacements = <AstNode, String>{};
+    var linterContext = getLinterContext();
+

+    void replace(Expression expression, {required bool addConst}) {


+      if (expression is InstanceCreationExpression && _hasLint(expression)) {

+        replacements[expression.constructorName] =
+            '${addConst ? 'const ' : ''}DecoratedBox';
+      }
+    }
+
+    /// Replace the expression if [isReplace] is `true` and it [_hasLint]
+    /// and return whether it can be a `const` or not.
+    bool canExpressionBeConst(Expression expression,
+        {required bool isReplace}) {


+      var canBeConst = linterContext.canBeConst(expression);
+      if (!canBeConst &&

+          isReplace &&
+          expression is InstanceCreationExpression &&
+          _hasLint(expression)) {


+        canBeConst = true;
+        var childrenConstMap = <InstanceCreationExpression, bool>{};
+        for (var argument in expression.argumentList.arguments) {
+          if (argument is NamedExpression) {
+            var child = argument.expression;
+            var canChildBeConst =

+                canExpressionBeConst(child, isReplace: applyingBulkFixes);


+            canBeConst &= canChildBeConst;
+            if (child is InstanceCreationExpression) {
+              childrenConstMap[child] = canChildBeConst;
+            }
+          } else {

+            canBeConst &= canExpressionBeConst(argument, isReplace: isReplace);
+          }
+        }
+
+        replace(expression, addConst: canBeConst);


+
+        for (var entry in childrenConstMap.entries) {
+          var child = entry.key;
+          var canChildBeConst = entry.value;

+          if (applyingBulkFixes) {
+            replace(child, addConst: canChildBeConst && !canBeConst);


+          }
+          if (canBeConst) {
+            var keyword = child.keyword;

+            if (keyword != null && keyword.type == Keyword.CONST) {
+              deletions.add(keyword);


+            }
+          }
+        }
+      }
+      return canBeConst;
+    }
+

+    canExpressionBeConst(instanceCreation, isReplace: true);


+
+    await builder.addDartFileEdit(file, (builder) {
+      for (var entry in replacements.entries) {
+        builder.addSimpleReplacement(range.node(entry.key), entry.value);
+      }
+      for (var token in deletions) {
+        builder.addDeletion(range.startStart(token, token.next!));
+      }
+    });
+  }
+

+  /// Return `true` if the specified [expression] has the lint fixed by this


+  /// producer.
+  bool _hasLint(InstanceCreationExpression expression) {
+    var constructorName = expression.constructorName;
+    return resolvedResult.errors.any((error) {
+      var errorCode = error.errorCode;
+      return errorCode.type == ErrorType.LINT &&
+          errorCode.name == LintNames.use_decorated_box &&
+          error.offset == constructorName.offset &&
+          error.length == constructorName.length;
+    });
+  }
+}

diff --git a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
index 17abab1..acba8f3 100644
--- a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
+++ b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
@@ -2059,7 +2059,7 @@


 LintCode.use_colored_box:
   status: needsEvaluation
 LintCode.use_decorated_box:
-  status: needsEvaluation
+  status: hasFix
 LintCode.use_enums:
   status: hasFix
 LintCode.use_full_hex_values_for_flutter_colors:
diff --git a/pkg/analysis_server/lib/src/services/correction/fix.dart b/pkg/analysis_server/lib/src/services/correction/fix.dart

index bf989db..bbf8dae 100644
--- a/pkg/analysis_server/lib/src/services/correction/fix.dart
+++ b/pkg/analysis_server/lib/src/services/correction/fix.dart
@@ -1538,16 +1538,6 @@


     DartFixKindPriority.DEFAULT,
     "Replace 'var' with 'dynamic'",
   );
-  static const REPLACE_WITH_EIGHT_DIGIT_HEX = FixKind(
-    'dart.fix.replace.withEightDigitHex',
-    DartFixKindPriority.DEFAULT,
-    "Replace with '{0}'",
-  );
-  static const REPLACE_WITH_EIGHT_DIGIT_HEX_MULTI = FixKind(
-    'dart.fix.replace.withEightDigitHex.multi',
-    DartFixKindPriority.IN_FILE,
-    'Replace with hex digits everywhere in file',
-  );
   static const REPLACE_WITH_BRACKETS = FixKind(
     'dart.fix.replace.withBrackets',
     DartFixKindPriority.DEFAULT,

@@ -1568,6 +1558,26 @@

index a30367e..d0109e0 100644
--- a/pkg/analysis_server/lib/src/services/correction/fix_internal.dart
+++ b/pkg/analysis_server/lib/src/services/correction/fix_internal.dart
@@ -186,6 +186,7 @@


 import 'package:analysis_server/src/services/correction/dart/replace_var_with_dynamic.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_brackets.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_conditional_assignment.dart';
+import 'package:analysis_server/src/services/correction/dart/replace_with_decorated_box.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_eight_digit_hex.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_extension_name.dart';
 import 'package:analysis_server/src/services/correction/dart/replace_with_identifier.dart';

@@ -720,6 +721,9 @@


     LintNames.unnecessary_this: [
       RemoveThisExpression.new,
     ],
+    LintNames.use_decorated_box: [
+      ReplaceWithDecoratedBox.new,
+    ],
     LintNames.use_enums: [
       ConvertClassToEnum.new,
     ],
diff --git a/pkg/analysis_server/lib/src/services/linter/lint_names.dart b/pkg/analysis_server/lib/src/services/linter/lint_names.dart

index 3d9c9d1..c5d9e26 100644
--- a/pkg/analysis_server/lib/src/services/linter/lint_names.dart
+++ b/pkg/analysis_server/lib/src/services/linter/lint_names.dart
@@ -166,6 +166,7 @@


   static const String unnecessary_string_interpolations =
       'unnecessary_string_interpolations';
   static const String unnecessary_this = 'unnecessary_this';
+  static const String use_decorated_box = 'use_decorated_box';
   static const String use_enums = 'use_enums';
   static const String use_full_hex_values_for_flutter_colors =
       'use_full_hex_values_for_flutter_colors';

diff --git a/pkg/analysis_server/test/mock_packages/flutter/lib/material.dart b/pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
index 16e6355..81fdd3f 100644
--- a/pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
+++ b/pkg/analysis_server/test/mock_packages/flutter/lib/material.dart
@@ -5,5 +5,6 @@
 export 'src/material/app_bar.dart';
 export 'src/material/colors.dart';
 export 'src/material/icons.dart';
+export 'src/material/ink_well.dart';
 export 'src/material/scaffold.dart';
 export 'widgets.dart';


diff --git a/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart b/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
index 76bde00..ec69d60 100644
--- a/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
+++ b/pkg/analysis_server/test/mock_packages/flutter/lib/src/foundation/key.dart
@@ -12,6 +12,10 @@
   const LocalKey() : super._();
 }
 
+class UniqueKey extends LocalKey {
+  UniqueKey();
+}
+
 class ValueKey<T> extends LocalKey {
   final T value;

diff --git a/pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart b/pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart
new file mode 100644
index 0000000..c037dfe
--- /dev/null
+++ b/pkg/analysis_server/test/mock_packages/flutter/lib/src/material/ink_well.dart
@@ -0,0 +1,33 @@
+// Copyright 2014 The Flutter Authors. 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:flutter/foundation.dart';
+import 'package:flutter/widgets.dart';
+
+class InkResponse extends StatelessWidget {
+  const InkResponse({
+    super.key,
+    this.child,
+    this.onTap,
+    this.onTapDown,
+    this.onTapUp,
+    this.onTapCancel,
+    this.onDoubleTap,
+    this.onLongPress,
+    this.onHighlightChanged,
+    this.onHover,
+  });
+
+  final Widget? child;
+
+  final ValueChanged<bool>? onHover;
+}
+
+class InkWell extends InkResponse {
+  const InkWell({
+    super.key,
+    super.child,
+    super.onHover,
+  });
+}

index 0000000..3785603
--- /dev/null
+++ b/pkg/analysis_server/test/src/services/correction/fix/replace_with_decorated_box_test.dart
@@ -0,0 +1,417 @@


+// Copyright (c) 2023, 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.
+

+    child: Container(
+      decoration: const BoxDecoration(),
+      child: const Text(''),
+    ),
+  );
+}
+''');

+    await assertHasFix('''
+import 'package:flutter/material.dart';
+
+void f(Color color) {
+  const DecoratedBox(
+    decoration: BoxDecoration(),

+    child: DecoratedBox(


+      decoration: BoxDecoration(),
+      child: Text(''),
+    ),
+  );
+}
+''');
+  }
+}
+
+@reflectiveTest

+class ReplaceWithDecoratedBoxInFileTest extends FixInFileProcessorTest {
+  @override
+  void setUp() {
+    super.setUp();
+    writeTestPackageConfig(flutter: true);

+    createAnalysisOptionsFile(lints: [LintNames.use_decorated_box]);
+  }
+
+  Future<void> test_functionExpression() async {


+    await resolveTestCode(r'''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: const BoxDecoration(),

+    child: InkWell(
+      onHover: (b) {


+        Container(
+          decoration: const BoxDecoration(),
+          child: const Text(''),
+        );

+      },


+      child: Container(
+        decoration: const BoxDecoration(),
+        child: const Text(''),
+      ),

+    ),
+  );
+}
+''');
+    var fixes = await getFixesForFirstError();
+    expect(fixes, hasLength(1));
+    assertProduces(fixes.first, r'''
+import 'package:flutter/material.dart';
+
+void f() {
+  DecoratedBox(
+    decoration: const BoxDecoration(),

+    child: InkWell(
+      onHover: (b) {


+        Container(
+          decoration: const BoxDecoration(),
+          child: const Text(''),
+        );

+      },


+      child: Container(
+        decoration: const BoxDecoration(),
+        child: const Text(''),
+      ),

+    ),
+  );
+}
+''');
+  }
+

+  Future<void> test_notDirectParent() async {

+
+  Future<void> test_parentConst_childConst() async {

+  Future<void> test_hierarchy() async {
+    useLineEndingsForPlatform = false;
+


+    await resolveTestCode('''
+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: BoxDecoration(),

+    child: Container(


+      decoration: const BoxDecoration(),
+      child: Container(

+        decoration: BoxDecoration(),
+        child: Text(''),
+      ),
+    ),
+  );
+}
+''');
+

+    await assertHasFix(
+      '''


+import 'package:flutter/material.dart';
+
+void f() {
+  DecoratedBox(

+    decoration: BoxDecoration(),


+    child: Container(
+      decoration: const BoxDecoration(),

+      child: Container(


+        decoration: BoxDecoration(),
+        child: Text(''),

+      ),
+    ),
+  );
+}

+''',
+      allowFixAllFixes: true,
+      errorFilter: (error) => error.offset == 54,
+    );
+
+    await assertHasFix(
+      '''


+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: BoxDecoration(),

+    child: DecoratedBox(


+      decoration: const BoxDecoration(),
+      child: Container(

+        decoration: BoxDecoration(),
+        child: Text(''),
+      ),
+    ),
+  );
+}

+''',
+      allowFixAllFixes: true,
+      errorFilter: (error) => error.offset == 109,
+    );
+
+    await assertHasFix(
+      '''


+import 'package:flutter/material.dart';
+
+void f() {
+  Container(
+    decoration: BoxDecoration(),

+    child: Container(
+      decoration: const BoxDecoration(),

+      child: const DecoratedBox(
+        decoration: BoxDecoration(),
+        child: Text(''),
+      ),
+    ),
+  );
+}

+''',
+      allowFixAllFixes: true,
+      errorFilter: (error) => error.offset == 174,

index cc5a804..09ad149 100644
--- a/pkg/analysis_server/test/src/services/correction/fix/test_all.dart
+++ b/pkg/analysis_server/test/src/services/correction/fix/test_all.dart
@@ -228,6 +228,7 @@


 import 'replace_with_brackets_test.dart' as replace_with_brackets;
 import 'replace_with_conditional_assignment_test.dart'
     as replace_with_conditional_assignment;
+import 'replace_with_decorated_box_test.dart' as replace_with_decorated_box;
 import 'replace_with_eight_digit_hex_test.dart' as replace_with_eight_digit_hex;
 import 'replace_with_extension_name_test.dart' as replace_with_extension_name;
 import 'replace_with_identifier_test.dart' as replace_with_identifier;

@@ -451,6 +452,7 @@


     replace_var_with_dynamic.main();
     replace_with_brackets.main();
     replace_with_conditional_assignment.main();
+    replace_with_decorated_box.main();
     replace_with_eight_digit_hex.main();
     replace_with_extension_name.main();
     replace_with_identifier.main();

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

CBuild (Gerrit)

unread,

Feb 9, 2023, 5:47:04 PM2/9/23

to Commit Queue, Ahmed Ashour, rev...@dartlang.org, Brian Wilkerson

go/dart-cbuild result: FAILURE (NO REGRESSIONS DETECTED)

Details: https://goto.google.com/dart-cbuild/find/adb321a4b8777b6fd1e7a2a6e29f49cce1b57517

View Change

To view, visit change 280165. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

[L] Change in dart/sdk[main]: [analysis_server] add a fix for `use_decorated_box`

Ahmed Ashour (Gerrit)

Brian Wilkerson (Gerrit)

Ahmed Ashour (Gerrit)

Brian Wilkerson (Gerrit)

Ahmed Ashour (Gerrit)

Brian Wilkerson (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Brian Wilkerson (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Brian Wilkerson (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Ahmed Ashour (Gerrit)

Brian Wilkerson (Gerrit)

Ahmed Ashour (Gerrit)

Commit Queue (Gerrit)

CBuild (Gerrit)