-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update return rules for null-safety? #914
Comments
I'm not sure I see any reason to change any of these. I do think we should clean up the errors/warnings around return statements as part of this release, since they're in an odd state. We specified them as warnings to avoid breakage, but then some or all of them got implemented as errors in the analyzer, so currently the analyzer and CFE differ. We should take this opportunity to make all of them consistent. |
The point is that null-safety introduces an anomaly: // In a legacy library.
int f1() => 42 as dynamic; // OK.
Future<int> f2() async => 42 as dynamic; // OK.
// In a library where null-safety is enabled.
int f3() => 42 as dynamic; // OK.
Future<int> f4() async => 42 as dynamic; // Error. The intuition is that we always "return" However, we have specified the But note that we are never performing an assignment from an expression of type So we're performing a cast from We might just as well have used the following (arguably more natural) specification:
The only reason why we did not do this was that we did not have a definition of the relevant concept when We have the notion of the 'element type of a generator function', and we'd need a corresponding concept of something like the 'element type of an asynchronous non-generator function'. With return type So I'd recommend that we define this new concept, and make that change in I closed #913 (a PR with two changes), because the first change was subsumed by existing rules about return statements in the language specification, and the second change was intended to specify that certain errors (here) are indeed errors — but at this point I'd prefer that we change the specification such that they are not errors after all. |
I agree in principle that a return in an async function should work like any other The context for |
[Edit: Never mind - user error. The below does fail at runtime.]
I don't think you really mean that do you? If so then this program (which currently works) becomes an error: Future<int> foo() async => (Future<num>.value(3) as dynamic);
void main() async {
print(await foo());
} |
As I said at the high level - I'm not averse to cleaning up the errors and warnings around returns. The spec was added after the fact to try to unify the de facto rules implemented by the two front ends into something consistent and minimally breaking - hence all of the odd exceptions around If someone wants to take this on (@eernstg?) then what needs to happen is:
|
It's never simple with Assume we have an async function with declared return type R, and let T be flatten(R). (We can call this the "element type" of the async function, or something). First of all: The simplest solution to specify, is to disallow returning Assuming we want to keep returning futures, the typing is tricky. Let's have a return statement of the form
That can be written as something like: return let tmp = e in tmp is Future<T> ? await tmp : (tmp implicit-downcast-to T);`. Statically typing that to disallow cases where
That will disallow The static return expression types allowed by this rule, for the element type T, are:
That's the same as saying:
That's almost the same as S being assignable to The static return expression types allowed by the rule, for the element type T, are:
These are the types which are either subtypes of T or are So, let's look at some cases: import "dart:async";
var f1 = Future<int>.value(0);
var f2 = Future<Future<int>>.value(f1);
var f3 = Future<Future<Future<int>>>.value(f2);
var fo1 = Future<dynamic>.value(2);
Future<int> foo(int x) async {
if (x == 0) return x;
if (x == 1) return x as dynamic;
if (x == 2) return x as FutureOr<int>;
if (x == 3) return x as FutureOr<dynamic>;
if (x == 10) return f1;
if (x == 11) return f1 as dynamic;
if (x == 12) return f1 as Future<dynamic>; // Disallowed by 1.
if (x == 13) return f1 as FutureOr<int>;
if (x == 14) return f1 as FutureOr<dynamic>; // Disallowed by 1.
if (x == 15) return f1 as FutureOr<FutureOr<int>>; // Disallowed by 1 and 2.
if (x == 16) return f1 as FutureOr<FutureOr<dynamic>>; // Disallowed by 1 and 2.
if (x == 20) return fo1; // Disallowed by 1. Throws at runtime.
if (x == 21) return fo1 as dynamic;
if (x == 22) return fo1 as FutureOr<dynamic>; // Disallowed by 1.
return throw "not";
}
Future<Future<int>> bar(int y) async {
if (y == 0) return f1; // Valid. Disallowed by analyzer.
if (y == 1) return f1 as Future<dynamic>; // Disallowed by 1.
if (y == 2) return f1 as FutureOr<int>; // Disallowed by 1 and 2. Allowed by CFE.
if (y == 3) return f1 as FutureOr<dynamic>; // Disallowed by 1.
if (y == 10) return f2;
if (y == 11) return f2 as dynamic;
if (y == 12) return f2 as Future<dynamic>; // Disallowed by 1.
if (y == 13) return f2 as FutureOr<dynamic>; // Disallowed by 1.
if (y == 14) return f2 as Future<Future<dynamic>>; // Disallowed by 1 and 2.
if (y == 15) return f2 as FutureOr<Future<dynamic>>; // Disallowed by 1 and 2
if (y == 16) return f2 as Future<FutureOr<dynamic>>; // Disallowed by 1 and 2.
if (y == 17) return f2 as FutureOr<FutureOr<dynamic>>; // Disallowed by 1 and 2.
return throw "not";
}
Future<FutureOr<int>> baz(int z) async {
if (z == 0) return z;
if (z == 1) return z as dynamic;
if (z == 2) return z as FutureOr<int>;
if (z == 3) return z as FutureOr<FutureOr<int>>;
if (z == 10) return f1;
if (z == 11) return f1 as dynamic;
if (z == 12) return f1 as Future<dynamic>; // Disallowed by 1.
if (z == 13) return f1 as FutureOr<int>;
if (z == 14) return f1 as FutureOr<dynamic>; // Disallowed by 1.
if (z == 15) return f1 as Future<FutureOr<int>>;
if (z == 16) return f1 as FutureOr<Future<int>>;
if (z == 17) return f1 as FutureOr<FutureOr<int>>;
if (z == 18) return f1 as Future<FutureOr<dynamic>>; // Disallowed by 1 and 2.
if (z == 19) return f1 as FutureOr<Future<dynamic>>; // Disallowed by 1, 2 and Analyzer.
if (z == 20) return f1 as FutureOr<FutureOr<dynamic>>; // Disallowed by 1 and 2.
if (z == 30) return f2; // Disallowed by CFE
if (z == 31) return f2 as dynamic;
if (z == 32) return f2 as Future<dynamic>; // Disallowed by 1.
if (z == 33) return f2 as Future<Future<int>>; // Disallowed by CFE
if (z == 34) return f2 as Future<Future<dynamic>>; // Disallowed by 1 and Analyzer
if (z == 35) return f2 as FutureOr<dynamic>; // Disallowed by 1.
if (z == 36) return f2 as FutureOr<Future<int>>;
if (z == 37) return f2 as Future<FutureOr<int>>;
if (z == 38) return f2 as FutureOr<FutureOr<int>>;
if (z == 39) return f2 as FutureOr<Future<dynamic>>; // Disallowed by 1, 2, and Analyzer
if (z == 40) return f2 as Future<FutureOr<dynamic>>; // Disallowed by 1 and 2.
if (z == 41) return f2 as FutureOr<FutureOr<dynamic>>; // Disallowed by 1 and 2.
if (z == 50) return f3; // Disallowed by Analyzer
return throw "not";
} Currently the cases without specific comments are accepted by both CFE and analyzer (checked in DartPad). The disallowed by 1/2 are referring to the rules above in a setting with no implicit downcasts except from dynamic (assignable-to means either subtype-of or |
Nice analysis, @lrhn! It matches my perspective on the situation in PR #930, where I've chosen the approach that you call model 2, and it could obviously be changed to model 1 by editing a couple of words. I introduced the notion of "the future value type" of an Model 2 does allow for an extra case compared to model 1: we can return an expression of type We can also return an expression So I'd argue that with model 2 we should only await a So the dynamic semantics should continue to check for a Conversely, model 1 introduces the conceptual oddity that the object obtained directly by evaluation of the returned expression is treated differently from an object which is awaited: For the former we allow static type The whole point of this issue is that this is new, and it is a breaking change. Also, the above oddity indicates to me that this breaking change is poorly justified conceptually. So that's the reason why I chose model 2 when I wrote PR #930. Note that PR #930 fixes an issue with the existing spec text, and is otherwise a no-op (no changes), as long as we consider pre-null-safety Dart; but it prepares the spec for null-safety because it avoids introducing the above mentioned breaking change "by accident" when null-safety is introduced. |
Good point that flatten(R) is not sufficient to find the future value type. The flatten function matches what
We actually do not. With the currently specified dynamic behavior, we evaluate the expression with static type We act like that because we:
So, for the case: Future<Future<int>> foo(Future<dynamic> f) async {
return f;
} we are in a situation where If The oddity that the return behaves differently from an We could change the run-time behavior to be more optimistic about futures.
This is the behavior which matches Model 2. We statically accept I don't think we should ever generate a type error for a future which matches the static type, and which is not awaited. We should just reject those statically. So, with the current specified run-time behavior, I'd prefer model 1. |
That is, if you're assuming that we make the static changes and do not follow up with any changes in the semantics. They would have to be non-breaking, of course, but allowing an extra case to succeed is non-breaking. However, there is an underlying conflict: As long as the semantics contains non-trivial behavioral choices based on the dynamic types of objects, it will be difficult to align the dynamic behavior with the static analysis. I believe that's the core issue here. As a clarifying detour, let's consider a design where the choice to await a future is made at compile-time. We would then have two kinds of Static await rulesWith an
With these rules, the presence of an await operation is determined statically, which is a useful and clarifying position to consider. Also, they are obviously sound. We preserve the property that the dynamic choice to await does not rely on any special powers granted to However, the static choice to await can take Extended static await rulesTo avoid the breakage implied by the static await rules, we'd need to ensure that we can also await a future which is the result of evaluating a returned expression of type
This is obviously still sound, it just allows some additional executions to succeed when Comparison with today's semanticsHere's the current dynamic semantics again, where
Let 'xsa' stand for the extended static await rules.
So the extended static await rules are a candidate for the dynamic semantics that we could consider: It is non-breaking (with the extremely marginal exception that a
The novelty here is simply that we support returning expressions of type
Makes sense. But I believe that the extended static await rules are a useful design point to consider, which would fit well with model 2 ("S is assignable to T or flatten(S) is assignable to T"), and it's less weird than the current approach. ;-) |
I read over this. I don't have any strong feelings here. As I understand it, the motivation for re-examining this was the asymmetry from sync to async, in which given a return type of |
I wasn't looking at that specific comparison, and The point I'm making is that the (arbitrary) choice to specify a rule about |
I agree that the accidental breaking change should be fixed. The question is where the accidental undesired breaking change ends and the deliberate "no implicit downcast" breaking change begins. Previously we allowed you to return an expression with static type Do we consider that part of the accidental breaking change, or part of the intentional "no implicit downcast" breaking change? If accidental, it should be fixed, and if intentional, it should not. I consider it intentional. You only get dynamic behavior for expressions with actual static type For return, it's not that simple. We actually have to look at the run-time type of a future before deciding whether to await it or not. Assume that the return statement expression has static type
Based on the simple rule that you should await a future when the result is guaranteed to be useful, and you must not await a future when the result is not guaranteed to be useful (and assuming that the static rules will prevent cases where awaiting might be useful, and not awaiting will not). The static type of the expression and the static return type are not enough to decide whether to await. The run-time type of the value is also not enough, it is the interaction between the declared return type and the run-time value type that decides whether awaiting is correct. So while we are here, we should also make sure that whatever we change the rules to is precisely what we want. I do think the run-time semantics are fine: For a return type of The "Model 1" matches this behavior, so I think that's what we should use. (Although I would highly prefer to just never await implicitly, and always require an explicit await, but that might be too breaking, and too late in the NNBD-flow to add it now). |
@lrhn wrote:
We may await a future—an expression of type
Well, that is the accident! ;-) Even with For the dynamic semantics we do have two reasonable approaches:
I think the second makes more sense: We use the static type The only difference is that the current semantics will throw in some cases where the extended static await rules will allow the computation to succeed. Of course, it is no surprise that there are some cases where both dynamic semantics will end in a dynamic error. So it is true that there will be some cases where a future is awaited, and the obtained object is rejected by a dynamic type check. But the alternative is not that we could have had a successful execution, allowing us to conclude that it was better to avoid awaiting anything; the alternative is that the program throws immediately when the future dynamically fails to give a static guarantee that the awaited object will work. I just don't think that's going to make anybody more happy.
Why would we need to fix anything? The previous specification wording as well as all proposals will ensure that the But maybe it's supposed to be the future value type which is
It would be awaited with all the proposals, because the future value type is
Same situation, awaited in all proposals, no problem.
Agreed: In an async function with return type
Again ensured by all proposals, no problem. So there are no problems relative to these expected properties, no matter which dynamic semantics we'd adopt. Also, in every case where we await, the static type of the returned expression is always a subtype of To summarize: I believe that we can land #930 (which specifies model 2, because that's the one that isn't a breaking change, pre-null-safety). We may or may not wish to change it to model 1 when we introduce null-safety. This will be a breaking change, but null-safety breaks lots of things. However, I still think that model 2 is more consistent, and pragmatically more useful, and if we do keep model 2 for code with null-safety then we may wish to use the 'extended static await' semantics, in order to make the presence/absence of await more predictable, and in order to allow a few more program executions to succeed where they would otherwise incur a dynamic error. |
Would this be less confusing for users and our implementations if |
I'm pretty sure that's a yes. ;-) |
I wrote a comment on the spec CL, so I'll just restate it here for completeness: I like rules that are effectively statically deciding whether to await (as far as possible). Consider "generalized static await" rules (to give them a name), where the rules are in priority order, so read an "otherwise" before all but the first.
The type we are checking the The "S is I think the rules can be written shorter as:
with some of the tests and casts being redundant in some cases. So I propose those rules for the Null Safe semantics of async return. They're static, and feel more orthogonal to me than the "extended static await" rules. They only do implicit down-casts when there is a dynamic involved, and only on the value that is actually statically typed as dynamic. The breaking case is, as usual: Future<Future<int>> foo() async {
Future<dynamic> x = Future<int>.value(42);
return x;
} because the |
I like it! ;-) |
Closing: New rules landed as #941, #948, tests landed as https://dart-review.googlesource.com/c/sdk/+/145587. |
Cf. dart-lang/sdk#41324.
We may wish to revisit the rules about
return e;
because the rules in the language specification mentionNull
specifically in one case, and may need to treatNever
specifically in other cases).For instance,
Future<Never> f() async => null as dynamic;
is a compile-time error with the current rules, with null-safety. But this is becauseFuture<dynamic>
is no longer assignable toFuture<Never>
(so the wrapping withFuture
gives the new assignability a distinction that the old assignability did not have). In contrast,Never f1() => null as dynamic;
is OK (with null-safety as well as without) becausedynamic
is assignable toNever
.In both cases we could argue that an expression of type
dynamic
can be trusted to throw or loop (that is, to not complete, with or without a value) just as well as it could be trusted to return an object of an arbitrary type which is required by the context, or to some type that has a specific member with a specific signature. And we do trust thedynamic
expression for all those purposes.So it seems consistent to allow
return e;
whene
has typedynamic
, both in a synchronous non-generator function and in an asynchronous non-generator function.If we do this then we'd need to adjust the language specification to avoid the wrapping in
Future<...>
. This change should be a no-op semantically for now (same meaning), but it should ensure that the rules are still specifying the intended behavior when we introduce null-safety into the language specification.@munificent, @lrhn, @leafpetersen, WDYT?
The text was updated successfully, but these errors were encountered: