Skip to content

Commit

Permalink
Fix various bugs and regularize behavior of returns statements in
Browse files Browse the repository at this point in the history 
various kinds of functions.

Closes-bug: #31887
Closes-bug: #30638
Closes-bug: #32233
Closes-bug: #32881
Closes-bug: #31278
Change-Id: I4ebd7e71096d611e189b571ba5de2998dd11c98b
Reviewed-on: https://dart-review.googlesource.com/60300
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Leaf Petersen <leafp@google.com>
  • Loading branch information
@leafpetersen
leafpetersen authored and commit-bot@chromium.org committed Jun 15, 2018
1 parent f1d1da6 commit 8e9e8e1
Show file tree
Hide file tree
Showing 38 changed files with 870 additions and 93 deletions.
26 changes: 25 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,30 @@

### Language

* Numerous corner case bugs around return statements in synchronous and
asynchronous functions fixed. Specifically:
* Issues [31887][issue 31887], [32881][issue 32881]. Future flattening should
not be recursive.
* Issues [30638][issue 30638], [32233][issue 32233]. Incorrect downcast errors
with `FutureOr`
* Issue [32233][issue 32233]. Errors when returning `FutureOr`
* Issue [33218][issue 33218]. Returns in functions with void related types
* Issue [31278][issue 31278]. Incorrect hint on empty returns in async
functions
* An empty `return;` in an async function with return type `Future<Object>` will
not report an error.
* `return exp;` where `exp` has type `void` in an async function is now an error
unless the return type of the function is `void` or `dynamic`.
* Mixed return statements of the form `return;` and `return exp;` are now
allowed when `exp` has type `void`.

[issue 31887]: https://github.com/dart-lang/sdk/issues/31887
[issue 30638]: https://github.com/dart-lang/sdk/issues/30638
[issue 32233]: https://github.com/dart-lang/sdk/issues/32233
[issue 32881]: https://github.com/dart-lang/sdk/issues/32881
[issue 33218]: https://github.com/dart-lang/sdk/issues/33218
[issue 31278]: https://github.com/dart-lang/sdk/issues/31278

#### Strong Mode

### Dart VM
Expand All @@ -28,7 +52,7 @@

### Language

