From 59fee86145de8352c381d984c8ccfac4b3db154c Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 31 Mar 2025 09:23:29 -0700 Subject: [PATCH] DAS: Rearrange code to promote variables better The motivation here was to remove the local duplicate variables like `final_foo` or `foo_final`. These variables only existed because the variables they duplicate lose their promoted types inside the `builder.addDartFileEdit` closure. They lose their promoted type because they are multiply assigned in loops or not always promoted to be non-null. Often the fix is to replace a `if (x == null) return` with `} else { return; }`. I think this code more directly represents the flow of code, rather than relying on the nullity of a variable, like "is this variable still null? Oh then we must not have entered any of the situations above, so we should return." Change-Id: I9dd6686b7f4d9c6caf59c171179b129a5a957ba8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418160 Reviewed-by: Phil Quitslund Commit-Queue: Samuel Rawlins --- .../services/correction/dart/add_async.dart | 14 +++--- .../dart/add_missing_enum_case_clauses.dart | 4 +- .../dart/change_to_static_access.dart | 44 ++++++++++--------- .../convert_to_expression_function_body.dart | 18 ++++---- .../dart/convert_to_relative_import.dart | 4 +- .../dart/remove_unused_parameter.dart | 19 +++----- .../dart/replace_null_with_closure.dart | 11 +++-- .../dart/replace_with_null_aware.dart | 14 +++--- .../dart/update_sdk_constraints.dart | 8 ++-- 9 files changed, 64 insertions(+), 72 deletions(-) diff --git a/pkg/analysis_server/lib/src/services/correction/dart/add_async.dart b/pkg/analysis_server/lib/src/services/correction/dart/add_async.dart index 51682cd772d..ff833b7d84a 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/add_async.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/add_async.dart @@ -39,8 +39,8 @@ class AddAsync extends ResolvedCorrectionProducer { switch (_type) { case _Type.missingReturn: var node = this.node; - FunctionBody? body; - DartType? returnType; + FunctionBody body; + DartType returnType; switch (node) { case FunctionDeclaration(): body = node.functionExpression.body; @@ -48,18 +48,18 @@ class AddAsync extends ResolvedCorrectionProducer { returnType = declaredElement.returnType; } else if (node.declaredFragment case var declaredFragment?) { returnType = declaredFragment.element.returnType; + } else { + return; } case MethodDeclaration(): body = node.body; returnType = node.declaredFragment!.element.returnType; - } - if (body == null || returnType == null) { - return; + default: + return; } if (_isFutureVoid(returnType) && _hasNoReturns(body)) { - var final_body = body; await builder.addDartFileEdit(file, (builder) { - builder.addSimpleInsertion(final_body.offset, 'async '); + builder.addSimpleInsertion(body.offset, 'async '); }); } case _Type.wrongReturnType: diff --git a/pkg/analysis_server/lib/src/services/correction/dart/add_missing_enum_case_clauses.dart b/pkg/analysis_server/lib/src/services/correction/dart/add_missing_enum_case_clauses.dart index e489ec66ab8..dc030329b71 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/add_missing_enum_case_clauses.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/add_missing_enum_case_clauses.dart @@ -83,7 +83,7 @@ class AddMissingEnumCaseClauses extends ResolvedCorrectionProducer { var singleIndent = utils.oneIndent; var prefixString = prefix.isNotEmpty ? '$prefix.' : ''; - var enumName_final = '$prefixString$enumName'; + var prefixedEnumName = '$prefixString$enumName'; await builder.addDartFileEdit(file, (builder) { builder.insertCaseClauseAtEnd( switchKeyword: statement.switchKeyword, @@ -110,7 +110,7 @@ class AddMissingEnumCaseClauses extends ResolvedCorrectionProducer { // TODO(brianwilkerson): Consider inserting the names in order into the // switch statement. for (var constantName in unhandledEnumCases) { - addMissingCase('$enumName_final.$constantName'); + addMissingCase('$prefixedEnumName.$constantName'); } if (unhandledNullValue) { addMissingCase('null'); diff --git a/pkg/analysis_server/lib/src/services/correction/dart/change_to_static_access.dart b/pkg/analysis_server/lib/src/services/correction/dart/change_to_static_access.dart index 5ff483f3022..460300a69b2 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/change_to_static_access.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/change_to_static_access.dart @@ -28,28 +28,32 @@ class ChangeToStaticAccess extends ResolvedCorrectionProducer { @override Future compute(ChangeBuilder builder) async { - Expression? target; - Element2? invokedElement; var identifier = node; - if (identifier is SimpleIdentifier) { - var parent = identifier.parent; - if (parent is MethodInvocation) { - if (parent.methodName == identifier) { - target = parent.target; - invokedElement = identifier.element; - } - } else if (parent is PrefixedIdentifier) { - if (parent.identifier == identifier) { - target = parent.prefix; - invokedElement = identifier.element; - } - } - } - if (target == null || invokedElement is! ExecutableElement2) { + if (identifier is! SimpleIdentifier) { + return; + } + + Expression target; + var parent = identifier.parent; + if (parent case MethodInvocation(target: var parentTarget?)) { + if (parent.methodName != identifier) { + return; + } + target = parentTarget; + } else if (parent is PrefixedIdentifier) { + if (parent.identifier != identifier) { + return; + } + target = parent.prefix; + } else { + return; + } + + var invokedElement = identifier.element; + if (invokedElement is! ExecutableElement2) { return; } - var target_final = target; var declaringElement = invokedElement.enclosingElement2; if (declaringElement is InterfaceElement2) { @@ -57,7 +61,7 @@ class ChangeToStaticAccess extends ResolvedCorrectionProducer { if (declaringElementName != null) { _className = declaringElementName; await builder.addDartFileEdit(file, (builder) { - builder.addReplacement(range.node(target_final), (builder) { + builder.addReplacement(range.node(target), (builder) { builder.writeReference(declaringElement); }); }); @@ -67,7 +71,7 @@ class ChangeToStaticAccess extends ResolvedCorrectionProducer { if (extensionName != null) { _className = extensionName; await builder.addDartFileEdit(file, (builder) { - builder.addReplacement(range.node(target_final), (builder) { + builder.addReplacement(range.node(target), (builder) { builder.writeReference(declaringElement); }); }); diff --git a/pkg/analysis_server/lib/src/services/correction/dart/convert_to_expression_function_body.dart b/pkg/analysis_server/lib/src/services/correction/dart/convert_to_expression_function_body.dart index 1020739319f..85b0a1d32c6 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/convert_to_expression_function_body.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/convert_to_expression_function_body.dart @@ -50,11 +50,11 @@ class ConvertToExpressionFunctionBody extends ResolvedCorrectionProducer { if (statements.length != 1) { return; } - var onlyStatement = statements.first; - // prepare returned expression - Expression? returnExpression; - if (onlyStatement is ReturnStatement) { - returnExpression = onlyStatement.expression; + var onlyStatement = statements.single; + // Prepare the returned expression. + Expression returnExpression; + if (onlyStatement case ReturnStatement(:var expression?)) { + returnExpression = expression; if (onlyStatement.returnKeyword.precedingComments != null) { // TODO(srawlins): Include comments in fixed output. // https://github.com/dart-lang/sdk/issues/29313 @@ -82,25 +82,23 @@ class ConvertToExpressionFunctionBody extends ResolvedCorrectionProducer { // https://github.com/dart-lang/sdk/issues/29313 return; } - } - if (returnExpression == null) { + } else { return; } - // Return expressions can be quite large, e.g. Flutter build() methods. + // 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) { return; } - var returnExpression_final = returnExpression; await builder.addDartFileEdit(file, (builder) { builder.addReplacement(range.node(body), (builder) { if (body.isAsynchronous) { builder.write('async '); } builder.write('=> '); - builder.write(utils.getNodeText(returnExpression_final)); + builder.write(utils.getNodeText(returnExpression)); var parent = body.parent; if (parent is! FunctionExpression || parent.parent is FunctionDeclaration) { diff --git a/pkg/analysis_server/lib/src/services/correction/dart/convert_to_relative_import.dart b/pkg/analysis_server/lib/src/services/correction/dart/convert_to_relative_import.dart index 044125ee12e..2e1528ed78f 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/convert_to_relative_import.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/convert_to_relative_import.dart @@ -74,10 +74,10 @@ class ConvertToRelativeImport extends ResolvedCorrectionProducer { from: path.dirname(sourceUri.path), ); - var node_final = targetNode; + var uriNode = targetNode.uri; await builder.addDartFileEdit(file, (builder) { builder.addSimpleReplacement( - range.node(node_final.uri).getExpanded(-1), + range.node(uriNode).getExpanded(-1), relativePath, ); }); diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart index f0443376afc..50dd19a4a02 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart @@ -25,23 +25,20 @@ class RemoveUnusedParameter extends ResolvedCorrectionProducer { @override Future compute(ChangeBuilder builder) async { - // To work for the unused_parameter hint as well as the lint, we must - // allow for passing in `SimpleIdentifier`s. + // To work for the unused_element_parameter warning as well as the lint (?), + // we must allow for passing in `SimpleIdentifier`s. var maybeParameter = node; var maybeParameterParent = maybeParameter.parent; if (maybeParameter is SimpleIdentifier && maybeParameterParent != null) { maybeParameter = maybeParameterParent; } - var parameter = maybeParameter; - if (parameter is! FormalParameter) { + if (maybeParameter is! FormalParameter) { return; } - var parent = parameter.parent; - if (parent is DefaultFormalParameter) { - parameter = parent; - } + var parent = maybeParameter.parent; + var parameter = parent is DefaultFormalParameter ? parent : maybeParameter; var parameterList = parameter.parent; if (parameterList is! FormalParameterList) { @@ -50,7 +47,6 @@ class RemoveUnusedParameter extends ResolvedCorrectionProducer { var parameters = parameterList.parameters; var index = parameters.indexOf(parameter); - var parameter_final = parameter; await builder.addDartFileEdit(file, (builder) { if (index == 0) { // Remove the first parameter in the list. @@ -65,7 +61,7 @@ class RemoveUnusedParameter extends ResolvedCorrectionProducer { ); } else { var following = parameters[1]; - if (parameter_final.isRequiredPositional && + if (parameter.isRequiredPositional && !following.isRequiredPositional) { // The parameter to be removed and the following parameter are not // of the same kind, so there is a delimiter between them that we @@ -85,8 +81,7 @@ class RemoveUnusedParameter extends ResolvedCorrectionProducer { } } else { var preceding = parameters[index - 1]; - if (preceding.isRequiredPositional && - !parameter_final.isRequiredPositional) { + if (preceding.isRequiredPositional && !parameter.isRequiredPositional) { // The parameter to be removed and the preceding parameter are not // of the same kind, so there is a delimiter between them. if (index == parameters.length - 1) { diff --git a/pkg/analysis_server/lib/src/services/correction/dart/replace_null_with_closure.dart b/pkg/analysis_server/lib/src/services/correction/dart/replace_null_with_closure.dart index 5b6961d1f08..6618f89a9ed 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/replace_null_with_closure.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/replace_null_with_closure.dart @@ -26,7 +26,7 @@ class ReplaceNullWithClosure extends ResolvedCorrectionProducer { @override Future compute(ChangeBuilder builder) async { - AstNode? nodeToFix; + AstNode nodeToFix; var parameters = const []; var coveringNode = this.coveringNode; @@ -41,18 +41,17 @@ class ReplaceNullWithClosure extends ResolvedCorrectionProducer { } } nodeToFix = expression; + } else { + return; } } else if (coveringNode is NullLiteral) { nodeToFix = coveringNode; - } - - if (nodeToFix == null) { + } else { return; } - var nodeToFix_final = nodeToFix; await builder.addDartFileEdit(file, (builder) { - builder.addReplacement(range.node(nodeToFix_final), (builder) { + builder.addReplacement(range.node(nodeToFix), (builder) { builder.writeFormalParameters(parameters); builder.write(' => null'); }); diff --git a/pkg/analysis_server/lib/src/services/correction/dart/replace_with_null_aware.dart b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_null_aware.dart index e2d1e8c99da..a3af841ee5a 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/replace_with_null_aware.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_null_aware.dart @@ -45,11 +45,9 @@ class ReplaceWithNullAware extends ResolvedCorrectionProducer { } Future _computeInChain(ChangeBuilder builder) async { - var node = coveringNode; - if (node is Expression) { - var node_final = node; - await builder.addDartFileEdit(file, (builder) { - var parent = node_final.parent; + await builder.addDartFileEdit(file, (builder) { + var node = coveringNode; + if (node case Expression(:var parent)) { while (parent != null) { if (parent is MethodInvocation && parent.target == node) { var operator = parent.operator; @@ -62,10 +60,10 @@ class ReplaceWithNullAware extends ResolvedCorrectionProducer { break; } node = parent; - parent = node?.parent; + parent = node.parent; } - }); - } + } + }); } Future _computeSingle(ChangeBuilder builder) async { diff --git a/pkg/analysis_server/lib/src/services/correction/dart/update_sdk_constraints.dart b/pkg/analysis_server/lib/src/services/correction/dart/update_sdk_constraints.dart index 48d8c256772..1a945c504c1 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/update_sdk_constraints.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/update_sdk_constraints.dart @@ -49,7 +49,7 @@ class UpdateSdkConstraints extends ResolvedCorrectionProducer { length = spaceOffset; } - String? newText; + String newText; if (text == 'any') { newText = '^$_minimumVersion'; } else if (text.startsWith('^')) { @@ -58,14 +58,12 @@ class UpdateSdkConstraints extends ResolvedCorrectionProducer { newText = '>=$_minimumVersion'; } else if (text.startsWith('>')) { newText = '>=$_minimumVersion'; - } - if (newText == null) { + } else { return; } - var newText_final = newText; await builder.addYamlFileEdit(pubspecFile.path, (builder) { - builder.addSimpleReplacement(SourceRange(offset, length), newText_final); + builder.addSimpleReplacement(SourceRange(offset, length), newText); }); }