From 9a2e2801d97934ae99e4bde73351889fc4130929 Mon Sep 17 00:00:00 2001 From: pq Date: Tue, 10 Sep 2019 12:28:26 +0000 Subject: [PATCH] fix for avoid_relative_lib_imports Change-Id: Ib7ffd8cab3d1f0b22ebb9ad2d284c65d3194de15 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/116484 Reviewed-by: Brian Wilkerson Commit-Queue: Phil Quitslund --- .../lib/src/services/correction/assist.dart | 4 +- .../services/correction/assist_internal.dart | 48 +-- .../services/correction/base_processor.dart | 392 ++++++++++-------- .../lib/src/services/correction/fix.dart | 2 + .../src/services/correction/fix_internal.dart | 18 +- .../lib/src/services/linter/lint_names.dart | 1 + .../convert_to_package_import_test.dart | 14 +- .../fix/convert_to_package_import_test.dart | 38 ++ .../src/services/correction/fix/test_all.dart | 2 + 9 files changed, 294 insertions(+), 225 deletions(-) create mode 100644 pkg/analysis_server/test/src/services/correction/fix/convert_to_package_import_test.dart diff --git a/pkg/analysis_server/lib/src/services/correction/assist.dart b/pkg/analysis_server/lib/src/services/correction/assist.dart index 9cb8c62ec4125..0a0ea87c6e61d 100644 --- a/pkg/analysis_server/lib/src/services/correction/assist.dart +++ b/pkg/analysis_server/lib/src/services/correction/assist.dart @@ -104,9 +104,7 @@ class DartAssistKind { static const CONVERT_TO_PACKAGE_IMPORT = const AssistKind( 'dart.assist.convert.relativeToPackageImport', 30, - "Convert to 'package:' import", - // todo (pq): migrate to (conditional) fix - associatedErrorCodes: ['avoid_relative_lib_imports']); + "Convert to 'package:' import"); static const CONVERT_TO_SET_LITERAL = const AssistKind( 'dart.assist.convert.toSetLiteral', 30, "Convert to set literal", // todo (brianwilkerson): unify w/ fix diff --git a/pkg/analysis_server/lib/src/services/correction/assist_internal.dart b/pkg/analysis_server/lib/src/services/correction/assist_internal.dart index f3ba6127f068f..8635a2481b6d7 100644 --- a/pkg/analysis_server/lib/src/services/correction/assist_internal.dart +++ b/pkg/analysis_server/lib/src/services/correction/assist_internal.dart @@ -101,7 +101,11 @@ class AssistProcessor extends BaseProcessor { )) { await _addProposal_convertToNullAware(); } - await _addProposal_convertToPackageImport(); + if (!_containsErrorCode( + {LintNames.avoid_relative_lib_imports}, + )) { + await _addProposal_convertToPackageImport(); + } await _addProposal_convertToSingleQuotedString(); await _addProposal_encapsulateField(); await _addProposal_exchangeOperands(); @@ -1310,45 +1314,9 @@ class AssistProcessor extends BaseProcessor { } Future _addProposal_convertToPackageImport() async { - var node = this.node; - if (node is StringLiteral) { - node = node.parent; - } - if (node is ImportDirective) { - ImportDirective importDirective = node; - var uriSource = importDirective.uriSource; - - // Ignore if invalid URI. - if (uriSource == null) { - return; - } - - var importUri = uriSource.uri; - if (importUri.scheme != 'package') { - return; - } - - // Don't offer to convert a 'package:' URI to itself. - try { - if (Uri.parse(importDirective.uriContent).scheme == 'package') { - return; - } - } on FormatException { - return; - } - - var changeBuilder = _newDartChangeBuilder(); - await changeBuilder.addFileEdit(file, (builder) { - builder.addSimpleReplacement( - range.node(importDirective.uri), - "'$importUri'", - ); - }); - _addAssistFromBuilder( - changeBuilder, - DartAssistKind.CONVERT_TO_PACKAGE_IMPORT, - ); - } + final changeBuilder = await createBuilder_convertToPackageImport(); + _addAssistFromBuilder( + changeBuilder, DartAssistKind.CONVERT_TO_PACKAGE_IMPORT); } Future _addProposal_convertToSingleQuotedString() async { diff --git a/pkg/analysis_server/lib/src/services/correction/base_processor.dart b/pkg/analysis_server/lib/src/services/correction/base_processor.dart index a67b7ca94f74f..d8ed16e2381d4 100644 --- a/pkg/analysis_server/lib/src/services/correction/base_processor.dart +++ b/pkg/analysis_server/lib/src/services/correction/base_processor.dart @@ -155,182 +155,6 @@ abstract class BaseProcessor { return validChange ? changeBuilder : null; } - Future createBuilder_convertToNullAware() async { - AstNode node = this.node; - if (node is! ConditionalExpression) { - _coverageMarker(); - return null; - } - ConditionalExpression conditional = node; - Expression condition = conditional.condition.unParenthesized; - SimpleIdentifier identifier; - Expression nullExpression; - Expression nonNullExpression; - int periodOffset; - - if (condition is BinaryExpression) { - // - // Identify the variable being compared to `null`, or return if the - // condition isn't a simple comparison of `null` to a variable's value. - // - Expression leftOperand = condition.leftOperand; - Expression rightOperand = condition.rightOperand; - if (leftOperand is NullLiteral && rightOperand is SimpleIdentifier) { - identifier = rightOperand; - } else if (rightOperand is NullLiteral && - leftOperand is SimpleIdentifier) { - identifier = leftOperand; - } else { - _coverageMarker(); - return null; - } - if (identifier.staticElement is! LocalElement) { - _coverageMarker(); - return null; - } - // - // Identify the expression executed when the variable is `null` and when - // it is non-`null`. Return if the `null` expression isn't a null literal - // or if the non-`null` expression isn't a method invocation whose target - // is the save variable being compared to `null`. - // - if (condition.operator.type == TokenType.EQ_EQ) { - nullExpression = conditional.thenExpression; - nonNullExpression = conditional.elseExpression; - } else if (condition.operator.type == TokenType.BANG_EQ) { - nonNullExpression = conditional.thenExpression; - nullExpression = conditional.elseExpression; - } - if (nullExpression == null || nonNullExpression == null) { - _coverageMarker(); - return null; - } - if (nullExpression.unParenthesized is! NullLiteral) { - _coverageMarker(); - return null; - } - Expression unwrappedExpression = nonNullExpression.unParenthesized; - Expression target; - Token operator; - if (unwrappedExpression is MethodInvocation) { - target = unwrappedExpression.target; - operator = unwrappedExpression.operator; - } else if (unwrappedExpression is PrefixedIdentifier) { - target = unwrappedExpression.prefix; - operator = unwrappedExpression.period; - } else { - _coverageMarker(); - return null; - } - if (operator.type != TokenType.PERIOD) { - _coverageMarker(); - return null; - } - if (!(target is SimpleIdentifier && - target.staticElement == identifier.staticElement)) { - _coverageMarker(); - return null; - } - periodOffset = operator.offset; - - DartChangeBuilder changeBuilder = _newDartChangeBuilder(); - await changeBuilder.addFileEdit(file, (DartFileEditBuilder builder) { - builder.addDeletion(range.startStart(node, nonNullExpression)); - builder.addSimpleInsertion(periodOffset, '?'); - builder.addDeletion(range.endEnd(nonNullExpression, node)); - }); - return changeBuilder; - } - return null; - } - - Future createBuilder_convertToExpressionFunctionBody() async { - // prepare current body - FunctionBody body = getEnclosingFunctionBody(); - if (body is! BlockFunctionBody || body.isGenerator) { - _coverageMarker(); - return null; - } - // prepare return statement - List statements = (body as BlockFunctionBody).block.statements; - if (statements.length != 1) { - _coverageMarker(); - return null; - } - Statement onlyStatement = statements.first; - // prepare returned expression - Expression returnExpression; - if (onlyStatement is ReturnStatement) { - returnExpression = onlyStatement.expression; - } else if (onlyStatement is ExpressionStatement) { - returnExpression = onlyStatement.expression; - } - if (returnExpression == null) { - _coverageMarker(); - return null; - } - - // Return expressions can be quite large, e.g. Flutter build() methods. - // It is surprising to see this Quick Assist deep in the function body. - if (selectionOffset >= returnExpression.offset) { - _coverageMarker(); - return null; - } - - var changeBuilder = _newDartChangeBuilder(); - await changeBuilder.addFileEdit(file, (DartFileEditBuilder builder) { - builder.addReplacement(range.node(body), (DartEditBuilder builder) { - if (body.isAsynchronous) { - builder.write('async '); - } - builder.write('=> '); - builder.write(_getNodeText(returnExpression)); - if (body.parent is! FunctionExpression || - body.parent.parent is FunctionDeclaration) { - builder.write(';'); - } - }); - }); - return changeBuilder; - } - - /// Returns the text of the given node in the unit. - String /* TODO (pq): make visible */ _getNodeText(AstNode node) => - utils.getNodeText(node); - - FunctionBody getEnclosingFunctionBody() { - // TODO(brianwilkerson) Determine whether there is a reason why this method - // isn't just "return node.getAncestor((node) => node is FunctionBody);" - { - FunctionExpression function = - node.thisOrAncestorOfType(); - if (function != null) { - return function.body; - } - } - { - FunctionDeclaration function = - node.thisOrAncestorOfType(); - if (function != null) { - return function.functionExpression.body; - } - } - { - ConstructorDeclaration constructor = - node.thisOrAncestorOfType(); - if (constructor != null) { - return constructor.body; - } - } - { - MethodDeclaration method = node.thisOrAncestorOfType(); - if (method != null) { - return method.body; - } - } - return null; - } - Future createBuilder_addTypeAnnotation_VariableDeclaration() async { AstNode node = this.node; @@ -649,6 +473,56 @@ abstract class BaseProcessor { return changeBuilder; } + Future createBuilder_convertToExpressionFunctionBody() async { + // prepare current body + FunctionBody body = getEnclosingFunctionBody(); + if (body is! BlockFunctionBody || body.isGenerator) { + _coverageMarker(); + return null; + } + // prepare return statement + List statements = (body as BlockFunctionBody).block.statements; + if (statements.length != 1) { + _coverageMarker(); + return null; + } + Statement onlyStatement = statements.first; + // prepare returned expression + Expression returnExpression; + if (onlyStatement is ReturnStatement) { + returnExpression = onlyStatement.expression; + } else if (onlyStatement is ExpressionStatement) { + returnExpression = onlyStatement.expression; + } + if (returnExpression == null) { + _coverageMarker(); + return null; + } + + // Return expressions can be quite large, e.g. Flutter build() methods. + // It is surprising to see this Quick Assist deep in the function body. + if (selectionOffset >= returnExpression.offset) { + _coverageMarker(); + return null; + } + + var changeBuilder = _newDartChangeBuilder(); + await changeBuilder.addFileEdit(file, (DartFileEditBuilder builder) { + builder.addReplacement(range.node(body), (DartEditBuilder builder) { + if (body.isAsynchronous) { + builder.write('async '); + } + builder.write('=> '); + builder.write(_getNodeText(returnExpression)); + if (body.parent is! FunctionExpression || + body.parent.parent is FunctionDeclaration) { + builder.write(';'); + } + }); + }); + return changeBuilder; + } + Future createBuilder_convertToIntLiteral() async { if (node is! DoubleLiteral) { _coverageMarker(); @@ -676,6 +550,135 @@ abstract class BaseProcessor { return changeBuilder; } + Future createBuilder_convertToNullAware() async { + AstNode node = this.node; + if (node is! ConditionalExpression) { + _coverageMarker(); + return null; + } + ConditionalExpression conditional = node; + Expression condition = conditional.condition.unParenthesized; + SimpleIdentifier identifier; + Expression nullExpression; + Expression nonNullExpression; + int periodOffset; + + if (condition is BinaryExpression) { + // + // Identify the variable being compared to `null`, or return if the + // condition isn't a simple comparison of `null` to a variable's value. + // + Expression leftOperand = condition.leftOperand; + Expression rightOperand = condition.rightOperand; + if (leftOperand is NullLiteral && rightOperand is SimpleIdentifier) { + identifier = rightOperand; + } else if (rightOperand is NullLiteral && + leftOperand is SimpleIdentifier) { + identifier = leftOperand; + } else { + _coverageMarker(); + return null; + } + if (identifier.staticElement is! LocalElement) { + _coverageMarker(); + return null; + } + // + // Identify the expression executed when the variable is `null` and when + // it is non-`null`. Return if the `null` expression isn't a null literal + // or if the non-`null` expression isn't a method invocation whose target + // is the save variable being compared to `null`. + // + if (condition.operator.type == TokenType.EQ_EQ) { + nullExpression = conditional.thenExpression; + nonNullExpression = conditional.elseExpression; + } else if (condition.operator.type == TokenType.BANG_EQ) { + nonNullExpression = conditional.thenExpression; + nullExpression = conditional.elseExpression; + } + if (nullExpression == null || nonNullExpression == null) { + _coverageMarker(); + return null; + } + if (nullExpression.unParenthesized is! NullLiteral) { + _coverageMarker(); + return null; + } + Expression unwrappedExpression = nonNullExpression.unParenthesized; + Expression target; + Token operator; + if (unwrappedExpression is MethodInvocation) { + target = unwrappedExpression.target; + operator = unwrappedExpression.operator; + } else if (unwrappedExpression is PrefixedIdentifier) { + target = unwrappedExpression.prefix; + operator = unwrappedExpression.period; + } else { + _coverageMarker(); + return null; + } + if (operator.type != TokenType.PERIOD) { + _coverageMarker(); + return null; + } + if (!(target is SimpleIdentifier && + target.staticElement == identifier.staticElement)) { + _coverageMarker(); + return null; + } + periodOffset = operator.offset; + + DartChangeBuilder changeBuilder = _newDartChangeBuilder(); + await changeBuilder.addFileEdit(file, (DartFileEditBuilder builder) { + builder.addDeletion(range.startStart(node, nonNullExpression)); + builder.addSimpleInsertion(periodOffset, '?'); + builder.addDeletion(range.endEnd(nonNullExpression, node)); + }); + return changeBuilder; + } + return null; + } + + Future createBuilder_convertToPackageImport() async { + var node = this.node; + if (node is StringLiteral) { + node = node.parent; + } + if (node is ImportDirective) { + ImportDirective importDirective = node; + var uriSource = importDirective.uriSource; + + // Ignore if invalid URI. + if (uriSource == null) { + return null; + } + + var importUri = uriSource.uri; + if (importUri.scheme != 'package') { + return null; + } + + // Don't offer to convert a 'package:' URI to itself. + try { + if (Uri.parse(importDirective.uriContent).scheme == 'package') { + return null; + } + } on FormatException { + return null; + } + + var changeBuilder = _newDartChangeBuilder(); + await changeBuilder.addFileEdit(file, (builder) { + builder.addSimpleReplacement( + range.node(importDirective.uri), + "'$importUri'", + ); + }); + return changeBuilder; + } + return null; + } + @protected Future createBuilder_useCurlyBraces() async { Future doStatement(DoStatement node) async { @@ -803,6 +806,39 @@ abstract class BaseProcessor { return null; } + FunctionBody getEnclosingFunctionBody() { + // TODO(brianwilkerson) Determine whether there is a reason why this method + // isn't just "return node.getAncestor((node) => node is FunctionBody);" + { + FunctionExpression function = + node.thisOrAncestorOfType(); + if (function != null) { + return function.body; + } + } + { + FunctionDeclaration function = + node.thisOrAncestorOfType(); + if (function != null) { + return function.functionExpression.body; + } + } + { + ConstructorDeclaration constructor = + node.thisOrAncestorOfType(); + if (constructor != null) { + return constructor.body; + } + } + { + MethodDeclaration method = node.thisOrAncestorOfType(); + if (method != null) { + return method.body; + } + } + return null; + } + @protected bool setupCompute() { final locator = NodeLocator(selectionOffset, selectionEnd); @@ -822,6 +858,10 @@ abstract class BaseProcessor { } } + /// Returns the text of the given node in the unit. + String /* TODO (pq): make visible */ _getNodeText(AstNode node) => + utils.getNodeText(node); + DartChangeBuilder _newDartChangeBuilder() => DartChangeBuilderImpl.forWorkspace(workspace); diff --git a/pkg/analysis_server/lib/src/services/correction/fix.dart b/pkg/analysis_server/lib/src/services/correction/fix.dart index ff70d9385a1c2..a8ceb1b9ae395 100644 --- a/pkg/analysis_server/lib/src/services/correction/fix.dart +++ b/pkg/analysis_server/lib/src/services/correction/fix.dart @@ -199,6 +199,8 @@ class DartFixKind { 'CONVERT_TO_NAMED_ARGUMENTS', 50, "Convert to named arguments"); static const CONVERT_TO_NULL_AWARE = const FixKind('CONVERT_TO_NULL_AWARE', 50, "Convert to use '?.'"); + static const CONVERT_TO_PACKAGE_IMPORT = const FixKind( + 'CONVERT_TO_PACKAGE_IMPORT', 50, "Convert to 'package:' import"); static const CREATE_CLASS = const FixKind('CREATE_CLASS', 50, "Create class '{0}'"); static const CREATE_CONSTRUCTOR = 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 0a857ce3a8a8e..a20936838471e 100644 --- a/pkg/analysis_server/lib/src/services/correction/fix_internal.dart +++ b/pkg/analysis_server/lib/src/services/correction/fix_internal.dart @@ -585,6 +585,9 @@ class FixProcessor extends BaseProcessor { if (name == LintNames.avoid_init_to_null) { await _addFix_removeInitializer(); } + if (name == LintNames.avoid_relative_lib_imports) { + await _addFix_convertToPackageImport(); + } if (name == LintNames.avoid_return_types_on_setters) { await _addFix_removeTypeAnnotation(); } @@ -738,11 +741,6 @@ class FixProcessor extends BaseProcessor { _addFixFromBuilder(changeBuilder, DartFixKind.ADD_CURLY_BRACES); } - Future _addFix_convertToNullAware() async { - final changeBuilder = await createBuilder_convertToNullAware(); - _addFixFromBuilder(changeBuilder, DartFixKind.CONVERT_TO_NULL_AWARE); - } - Future _addFix_addExplicitCast() async { if (coveredNode is! Expression) { return; @@ -1449,6 +1447,16 @@ class FixProcessor extends BaseProcessor { } } + Future _addFix_convertToNullAware() async { + final changeBuilder = await createBuilder_convertToNullAware(); + _addFixFromBuilder(changeBuilder, DartFixKind.CONVERT_TO_NULL_AWARE); + } + + Future _addFix_convertToPackageImport() async { + final changeBuilder = await createBuilder_convertToPackageImport(); + _addFixFromBuilder(changeBuilder, DartFixKind.CONVERT_TO_PACKAGE_IMPORT); + } + Future _addFix_createClass() async { Element prefixElement = null; String name = null; 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 c52000bc99660..21884f36eeb9a 100644 --- a/pkg/analysis_server/lib/src/services/linter/lint_names.dart +++ b/pkg/analysis_server/lib/src/services/linter/lint_names.dart @@ -12,6 +12,7 @@ class LintNames { 'avoid_annotating_with_dynamic'; static const String avoid_empty_else = 'avoid_empty_else'; static const String avoid_init_to_null = 'avoid_init_to_null'; + static const String avoid_relative_lib_imports = 'avoid_relative_lib_imports'; static const String avoid_return_types_on_setters = 'avoid_return_types_on_setters'; static const String avoid_types_on_closure_parameters = diff --git a/pkg/analysis_server/test/src/services/correction/assist/convert_to_package_import_test.dart b/pkg/analysis_server/test/src/services/correction/assist/convert_to_package_import_test.dart index 84df167f42ece..9da35b69e1706 100644 --- a/pkg/analysis_server/test/src/services/correction/assist/convert_to_package_import_test.dart +++ b/pkg/analysis_server/test/src/services/correction/assist/convert_to_package_import_test.dart @@ -3,6 +3,7 @@ // BSD-style license that can be found in the LICENSE file. import 'package:analysis_server/src/services/correction/assist.dart'; +import 'package:analysis_server/src/services/linter/lint_names.dart'; import 'package:analyzer_plugin/utilities/assist/assist.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -52,7 +53,7 @@ import ':[invalidUri]'; test_nonPackage_Uri() async { addSource('/home/test/lib/foo.dart', ''); - + testFile = convertPath('/home/test/lib/src/test.dart'); await resolveTestUnit(''' import 'dart:core'; '''); @@ -83,4 +84,15 @@ import '../foo/bar.dart'; import 'package:test/foo/bar.dart'; '''); } + + test_relativeImport_noAssistWithLint() async { + createAnalysisOptionsFile(lints: [LintNames.avoid_relative_lib_imports]); + verifyNoTestUnitErrors = false; + addSource('/home/test/lib/foo.dart', ''); + + await resolveTestUnit(''' +import '../lib/foo.dart'; +'''); + await assertNoAssist(); + } } diff --git a/pkg/analysis_server/test/src/services/correction/fix/convert_to_package_import_test.dart b/pkg/analysis_server/test/src/services/correction/fix/convert_to_package_import_test.dart new file mode 100644 index 0000000000000..dfa470615afa5 --- /dev/null +++ b/pkg/analysis_server/test/src/services/correction/fix/convert_to_package_import_test.dart @@ -0,0 +1,38 @@ +// Copyright (c) 2019, 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_reflective_loader/test_reflective_loader.dart'; + +import 'fix_processor.dart'; + +main() { + defineReflectiveSuite(() { + defineReflectiveTests(ConvertToPackageImportTest); + }); +} + +@reflectiveTest +class ConvertToPackageImportTest extends FixProcessorLintTest { + @override + FixKind get kind => DartFixKind.CONVERT_TO_PACKAGE_IMPORT; + + @override + String get lintCode => LintNames.avoid_relative_lib_imports; + + /// More coverage in the `convert_to_package_import_test.dart` assist test. + test_relativeImport() async { + addSource('/home/test/lib/foo.dart', ''); + testFile = convertPath('/home/test/lib/src/test.dart'); + await resolveTestUnit(''' +import /*LINT*/'../lib/foo.dart'; +'''); + + await assertHasFix(''' +import /*LINT*/'package:test/lib/foo.dart'; +'''); + } +} 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 67efef9bd755e..a491ac53e6fb7 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 @@ -43,6 +43,7 @@ import 'convert_to_if_element_test.dart' as convert_to_if_element; import 'convert_to_int_literal_test.dart' as convert_to_int_literal; import 'convert_to_named_arguments_test.dart' as convert_to_named_arguments; import 'convert_to_null_aware_test.dart' as convert_to_null_aware; +import 'convert_to_package_import_test.dart' as convert_to_package_import; import 'create_class_test.dart' as create_class; import 'create_constructor_for_final_fields_test.dart' as create_constructor_for_final_field; @@ -156,6 +157,7 @@ main() { convert_to_int_literal.main(); convert_to_named_arguments.main(); convert_to_null_aware.main(); + convert_to_package_import.main(); create_class.main(); create_constructor_for_final_field.main(); create_constructor_super.main();