Inference chooses `void` when combining `Object` or `dynamic` and `void` ([issue
* Inference chooses `void` when combining `Object` or `dynamic` and `void` ([issue
3341]). When combining with other top types, inference now prefers `void`. So
for example, given:

Expand Down
10 changes: 7 additions & 3 deletions pkg/analyzer/lib/src/dart/element/type.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1447,9 +1447,13 @@ class InterfaceTypeImpl extends TypeImpl implements InterfaceType {

@override
DartType flattenFutures(TypeSystem typeSystem) {
// Implement the case: "If T = Future<S> then flatten(T) = flatten(S)."
if (isDartAsyncFuture && typeArguments.isNotEmpty) {
return typeArguments[0].flattenFutures(typeSystem);
// Implement the cases:
// - "If T = FutureOr<S> then flatten(T) = S."
// - "If T = Future<S> then flatten(T) = S."
if (isDartAsyncFutureOr || isDartAsyncFuture) {
return typeArguments.isNotEmpty
? typeArguments[0]
: DynamicTypeImpl.instance;
}

// Implement the case: "Otherwise if T <: Future then let S be a type
Expand Down
163 changes: 126 additions & 37 deletions pkg/analyzer/lib/src/generated/error_verifier.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1618,6 +1618,57 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
}
}

/**
* Check that return statements without expressions are not in a generative
* constructor and the return type is not assignable to `null`; that is, we
* don't have `return;` if the enclosing method has a non-void containing
* return type.
*
* See [CompileTimeErrorCode.RETURN_IN_GENERATIVE_CONSTRUCTOR],
* [StaticWarningCode.RETURN_WITHOUT_VALUE], and
* [StaticTypeWarningCode.RETURN_OF_INVALID_TYPE].
*/
void _checkForAllEmptyReturnStatementErrorCodes(
ReturnStatement statement, DartType expectedReturnType) {
if (_inGenerator) {
return;
}
if (_options.strongMode) {
var returnType = _inAsync
? expectedReturnType.flattenFutures(_typeSystem)
: expectedReturnType;
if (returnType.isDynamic ||
returnType.isDartCoreNull ||
returnType.isVoid) {
return;
}
} else {
// TODO(leafp): Delete this non-strong path
if (_inAsync) {
if (expectedReturnType.isDynamic || expectedReturnType.isVoid) {
return;
}
if (expectedReturnType is InterfaceType &&
expectedReturnType.isDartAsyncFuture) {
DartType futureArgument = expectedReturnType.typeArguments[0];
if (futureArgument.isDynamic ||
futureArgument.isDartCoreNull ||
futureArgument.isVoid ||
futureArgument.isObject) {
return;
}
}
} else if (expectedReturnType.isDynamic || expectedReturnType.isVoid) {
return;
}
}
// If we reach here, this is an invalid return
_hasReturnWithoutValue = true;
_errorReporter.reportErrorForNode(
StaticWarningCode.RETURN_WITHOUT_VALUE, statement);
return;
}

/**
* Verify that the given [constructor] declaration does not violate any of the
* error codes relating to the initialization of fields in the enclosing
Expand Down Expand Up @@ -2307,7 +2358,8 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
*
* Check that return statements without expressions are not in a generative
* constructor and the return type is not assignable to `null`; that is, we
* don't have `return;` if the enclosing method has a return type.
* don't have `return;` if the enclosing method has a non-void containing
* return type.
*
* Check that the return type matches the type of the declared return type in
* the enclosing method or function.
Expand All @@ -2322,6 +2374,7 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
? DynamicTypeImpl.instance
: functionType.returnType;
Expression returnExpression = statement.expression;

// RETURN_IN_GENERATIVE_CONSTRUCTOR
bool isGenerativeConstructor(ExecutableElement element) =>
element is ConstructorElement && !element.isFactory;
Expand All @@ -2336,41 +2389,15 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
}
// RETURN_WITHOUT_VALUE
if (returnExpression == null) {
if (_inGenerator) {
return;
} else if (_inAsync) {
if (expectedReturnType.isDynamic || expectedReturnType.isVoid) {
return;
}
if (expectedReturnType is InterfaceType &&
expectedReturnType.isDartAsyncFuture) {
DartType futureArgument = expectedReturnType.typeArguments[0];
if (futureArgument.isDynamic ||
futureArgument.isDartCoreNull ||
futureArgument.isVoid ||
futureArgument.isObject) {
return;
}
}
} else if (expectedReturnType.isDynamic ||
expectedReturnType.isVoid ||
(expectedReturnType.isDartCoreNull && _options.strongMode)) {
// TODO(leafp): Empty returns shouldn't be allowed for Null in strong
// mode either once we allow void as a type argument. But for now, the
// only type we can validly infer for f.then((_) {print("hello");}) is
// Future<Null>, so we allow this.
return;
}
_hasReturnWithoutValue = true;
_errorReporter.reportErrorForNode(
StaticWarningCode.RETURN_WITHOUT_VALUE, statement);
_checkForAllEmptyReturnStatementErrorCodes(statement, expectedReturnType);
return;
} else if (_inGenerator) {
// RETURN_IN_GENERATOR
_errorReporter.reportErrorForNode(
CompileTimeErrorCode.RETURN_IN_GENERATOR,
statement,
[_inAsync ? "async*" : "sync*"]);
return;
}

_checkForReturnOfInvalidType(returnExpression, expectedReturnType);
Expand Down Expand Up @@ -4837,8 +4864,10 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
if (_hasReturnWithoutValue) {
return;
}
if (_returnsWith.isNotEmpty && _returnsWithout.isNotEmpty) {
for (ReturnStatement returnWith in _returnsWith) {
var nonVoidReturnsWith =
_returnsWith.where((stmt) => !getStaticType(stmt.expression).isVoid);
if (nonVoidReturnsWith.isNotEmpty && _returnsWithout.isNotEmpty) {
for (ReturnStatement returnWith in nonVoidReturnsWith) {
_errorReporter.reportErrorForToken(
StaticWarningCode.MIXED_RETURN_TYPES, returnWith.returnKeyword);
}
Expand Down Expand Up @@ -5647,7 +5676,7 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
* See [StaticTypeWarningCode.RETURN_OF_INVALID_TYPE].
*/
void _checkForReturnOfInvalidType(
Expression returnExpression, DartType expectedReturnType,
Expression returnExpression, DartType expectedType,
{bool isArrowFunction = false}) {
if (_enclosingFunction == null) {
return;
Expand All @@ -5658,19 +5687,79 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
// of the return type. So no need to do any further checking.
return;
}
if (_options.strongMode) {
if (returnExpression == null) {
return; // Empty returns are handled elsewhere
}

DartType expressionType = getStaticType(returnExpression);

void reportTypeError() {
String displayName = _enclosingFunction.displayName;

if (displayName.isEmpty) {
_errorReporter.reportTypeErrorForNode(
StaticTypeWarningCode.RETURN_OF_INVALID_TYPE_FROM_CLOSURE,
returnExpression,
[expressionType, expectedType]);
} else {
_errorReporter.reportTypeErrorForNode(
StaticTypeWarningCode.RETURN_OF_INVALID_TYPE,
returnExpression,
[expressionType, expectedType, displayName]);
}
}

var toType = expectedType;
var fromType = expressionType;

if (isArrowFunction) {
if (_inAsync && toType.flattenFutures(_typeSystem).isVoid) {
return;
} else if (toType.isVoid) {
return;
}
}

if (toType.isDynamic) {
return;
}

if (toType.isVoid) {
if (fromType.isVoid) {
return;
}
if (!_inAsync && fromType.isDynamic ||
fromType.isDartCoreNull ||
fromType.isBottom) {
return;
}
} else if (!fromType.isVoid) {
if (_inAsync) {
fromType = _typeProvider.futureType
.instantiate(<DartType>[fromType.flattenFutures(_typeSystem)]);
}
if (_expressionIsAssignableAtType(returnExpression, fromType, toType)) {
return;
}
}

return reportTypeError();
}
// TODO(leafp): Delete this non Dart 2 path
DartType staticReturnType = _computeReturnTypeForMethod(returnExpression);
String displayName = _enclosingFunction.displayName;

void reportTypeError() => _errorReporter.reportTypeErrorForNode(
StaticTypeWarningCode.RETURN_OF_INVALID_TYPE,
returnExpression,
[staticReturnType, expectedReturnType, displayName]);
[staticReturnType, expectedType, displayName]);
void reportTypeErrorFromClosure() => _errorReporter.reportTypeErrorForNode(
StaticTypeWarningCode.RETURN_OF_INVALID_TYPE_FROM_CLOSURE,
returnExpression,
[staticReturnType, expectedReturnType]);
[staticReturnType, expectedType]);

if (expectedReturnType.isVoid) {
if (expectedType.isVoid) {
if (isArrowFunction) {
// "void f(..) => e" admits all types for "e".
return;
Expand All @@ -5691,10 +5780,10 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {

// TODO(mfairhurst) Make this stricter once codebases are compliant.
final invalidVoidReturn = staticReturnType.isVoid &&
!(expectedReturnType.isVoid || expectedReturnType.isDynamic);
!(expectedType.isVoid || expectedType.isDynamic);
if (!invalidVoidReturn &&
_expressionIsAssignableAtType(
returnExpression, staticReturnType, expectedReturnType)) {
returnExpression, staticReturnType, expectedType)) {
return;
}
if (displayName.isEmpty) {
Expand Down
25 changes: 24 additions & 1 deletion pkg/analyzer/lib/src/generated/resolver.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1186,7 +1186,30 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
if (body.isGenerator) {
return;
}
// Check that the type is resolvable, and is not "void"

if (_typeSystem is StrongTypeSystemImpl) {
// Check that the type is resolvable, and is not "void"
DartType returnTypeType = returnType.type;
if (returnTypeType == null) {
return;
}

var flattenedType = body.isAsynchronous
? returnTypeType.flattenFutures(_typeSystem)
: returnTypeType;
if (flattenedType.isDynamic ||
flattenedType.isDartCoreNull ||
flattenedType.isVoid) {
return;
}
// Check the block for a return statement, if not, create the hint
if (!ExitDetector.exits(body)) {
_errorReporter.reportErrorForNode(HintCode.MISSING_RETURN, returnType,
[returnTypeType.displayName]);
}
}
// TODO(leafp): Delete this non-strong mode path
// Check that the type is resolvable and not "void"
DartType returnTypeType = returnType.type;
if (returnTypeType == null ||
returnTypeType.isVoid ||
Expand Down
6 changes: 1 addition & 5 deletions pkg/analyzer/test/generated/hint_code_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2573,11 +2573,7 @@ import 'dart:async';
FutureOr<void> f(Future f) async {}
''');
await computeAnalysisResult(source);
if (previewDart2) {
assertErrors(source, [HintCode.MISSING_RETURN]);
} else {
assertNoErrors(source);
}
assertNoErrors(source);
verify([source]);
}

Expand Down
8 changes: 0 additions & 8 deletions pkg/analyzer/test/generated/non_error_resolver_kernel_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,6 @@ class NonErrorResolverTest_Kernel extends NonErrorResolverTest_Driver {
return super.test_ambiguousImport_showCombinator();
}

@override
@failingTest
@notForDart2
test_async_return_flattens_futures() async {
// Only FutureOr is flattened.
return super.test_async_return_flattens_futures();
}

@override
@failingTest
@FastaProblem('https://github.com/dart-lang/sdk/issues/31604')
Expand Down
Loading

0 comments on commit 8e9e8e1

Please sign in to comment.