Add an assist to shadow a field when a local variable might be promotable

This is a fairly limited initial version of the assist. In addition to
implementation comments I'm interested in knowing whether there is value
in committing it as-is or whether it should be enhanced first.

Change-Id: Ia28328e1e5a759c0cec3936b334f719927f46dcf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134620
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/lib/src/services/correction/assist.dart

@@ -199,6 +199,8 @@ class DartAssistKind {
       'dart.assist.convert.replaceWithVar',
       30,
       "Replace type annotation with 'var'");
+  static const SHADOW_FIELD = AssistKind('dart.assist.shadowField', 30,
+      'Create a local variable that shadows the field');
   static const SORT_CHILD_PROPERTY_LAST = AssistKind(
       'dart.assist.sort.child.properties.last',
       30,

pkg/analysis_server/lib/src/services/correction/assist_internal.dart

@@ -16,6 +16,7 @@ import 'package:analysis_server/src/services/correction/dart/convert_to_map_lite
 import 'package:analysis_server/src/services/correction/dart/convert_to_null_aware.dart';
 import 'package:analysis_server/src/services/correction/dart/convert_to_set_literal.dart';
 import 'package:analysis_server/src/services/correction/dart/exchange_operands.dart';
+import 'package:analysis_server/src/services/correction/dart/shadow_field.dart';
 import 'package:analysis_server/src/services/correction/dart/split_and_condition.dart';
 import 'package:analysis_server/src/services/correction/name_suggestion.dart';
 import 'package:analysis_server/src/services/correction/selection_analyzer.dart';
@@ -293,6 +294,10 @@ class AssistProcessor extends BaseProcessor {
       ExchangeOperands(),
       DartAssistKind.EXCHANGE_OPERANDS,
     );
+    await compute(
+      ShadowField(),
+      DartAssistKind.SHADOW_FIELD,
+    );
     await compute(
       SplitAndCondition(),
       DartAssistKind.SPLIT_AND_CONDITION,

pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart

@@ -25,6 +25,8 @@ abstract class CorrectionProducer {
 
   AstNode get node => _context.node;
 
+  ResolvedUnitResult get resolvedResult => _context.resolvedResult;
+
   int get selectionLength => _context.selectionLength;
 
   int get selectionOffset => _context.selectionOffset;

pkg/analysis_server/lib/src/services/correction/dart/shadow_field.dart

@@ -0,0 +1,118 @@
+// 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/dart/abstract_producer.dart';
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
+import 'package:analyzer/dart/ast/visitor.dart';
+import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer_plugin/utilities/change_builder/change_builder_dart.dart';
+
+class ShadowField extends CorrectionProducer {
+  @override
+  Future<void> compute(DartChangeBuilder builder) async {
+    if (node is! SimpleIdentifier) {
+      return;
+    }
+    var element = (node as SimpleIdentifier).staticElement;
+    if (element is! PropertyAccessorElement) {
+      return;
+    }
+    var accessor = element as PropertyAccessorElement;
+    if (!accessor.isGetter || accessor.enclosingElement is! ClassElement) {
+      // TODO(brianwilkerson) Should we also require that the getter be synthetic?
+      return;
+    }
+    var statement = getStatement();
+    if (statement == null) {
+      return;
+    }
+    if (statement.parent is! Block) {
+      // TODO(brianwilkerson) Support adding a block between the statement and
+      //  its parent (where the parent will be something like a while or if
+      //  statement). Also support the case where the parent is a case clause.
+      return;
+    }
+    var enclosingBlock = statement.parent as Block;
+    _ReferenceFinder finder = _ReferenceFinder(accessor.correspondingSetter);
+    enclosingBlock.accept(finder);
+    if (finder.hasSetterReference) {
+      return;
+    }
+    var fieldName = accessor.name;
+    var offset = statement.offset;
+    var prefix = utils.getLinePrefix(offset);
+    //
+    // Build the change.
+    //
+    await builder.addFileEdit(file, (DartFileEditBuilder builder) {
+      builder.addInsertion(offset, (builder) {
+        // TODO(brianwilkerson) Conditionally write a type annotation instead of
+        //  'var' when we're able to discover user preferences.
+        builder.write('var ');
+        builder.write(fieldName);
+        builder.write(' = this.');
+        builder.write(fieldName);
+        builder.writeln(';');
+        builder.write(prefix);
+      });
+      // TODO(brianwilkerson) Consider removing unnecessary casts and null
+      //  checks that are no longer needed because promotion works. This would
+      //  be dependent on whether enhanced promotion is supported in the library
+      //  being edited.
+    });
+  }
+
+  /// Return the statement immediately enclosing the [node] that would promote
+  /// the type of the field if it were replaced by a local variable.
+  Statement getStatement() {
+    var parent = node.parent;
+
+    Statement enclosingIf(Expression expression) {
+      var parent = expression.parent;
+      while (parent is BinaryExpression) {
+        var opType = (parent as BinaryExpression).operator.type;
+        if (opType != TokenType.AMPERSAND_AMPERSAND) {
+          break;
+        }
+        parent = parent.parent;
+      }
+      if (parent is IfStatement) {
+        return parent;
+      }
+      return null;
+    }
+
+    if (parent is IsExpression && parent.expression == node) {
+      return enclosingIf(parent);
+    } else if (parent is BinaryExpression &&
+        resolvedResult.libraryElement.isNonNullableByDefault) {
+      var opType = parent.operator.type;
+      if (opType == TokenType.EQ_EQ || opType == TokenType.BANG_EQ) {
+        return enclosingIf(parent);
+      }
+    }
+    return null;
+  }
+}
+
+/// A utility that will find any references to a setter within an AST structure.
+class _ReferenceFinder extends RecursiveAstVisitor<void> {
+  /// The setter being searched for.
+  final PropertyAccessorElement setter;
+
+  /// A flag indicating whether a reference to the [setter] has been found.
+  bool hasSetterReference = false;
+
+  /// Initialize a newly created reference finder to find references to the
+  /// given [setter].
+  _ReferenceFinder(this.setter);
+
+  @override
+  void visitSimpleIdentifier(SimpleIdentifier node) {
+    if (node.staticElement == setter) {
+      hasSetterReference = true;
+    }
+  }
+}

pkg/analysis_server/test/src/services/correction/assist/shadow_field_test.dart

@@ -0,0 +1,188 @@
+// 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/assist.dart';
+import 'package:analyzer_plugin/utilities/assist/assist.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import 'assist_processor.dart';
+
+void main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(ShadowFieldTest);
+    defineReflectiveTests(ShadowFieldWithNNBDTest);
+  });
+}
+
+@reflectiveTest
+class ShadowFieldTest extends AssistProcessorTest {
+  @override
+  AssistKind get kind => DartAssistKind.SHADOW_FIELD;
+
+  Future<void> test_is_assigned() async {
+    await resolveTestUnit('''
+class C {
+  num f = 0;
+
+  void m() {
+    if (f is int) {
+      print(f.abs());
+    }
+    f = 0;
+    print(f.abs());
+  }
+}
+''');
+    await assertNoAssistAt('f is');
+  }
+
+  Future<void> test_is_noBlock_while() async {
+    await resolveTestUnit('''
+class C {
+  num f = 0;
+
+  void m() {
+    while (true) 
+      if (f is int) {
+        print((f as int).abs());
+      }
+  }
+}
+''');
+    await assertNoAssistAt('f is');
+  }
+
+  Future<void> test_is_referencedViaThis() async {
+    await resolveTestUnit('''
+class C {
+  num f = 0;
+
+  void m() {
+    if (f is int) {
+      print(this.f);
+    }
+  }
+}
+''');
+    await assertHasAssistAt('f is', '''
+class C {
+  num f = 0;
+
+  void m() {
+    var f = this.f;
+    if (f is int) {
+      print(this.f);
+    }
+  }
+}
+''');
+  }
+
+  Future<void> test_is_simple() async {
+    await resolveTestUnit('''
+class C {
+  num f = 0;
+
+  void m() {
+    if (f is int) {
+      print((f as int).abs());
+    }
+  }
+}
+''');
+    await assertHasAssistAt('f is', '''
+class C {
+  num f = 0;
+
+  void m() {
+    var f = this.f;
+    if (f is int) {
+      print((f as int).abs());
+    }
+  }
+}
+''');
+  }
+}
+
+@reflectiveTest
+class ShadowFieldWithNNBDTest extends ShadowFieldTest {
+  @override
+  AssistKind get kind => DartAssistKind.SHADOW_FIELD;
+
+  @override
+  void setUp() {
+    super.setUp();
+    createAnalysisOptionsFile(experiments: ['non-nullable']);
+  }
+
+  Future<void> test_notNull() async {
+    await resolveTestUnit('''
+class C {
+  int? f;
+
+  void m() {
+    if (f != null) {
+      print(f!.abs());
+    }
+  }
+}
+''');
+    await assertHasAssistAt('f !=', '''
+class C {
+  int? f;
+
+  void m() {
+    var f = this.f;
+    if (f != null) {
+      print(f!.abs());
+    }
+  }
+}
+''');
+  }
+
+  Future<void> test_notNull_assigned() async {
+    await resolveTestUnit('''
+class C {
+  int? f;
+
+  void m() {
+    if (f != null) {
+      print(f!.abs());
+    }
+    f = 0;
+    print(f!.abs());
+  }
+}
+''');
+    await assertNoAssistAt('f != ');
+  }
+
+  Future<void> test_notNull_referencedViaThis() async {
+    await resolveTestUnit('''
+class C {
+  int? f;
+
+  void m() {
+    if (f != null) {
+      print(this.f!);
+    }
+  }
+}
+''');
+    await assertHasAssistAt('f != ', '''
+class C {
+  int? f;
+
+  void m() {
+    var f = this.f;
+    if (f != null) {
+      print(this.f!);
+    }
+  }
+}
+''');
+  }
+}

pkg/analysis_server/test/src/services/correction/assist/test_all.dart

@@ -72,6 +72,7 @@ import 'replace_conditional_with_if_else_test.dart'
 import 'replace_if_else_with_conditional_test.dart'
     as replace_if_else_with_conditional;
 import 'replace_with_var_test.dart' as replace_with_var;
+import 'shadow_field_test.dart' as shadow_field;
 import 'sort_child_property_last_test.dart' as sort_child_property_last;
 import 'split_and_condition_test.dart' as split_and_condition;
 import 'split_variable_declaration_test.dart' as split_variable_declaration;
@@ -146,6 +147,7 @@ void main() {
     replace_conditional_with_if_else.main();
     replace_if_else_with_conditional.main();
     replace_with_var.main();
+    shadow_field.main();
     sort_child_property_last.main();
     split_and_condition.main();
     split_variable_declaration.main();

pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart

@@ -9,6 +9,7 @@ import 'package:analyzer/dart/analysis/session.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/dart/element/nullability_suffix.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/dart/element/type_provider.dart';
 import 'package:analyzer/exception/exception.dart';
@@ -1090,6 +1091,9 @@ class DartEditBuilderImpl extends EditBuilderImpl implements DartEditBuilder {
     // Write the simple name.
     String name = element.displayName;
     write(name);
+    if (type.nullabilitySuffix == NullabilitySuffix.question) {
+      write('?');
+    }
 
     // Write type arguments.
     if (type is ParameterizedType) {