Skip to content

Commit e4b9170

Browse files
srawlinsCommit Queue
srawlins
authored and
Commit Queue
committed
DAS: Support more possible added return types.
* If there are no return statements, suggest a `void` or `Future<void>` return type. * Visit yield statements as well, to suggest proper return types in generators. * Combine the 'return type computers' in extract_method.dart and add_return_type.dart int one. Change-Id: I223a50b11e7e77183f166c193fa74b8a2800127e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/401862 Reviewed-by: Phil Quitslund <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]>
1 parent 6272722 commit e4b9170

File tree

5 files changed

+164
-97
lines changed

5 files changed

+164
-97
lines changed

‎pkg/analysis_server/lib/src/services/correction/dart/add_return_type.dart‎

Lines changed: 15 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,11 @@
44

55
import 'package:analysis_server/src/services/correction/assist.dart';
66
import 'package:analysis_server/src/services/correction/fix.dart';
7+
import 'package:analysis_server/src/services/correction/util.dart';
78
import 'package:analysis_server_plugin/edit/dart/correction_producer.dart';
89
import 'package:analyzer/dart/ast/ast.dart';
910
import 'package:analyzer/dart/ast/token.dart';
10-
import 'package:analyzer/dart/ast/visitor.dart';
1111
import 'package:analyzer/dart/element/type.dart';
12-
import 'package:analyzer/dart/element/type_system.dart';
1312
import 'package:analyzer/src/dart/ast/extensions.dart';
1413
import 'package:analyzer_plugin/utilities/assist/assist.dart';
1514
import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
@@ -82,20 +81,20 @@ class AddReturnType extends ResolvedCorrectionProducer {
8281
/// Returns the type of value returned by the function [body], or `null` if a
8382
/// type can't be inferred.
8483
DartType? _inferReturnType(FunctionBody body) {
85-
DartType? baseType;
86-
if (body is ExpressionFunctionBody) {
87-
baseType = body.expression.typeOrThrow;
88-
} else if (body is BlockFunctionBody) {
89-
var computer = _ReturnTypeComputer(unitResult.typeSystem);
90-
body.block.accept(computer);
91-
baseType = computer.returnType;
92-
if (baseType == null && computer.hasReturn) {
93-
baseType = typeProvider.voidType;
94-
}
95-
}
96-
97-
if (baseType == null) {
98-
return null;
84+
DartType baseType;
85+
switch (body) {
86+
case BlockFunctionBody():
87+
var computer = ReturnTypeComputer(
88+
unitResult.typeSystem,
89+
isGenerator: body.isGenerator,
90+
);
91+
body.block.accept(computer);
92+
baseType = computer.returnType ?? typeProvider.voidType;
93+
case ExpressionFunctionBody():
94+
baseType = body.expression.typeOrThrow;
95+
case EmptyFunctionBody():
96+
case NativeFunctionBody():
97+
return null;
9998
}
10099

101100
if (body.isAsynchronous) {
@@ -119,37 +118,3 @@ class AddReturnType extends ResolvedCorrectionProducer {
119118
return baseType;
120119
}
121120
}
122-
123-
/// Copied from lib/src/services/refactoring/extract_method.dart", but
124-
/// [hasReturn] was added.
125-
// TODO(brianwilkerson): Decide whether to unify the two classes.
126-
class _ReturnTypeComputer extends RecursiveAstVisitor<void> {
127-
final TypeSystem typeSystem;
128-
129-
DartType? returnType;
130-
131-
/// A flag indicating whether at least one return statement was found.
132-
bool hasReturn = false;
133-
134-
_ReturnTypeComputer(this.typeSystem);
135-
136-
@override
137-
void visitBlockFunctionBody(BlockFunctionBody node) {}
138-
139-
@override
140-
void visitReturnStatement(ReturnStatement node) {
141-
hasReturn = true;
142-
// prepare expression
143-
var type = node.expression?.typeOrThrow;
144-
if (type == null || type.isBottom) {
145-
return;
146-
}
147-
// combine types
148-
var current = returnType;
149-
if (current == null) {
150-
returnType = type;
151-
} else {
152-
returnType = typeSystem.leastUpperBound(current, type);
153-
}
154-
}
155-
}

‎pkg/analysis_server/lib/src/services/correction/util.dart‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,11 @@ import 'package:_fe_analyzer_shared/src/scanner/token.dart';
88
import 'package:analyzer/dart/ast/precedence.dart';
99
import 'package:analyzer/dart/ast/visitor.dart';
1010
import 'package:analyzer/dart/element/element2.dart';
11+
import 'package:analyzer/dart/element/type.dart';
12+
import 'package:analyzer/dart/element/type_system.dart';
1113
import 'package:analyzer/source/source_range.dart';
1214
import 'package:analyzer/src/dart/ast/ast.dart';
15+
import 'package:analyzer/src/dart/ast/extensions.dart';
1316
import 'package:analyzer/src/utilities/extensions/ast.dart';
1417
import 'package:analyzer_plugin/utilities/range_factory.dart';
1518
import 'package:path/path.dart' as path;
@@ -48,8 +51,6 @@ List<AstNode> findLocalElementReferences(AstNode root, LocalElement2 element) {
4851
return collector.references;
4952
}
5053

51-
// TODO(scheglov): replace with nodes once there will be
52-
// [CompilationUnit.getComments].
5354
/// Returns [SourceRange]s of all comments in [unit].
5455
List<SourceRange> getCommentRanges(CompilationUnit unit) {
5556
var ranges = <SourceRange>[];
@@ -65,6 +66,8 @@ List<SourceRange> getCommentRanges(CompilationUnit unit) {
6566
return ranges;
6667
}
6768

69+
// TODO(scheglov): replace with nodes once there will be
70+
// [CompilationUnit.getComments].
6871
/// Return all [LocalElement2]s defined in the given [node].
6972
List<LocalElement2> getDefinedLocalElements(AstNode node) {
7073
var collector = _LocalElementsCollector();
@@ -370,6 +373,58 @@ bool _allListsIdentical(List<List<Object>> lists, int position) {
370373
return true;
371374
}
372375

376+
/// Computes a valid return type for a function based on the contained return
377+
/// statements.
378+
class ReturnTypeComputer extends RecursiveAstVisitor<void> {
379+
final TypeSystem _typeSystem;
380+
381+
final bool _isGenerator;
382+
383+
DartType? returnType;
384+
385+
ReturnTypeComputer(this._typeSystem, {bool isGenerator = false})
386+
: _isGenerator = isGenerator;
387+
388+
@override
389+
void visitBlockFunctionBody(BlockFunctionBody node) {}
390+
391+
@override
392+
void visitReturnStatement(ReturnStatement node) {
393+
if (_isGenerator) {
394+
// Return statements are illegal in generators. Ignore.
395+
return;
396+
}
397+
var type = node.expression?.typeOrThrow;
398+
if (type == null || type.isBottom) {
399+
return;
400+
}
401+
var current = returnType;
402+
if (current == null) {
403+
returnType = type;
404+
} else {
405+
returnType = _typeSystem.leastUpperBound(current, type);
406+
}
407+
}
408+
409+
@override
410+
void visitYieldStatement(YieldStatement node) {
411+
if (!_isGenerator) {
412+
// Yield statements are illegal in non-generators. Ignore.
413+
return;
414+
}
415+
var type = node.expression.typeOrThrow;
416+
if (type.isBottom) {
417+
return;
418+
}
419+
var current = returnType;
420+
if (current == null) {
421+
returnType = type;
422+
} else {
423+
returnType = _typeSystem.leastUpperBound(current, type);
424+
}
425+
}
426+
}
427+
373428
class _DeclarationCollector extends RecursiveAstVisitor<void> {
374429
final String name;
375430
bool isDeclared = false;

‎pkg/analysis_server/lib/src/services/refactoring/legacy/extract_method.dart‎

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import 'package:analyzer/dart/element/element.dart';
2626
import 'package:analyzer/dart/element/element2.dart';
2727
import 'package:analyzer/dart/element/nullability_suffix.dart';
2828
import 'package:analyzer/dart/element/type.dart';
29-
import 'package:analyzer/dart/element/type_system.dart';
3029
import 'package:analyzer/source/source.dart';
3130
import 'package:analyzer/source/source_range.dart';
3231
import 'package:analyzer/src/dart/analysis/session_helper.dart';
@@ -975,10 +974,9 @@ final class ExtractMethodRefactoringImpl extends RefactoringImpl
975974
result.addError(errorExits);
976975
}
977976
}
978-
// maybe ends with "return" statement
977+
// Maybe ends with a "return" statement.
979978
if (selectionStatements != null) {
980-
var typeSystem = _resolveResult.typeSystem;
981-
var returnTypeComputer = _ReturnTypeComputer(typeSystem);
979+
var returnTypeComputer = ReturnTypeComputer(_resolveResult.typeSystem);
982980
for (var statement in selectionStatements) {
983981
statement.accept(returnTypeComputer);
984982
}
@@ -1757,41 +1755,6 @@ class _Occurrence {
17571755
_Occurrence(this.range, this.isSelection);
17581756
}
17591757

1760-
class _ReturnTypeComputer extends RecursiveAstVisitor<void> {
1761-
final TypeSystem typeSystem;
1762-
1763-
DartType? returnType;
1764-
1765-
_ReturnTypeComputer(this.typeSystem);
1766-
1767-
@override
1768-
void visitBlockFunctionBody(BlockFunctionBody node) {}
1769-
1770-
@override
1771-
void visitReturnStatement(ReturnStatement node) {
1772-
// prepare expression
1773-
var expression = node.expression;
1774-
if (expression == null) {
1775-
return;
1776-
}
1777-
// prepare type
1778-
var type = expression.typeOrThrow;
1779-
if (type.isBottom) {
1780-
return;
1781-
}
1782-
// combine types
1783-
returnType = _combine(returnType, type);
1784-
}
1785-
1786-
DartType _combine(DartType? returnType, DartType type) {
1787-
if (returnType == null) {
1788-
return type;
1789-
} else {
1790-
return typeSystem.leastUpperBound(returnType, type);
1791-
}
1792-
}
1793-
}
1794-
17951758
/// Generalized version of some source, in which references to the specific
17961759
/// variables are replaced with pattern variables, with back mapping from the
17971760
/// pattern to the original variable names.

‎pkg/analysis_server/test/src/services/correction/assist/add_return_type_test.dart‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,14 @@ class A {
6161
Future<void> test_method_block_noReturn() async {
6262
await resolveTestCode('''
6363
class A {
64-
/*caret*/m() {
65-
}
64+
/*caret*/m() {}
65+
}
66+
''');
67+
await assertHasAssist('''
68+
class A {
69+
void m() {}
6670
}
6771
''');
68-
await assertNoAssist();
6972
}
7073

7174
Future<void> test_method_block_returnDynamic() async {

‎pkg/analysis_server/test/src/services/correction/fix/add_return_type_test.dart‎

Lines changed: 84 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,14 @@ class A {
7070
Future<void> test_method_block_noReturn() async {
7171
await resolveTestCode('''
7272
class A {
73-
m() {
74-
}
73+
m() {}
74+
}
75+
''');
76+
await assertHasFix('''
77+
class A {
78+
void m() {}
7579
}
7680
''');
77-
await assertNoFix();
7881
}
7982

8083
Future<void> test_method_block_returnDynamic() async {
@@ -183,6 +186,60 @@ f(A a) => a.b();
183186
await assertNoFix();
184187
}
185188

189+
Future<void> test_topLevelFunction_async_hasReturns() async {
190+
await resolveTestCode('''
191+
f() async {
192+
if (1 == 2) {
193+
return 0;
194+
} else {
195+
return 1.5;
196+
}
197+
}
198+
''');
199+
await assertHasFix('''
200+
Future<num> f() async {
201+
if (1 == 2) {
202+
return 0;
203+
} else {
204+
return 1.5;
205+
}
206+
}
207+
''');
208+
}
209+
210+
Future<void> test_topLevelFunction_async_noReturns() async {
211+
await resolveTestCode('''
212+
f() async {}
213+
''');
214+
await assertHasFix('''
215+
Future<void> f() async {}
216+
''');
217+
}
218+
219+
Future<void> test_topLevelFunction_asyncStar_noYield() async {
220+
await resolveTestCode('''
221+
f() async* {}
222+
''');
223+
await assertHasFix('''
224+
Stream<void> f() async* {}
225+
''');
226+
}
227+
228+
Future<void> test_topLevelFunction_asyncStar_withYield() async {
229+
await resolveTestCode('''
230+
f() async* {
231+
yield 0;
232+
yield 1.5;
233+
}
234+
''');
235+
await assertHasFix('''
236+
Stream<num> f() async* {
237+
yield 0;
238+
yield 1.5;
239+
}
240+
''');
241+
}
242+
186243
Future<void> test_topLevelFunction_block() async {
187244
await resolveTestCode('''
188245
f() {
@@ -211,6 +268,30 @@ get foo => 0;
211268
''');
212269
await assertHasFix('''
213270
int get foo => 0;
271+
''');
272+
}
273+
274+
Future<void> test_topLevelFunction_syncStar_noYield() async {
275+
await resolveTestCode('''
276+
f() sync* {}
277+
''');
278+
await assertHasFix('''
279+
Iterable<void> f() sync* {}
280+
''');
281+
}
282+
283+
Future<void> test_topLevelFunction_syncStar_withYield() async {
284+
await resolveTestCode('''
285+
f() sync* {
286+
yield 0;
287+
yield 1.5;
288+
}
289+
''');
290+
await assertHasFix('''
291+
Iterable<num> f() sync* {
292+
yield 0;
293+
yield 1.5;
294+
}
214295
''');
215296
}
216297
}

0 commit comments

Comments
 (0)