Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707933002:
Tests for `void`.
|
Created:
3 years, 10 months ago by floitsch Modified:
3 years, 5 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionTests for `void`.
Patch Set 1 #Patch Set 2 : Add main. #Patch Set 3 : More tests. #
Total comments: 63
Patch Set 4 : Rebase #Patch Set 5 : Address comments. #
Total comments: 64
Messages
Total messages: 10 (3 generated)
lrn@google.com changed reviewers: + lrn@google.com
https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... File tests/language/generic_function_typedef_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... tests/language/generic_function_typedef_test.dart:8: class A { Isn't used. https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... tests/language/generic_function_typedef_test.dart:11: typedef F<T> = Function<S>(List<S> list, Function<A>(A), T); This defines a generic function that requires a generic function as second argument. https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... tests/language/generic_function_typedef_test.dart:14: foo2(List<int> x, bar(String y), int z) {} Neither of these functions are generic or take a generic function as second argument, so they can't satisfy F<T> for any T (except when we ignore the generic function type and treat it as Function(dynamic)). If you are testing Dart 1 semantics, document that, so nobody thinks this should work in strong mode. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type2_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:1: // Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file No void_type1_test.dart? https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:24: var z = null ?? x; /// param_null_equals2: error Could argue - why allow `?:` but not `??`? Since `e1??e2` is equivalent to `e1 == null ? e2 : null` and the latter is allowed, shouldn't the former be too? https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:26: return x; /// param_return: error Add: while (x) ; do {} while (x); for (var v in x); x += 1; // includes an implicit read of x. x?.toString(); x..toString(); Not sure if: for (x in []); /// <- allowed? fits in this test. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:42: Map<dynamic, dynamic> m3 = { 4: f() }; /// call_literal_map_value_init2: error Long line. Style guide says no spaces after '{' and before '}', so you can save it :) https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:93: void x; Consider a version with "final void x" too. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:94: (true? x: x); /// conditional_parens: error I think we should allow parentheses. It's always surprising when parentheses makes a difference. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:95: true? x: x; /// conditional_stmt: ok Nitpick: Space before ?. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:157: var a = new A<void>(); Use A<void> instead of var, otherwise you don't get the same static type in non-strong mode. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:177: var b = new B(); B b = new B(); https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:27: void /// 00: error That looks wrong. "var void x;" isn't valid syntax. Should it be something like: void /// 00: error /* /// 00: continued var */ /// 00: continued x; ? https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:39: foo() => null; Say what the problem here is. It's overriding an Object function with a void function, right? https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:43: void /// 02: error I don't think this should be an error. It's overriding a "void gee(Object x)" with either "void gee(dynamic/Object x)" or "void get(void x)". Either should be valid overrides. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:68: Object x; /// 04: ok? should be ok, but the setter goes from void to Object. The getter is fine, but the setter isn't. This is a case of overriding a (generically covariant) method in a covariant and non-generic way. I think you have argued that this shouldn't be allowed. The fact that void is involved shouldn't change that. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:75: Object /// 05: error This is effectively the same signature as the setter for x. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:114: A<dynamic> a4 = new A<void>(); /// dynamic_assignment: ok? So, basically, the question is whether dynamic is (treated as) a supertype of void or not? I'd say not - void is "not a value" and dynamic is "any value". https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type5_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type5_test.dart:28: var a = new A<void>(); var a = ... -> A<void> a = ...
eernst@google.com changed reviewers: + eernst@google.com
Several comments on the `void` related tests. (Just one comment on the `Function` type part. That issue should still be considered, somehow, even though we may ignore it for this CL). https://codereview.chromium.org/2707933002/diff/40001/pkg/front_end/test/fast... File pkg/front_end/test/fasta/function_type_recovery_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/pkg/front_end/test/fast... pkg/front_end/test/fasta/function_type_recovery_test.dart:8: typedef F = int Function(int f(String x)); Actually, upon request I specified that the `T Function(..)` syntax should _not_ allow for nested occurrences of old-style `functionSignature`s like `int f(String x)`, cf. https://gist.github.com/eernstg/ffc7bd281974e9018f10f0cb6cfee4aa. So we should not test that this is accepted, we should insist that it is rejected (ideally as a syntax error, and certainly at compile-time). The same issue arises at line 12 below. https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... File tests/language/generic_function_typedef2_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... tests/language/generic_function_typedef2_test.dart:38: Expect.isFalse(b is K); /// 04: continued So this is tested in the case where we have the static type warning (so K means `dynamic Function(dynamic Function<A>(MalformedType))`, and `b` ain't such a thing. But the test here in line 38 would still make sense when we do not include `<int>`, where K simply means `dynamic Function(dynamic Function<A>(A))`, so we could omit `/// 04: continued` here. Another issue is whether `b` really isn't that malformed type: Sect. 19.1 Types does not specify a malformed function type, so apparently function types are never malformed (a type application like `G<..>` is malformed iff `G` is malformed, so the type arguments can go wild). We could decide that function types will erase malformed components to `dynamic`, so the type above is `dynamic Function(dynamic Function<A>(dynamic))`, and we can confidently decide that `b` ain't it. Otherwise the function type itself would be malformed, in which case the type test should raise a run-time error: `Expect.throws .. /// 04:continued`. https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... File tests/language/generic_function_typedef_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/generic_... tests/language/generic_function_typedef_test.dart:14: foo2(List<int> x, bar(String y), int z) {} I believe we haven't yet written any CLs for extending the spec to include the `Function` based function type syntax in Dart 1 (I haven't done that, and I couldn't find a CL doing that), so we would need to decide on the missing pieces. For instance, it seems useful to erase generic function types like `Function<A>(A)` to `Function(dynamic)` and then just go from there, so that would most likely be part of such a specification. So do we just go ahead here and assume some semantics, and then write a spec for it later? We could still adjust the test if we end up wishing to have a slightly different semantics. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type2_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:14: use(x as Object); /// param_as: error? Presumably, a developer could have a situation where an instance method has statically known return type `void` but the developer knows that a subtype overrides that method declaration with one that has some other return type T. In that case it seems reasonable to allow the developer to assert "it's a T, trust me" and perform the cast. The point is that we may actually be able to trust that developer! This applies to explicit downcasts from `void` in general, and I believe that the argument can be made for all the cases, including this one where it is a parameter. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:17: x is Object; /// param_is: error Following the argument for allowing `x as T`, we could say that `x is T` should be allowed as well. But we don't have to, and I'd actually prefer disallowing it (i.e., keeping the treatment the way it is here). The reason is that the developer has a work-around: `(x as Object) is ..`. It would be OK to force developers to use this verbose form, because it is a really confusing and error-prone operation. So we drill just a tiny hole allowing developers to break out of the void-jail, and both properties are intended: There should be a hole, and it should be tiny. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:24: var z = null ?? x; /// param_null_equals2: error I think it's an ugly hack to allow the conditional expression to have subexpressions of type `void`, and we only do it (I think) because developers may wish to use the more compact syntax (even though it misrepresents the essence of `:?` which is that it yields a value). Assuming that we make the choice to live with that, I'd still prefer that we avoid taking it further, so `??` shouldn't have the same privilege. Looking for counter arguments: Do we have any applications of `e ?? myVoidExpression` that serve as a convincing motivation for having this feature? https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:25: var z = x ?? 499; /// param_null_equals2: error Here we _obviously_ want to look at the value of an expression of type `void`. So we should disallow it. (Developers still have the work-around: `(x as int) ?? 499`, if they really want it). https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:26: return x; /// param_return: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Add: > while (x) ; > do {} while (x); > for (var v in x); > x += 1; // includes an implicit read of x. > x?.toString(); > x..toString(); > > Not sure if: > for (x in []); /// <- allowed? > fits in this test. For all those I'd prefer a compile-time error: We just have a single tiny hole allowing developers to look at the value of a `void` expression, they can use the work-around here again, and all these cases contradict the 'voidness' of the value because they inherently inspect that value. `for (x in [..]);` could be allowed when we have `void x` in scope, because we just dump some values into a void variable, which is OK, and there are no attempts to read it again. This also matches up with the case `void x; x = 42;` below (line 51-52). https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:34: use(f() as Object); /// call_as: error? Same argument: Let's allow this. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:46: return f(); /// call_return: error I like this, but doesn't it contradict current treatment? All tools are happy about this: void f() {} void g() { return f(); } main() { g(); } The spec has 'On the other hand, it is possible to return the result of a void method from within a void method.' in Sect. 19.7 Void, though as non-normative text. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:56: use(x as Object); /// local_as: error? I'd propose to allow this, like the others. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:68: return x; /// local_return: error Again, I'd support making this an error, but it should probably be treated the same as `void g() { return f(); }` where `f` has return type `void`. In that case it should be allowed. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:77: use(global as Object); /// global_as: error? Allow, as usual. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:93: void x; On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Consider a version with "final void x" too. That's delightfully useless: Here's a variable where you can write (that's tolerated, though not so valuable) but you can't read (that's the whole point of being `void`), except that you can't write. Do we actually have a use case? `C implements Something`? But it raises the issue about `void get x => someVoidFunction(42);` which would make it possible to express computations by mentioning their name (I guess `x;` would then be ok, and it would run that void function). A non-void function would be OK, too, of course, as long as we are happy to ignore the returned value. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:94: (true? x: x); /// conditional_parens: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > I think we should allow parentheses. It's always surprising when parentheses > makes a difference. I can see that, especially if we have a need for parens inside a conditional expression (`b? aVoidFunction() : (a()..b()..c())`). But the complete expression `(...)` would then have type `void`, which would prevent it from occurring "almost everywhere", with the usual exceptions. Same for the conditional expression: It may be able to contain void expressions, and it may be nested, but we can't put it everywhere because it is itself subject to the "no void here" constraint on all the "normal" expressions. If we do this, parentheses should be OK everywhere, nested, etc. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:106: return true? x: x; /// conditional_return: error Usual consideration: We currently allow `return g()` in a void function when g is void, too. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:108: (true? 499: x); /// conditional2_parens: error With parens allowed, this would be ok. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:120: return true? 499: x; /// conditional2_return: error Same as line 106. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:122: (true? x: 499); /// conditional3_parens: error OK if parens are ok. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:134: return true? x: 499; /// conditional3_return: error Same as line 106. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:163: use(a.x as Object); /// instance_as: error? Same as similar cases: `as` could be the tiny exception that we do allow. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:183: use(b.x as Object); /// instance2_as: error? Like line 163. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:195: return b.x; /// instance2_return: error Like line 106. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:203: use(c.x as Object); /// instance3_as: error? Like line 163. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:68: Object x; /// 04: ok? should be ok, but the setter goes from void to Object. We have the following overriding relationship: A<void>: void get x D: Object get x A<void>: void set x(void value) D: void set x(Object value) The getter just follows the pattern we always had, so that's not allowed to be a problem. For the setter, if we use Dart 1 rules and consider `Object <: void` to hold, this would be a covariantly overridden parameter type, which is OK. So it should be allowed in Dart 1. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:75: Object /// 05: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > This is effectively the same signature as the setter for x. Same issue: If we use Dart 1 overriding rules then covariant parameter types are fine (no `covariant` modifier needed). https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:112: (); Yes, I think we should let the special treatment of downcasts from `void` bleed into the generic type: Any number of levels could be involved before we reach `void`, but it still counts as a "downcast from void" if the target type puts any other type in place of `void`. We could say that "isVoid(A<T>) == isVoid(T) for all A", and then apply the same restrictions on casts from all "isVoid" types rather than just on the type `void` itself. We'd need to say more precisely what it means to "downcast from void", because we should cover cases like `Map<void, void>` --> `Map<void, dynamic>` and `Map<dynamic, void>` --> `Map<void, dynamic>` (both of which will enable certain derived expressions to switch from `void` to some other type). https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:113: A a3 = new A<void>(); /// raw_assignment: ok? In Dart 1 the type annotation `A` is definitely `A<dynamic>`, so this is again a "downcast from void" via isVoid. This assumes that we consider `dynamic` and `Object` to occupy the same location in the type graph, but we've said things like that many times. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:114: A<dynamic> a4 = new A<void>(); /// dynamic_assignment: ok? Agreed, I'd want to make this an error, but the reasoning is that we downcast from an isVoid type to a non-isVoid type, which counts as "downcast from void", which is an error when statically known.
Only addressed the comments. Not yet taking the feedback from the meeting into account. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type2_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:1: // Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file On 2017/02/21 09:37:03, Lasse Reichstein Nielsen wrote: > No void_type1_test.dart? Existed already. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:24: var z = null ?? x; /// param_null_equals2: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Could argue - why allow `?:` but not `??`? Since `e1??e2` is equivalent to `e1 > == null ? e2 : null` and the latter is allowed, shouldn't the former be too? Not sure. There are use-cases, but I'm not sure they are that great: void handleNoMessage() { ... } msg ?? handleNoMessage(); In any case. Changed to not being the initializer value (which is clearly not allowed. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:26: return x; /// param_return: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Add: > while (x) ; > do {} while (x); > for (var v in x); > x += 1; // includes an implicit read of x. > x?.toString(); > x..toString(); > > Not sure if: > for (x in []); /// <- allowed? > fits in this test. Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:42: Map<dynamic, dynamic> m3 = { 4: f() }; /// call_literal_map_value_init2: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Long line. Style guide says no spaces after '{' and before '}', so you can save > it :) Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:93: void x; On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Consider a version with "final void x" too. Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:94: (true? x: x); /// conditional_parens: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > I think we should allow parentheses. It's always surprising when parentheses > makes a difference. Added a paren version. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:95: true? x: x; /// conditional_stmt: ok On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Nitpick: Space before ?. Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:157: var a = new A<void>(); On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Use A<void> instead of var, otherwise you don't get the same static type in > non-strong mode. Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type2_test.dart:177: var b = new B(); On 2017/02/21 09:37:03, Lasse Reichstein Nielsen wrote: > B b = new B(); Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:27: void /// 00: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > That looks wrong. "var void x;" isn't valid syntax. Should it be something like: > > void /// 00: error > /* /// 00: continued > var > */ /// 00: continued > x; > > ? Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:39: foo() => null; On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > Say what the problem here is. > It's overriding an Object function with a void function, right? Done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:43: void /// 02: error On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > I don't think this should be an error. > > It's overriding a "void gee(Object x)" with either "void gee(dynamic/Object x)" > or "void get(void x)". Either should be valid overrides. right. done. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:114: A<dynamic> a4 = new A<void>(); /// dynamic_assignment: ok? On 2017/02/21 13:39:13, eernst wrote: > Agreed, I'd want to make this an error, but the reasoning is that we downcast > from an isVoid type to a non-isVoid type, which counts as "downcast from void", > which is an error when statically known. > What about `dynamic a5 = new A<void>()` then? Added it to the test. https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type5_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type5_test.dart:28: var a = new A<void>(); On 2017/02/21 09:37:04, Lasse Reichstein Nielsen wrote: > var a = ... -> A<void> a = ... Done.
leafp@google.com changed reviewers: + leafp@google.com
Not sure of the strong/non-strong status of these tests, should maybe add language_strong versions? https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... File tests/language/void_type2_test.dart (right): https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:154: (true ? x : x); /// conditional_parens: error Seems odd to me. As far as I can find a unifying theme here, it's that you're not allowed to construct aggregates with void subcomponents, and not assign void things to other things without an explicit cast. Parens don't really feel like either of these. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:213: class B implements A<void> { Consider adding the contra-variant parameter case as well? bar(int x) {} overriden with bar(void x) {} ? https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:219: class C implements A<void> { Is it deliberate that there is an error here (unimplemented foo?) https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:5: // Dart test for type checks involving the void type. Are these strong mode, Dart 1.0, or both? https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:13: Object y; Is it worth also have a dynamic version to make it clear that dynamic is treated the same way as Object in all of these cases? https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:72: Object x; /// 04: ok? should be ok, but the setter goes from void to Object. I think not ok, at least not as we've formulated this. I still feel like we're in a bit of a state of confusion as to whether we want void to be equivalent to Object and dynamic + some lints, or void to be a supertype of Object and dynamic. But current thinking seems to be in favor of the supertype treatment, and I think it's just weird to special case out things like this one. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:115: <void> /// implicit_downcast: error What does this mean? Implicit downcasts aren't errors? https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:117: A a3 = new A<void>(); /// raw_assignment: ok? There should be no difference between this one and the previous one. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:118: A<dynamic> a4 = new A<void>(); /// A_dynamic_assignment: ok? ditto.
https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/40001/tests/language/void_typ... tests/language/void_type4_test.dart:114: A<dynamic> a4 = new A<void>(); /// dynamic_assignment: ok? > What about `dynamic a5 = new A<void>()` then? I want to say "allowed". Here you are not casting void-ness to non-void-ness, you are casting away everything that even refers to the void. Casting a List<void> to Object doesn't allow you to access any "void" members of the list - you don't even know that it is a list, so not only does the void no longer exist in the static type, the things previously typed as void also no longer exists in the static type. There is some structurally recursive property here. A cast is disallowed if: it casts void to a non-void type. It casts T<..., A, ...> to T<..., B, ...> and the cast A->B is disallowed. This includes supertypes, so List<void> -> Iterable<dynamic> is disallowed because List<void> is-a Iterable<void> and you are casting that to Iterable<non-void>.
Have to go now -- remaining comments on void_type4_test.dart will follow tomorrow. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... File tests/language/void_type2_test.dart (right): https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:11: true ? x : x; /// param_conditional: ok We decided at a language meeting long ago that the conditional expression should not be included, which is the reason why I've omitted that construct from the allowed contexts for void expressions in the informal spec. If we want to add it again we should make that decision. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:14: use(x as Object); /// param_as: error? It was my understanding that we had agreed to allow explicit casts out of `void`, but I recently learned that Lasse does not support this. So we need to adjust this to a plain `error`, or we need to adjust the informal spec. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:17: x is Object; /// param_is: error In the informal spec this is allowed, but as I noted in a comment I do not remember having had clear support for that decision in the language team. I can go either way, Lasse prefers to prohibit it. In any case either this should be adjusted, or the informal spec should. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:39: true ? f() : f(); /// call_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:42: use(f() as Object); /// call_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:45: f() is Object; /// call_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:67: true ? x : x; /// local_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:70: use(x as Object); /// local_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:73: x is Object; /// local_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:97: true ? x : x; /// final_local_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:100: use(x as Object); /// final_local_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:103: x is Object; /// final_local_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:116: for (x in [1, 2]) {} /// final_local_for_in2: ok Why would it be OK to assign to a final local, no matter whether it's void? https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:126: true ? global : global; /// global_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:129: use(global as Object); /// global_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:132: global is Object; /// global_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:154: (true ? x : x); /// conditional_parens: error The informal spec allows `(e)` even for void `e`, and I think we agreed on that, but the conditional should be treated like in line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:155: true ? x : x; /// conditional_stmt: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:156: true ? true ? x : x : true ? x : x; /// conditional_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:157: for (true ? x : x; false; true ? x : x) {} /// conditional_for: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:173: true ? true ? 499 : x : true ? 499 : x; /// conditional2_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:174: for (true ? 499 : x; false; true ? 499 : x) {} /// conditional2_for: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:188: (true ? x : 499); /// conditional3_parens: error Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:189: true ? x : 499; /// conditional3_stmt: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:190: true ? true ? x : 499 : true ? x : 499; /// conditional3_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:191: for (true ? x : 499; false; true ? x : 499) {} /// conditional3_for: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:229: true ? a.x : a.x; /// instance_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:232: use(a.x as Object); /// instance_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:235: a.x is Object; /// instance_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:257: true ? b.x : b.x; /// instance2_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:260: use(b.x as Object); /// instance2_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:263: b.x is Object; /// instance2_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:285: true ? c.x : c.x; /// instance3_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:288: use(c.x as Object); /// instance3_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:291: c.x is Object; /// instance3_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:314: true ? (x) : (x); /// paren_conditional: ok Should get the same treatment as line 11. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:317: use((x) as Object); /// paren_as: error? Should get the same treatment as line 14. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type2_test.dart:320: (x) is Object; /// paren_is: error Should get the same treatment as line 17. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... File tests/language/void_type3_test.dart (right): https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type3_test.dart:70: expectsF(g); /// 04: error I cannot see how this could be an error. It would also be surprising if `g is F` returns true, and still the checked-mode check on the argument of `expectsF` fails. The crucial point is probably that `void` as a return type is given special treatment in Dart 1.x (so a `void Function()` is _not_ an `Object Function()`, only vice versa), but `void` as a type annotation and as an actual type argument is not given special treatment: It behaves the same as `Object`. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type3_test.dart:77: expectsF2(g2); /// 06: error Same as line 70. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type3_test.dart:84: expectsF3(g3); /// 08: error Same as line 70. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:5: // Dart test for type checks involving the void type. On 2017/02/23 21:30:52, Leaf wrote: > Are these strong mode, Dart 1.0, or both? In Dart 1.x it is a static warning if m2 overrides m1 and the type of m2 isn't a subtype of the type of m1. So we won't get an error there. But we should get a static warning for the cases where a non-void return type is overridden by a `void` return type (lines 26, 32, 38). For strong mode I would expect `void` to be the same type as `Object` so no issues arise due to subtyping in these cases. With voidness preservation checks in place we should get a voidness preservation violation when a `void` return type overrides a non-void return type. We might introduce voidness preservation rules in several steps: Top-level occurrences of `void` could be handled in an early step (e.g., overriding `Object` as a return type by `void`), and nested occurrences could be handled later (e.g., overriding `A<Object>` as a return type by `A<void>`). Similarly, overriding relations could be handled earlier than data flow relations (assignments, parameter passing). So we could end up treating the return type override `Object` --> `void` as an error in strong mode "soon". But I certainly expected these tests to be for Dart 1.x semantics, because they're located in tests/language. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:26: void /// 00: error This would be a static warning for Dart 1.x. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:32: void /// 00b: error This would be a static warning for Dart 1.x. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:38: void /// 00c: error This would be a static warning for Dart 1.x. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:45: void /// 01: error This would be a static warning for Dart 1.x. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:52: h(A<void> a) {} (Just noting that these overriding declarations are OK because `void` as a parameter type is considered to be `Object` for subtyping, and we have no voidness preservation in place at this point; and because `A<void>` counts as `A<Object>` for overriding.)
Added comments on the remaining test cases. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... File tests/language/void_type4_test.dart (right): https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:72: Object x; /// 04: ok? should be ok, but the setter goes from void to Object. On 2017/02/23 21:30:53, Leaf wrote: > I think not ok, at least not as we've formulated this. I still feel like we're > in a bit of a state of confusion as to whether we want void to be equivalent to > Object and dynamic + some lints, or void to be a supertype of Object and > dynamic. But current thinking seems to be in favor of the supertype treatment, > and I think it's just weird to special case out things like this one. The informal spec of generalized void is built on the following foundation: `void` is the same type as `Object`, except (1) expressions of type void cannot occur in certain locations, (2: in the future) voidness preservation, and (3) function types with `void` as the return type preserve their special placement in the subtype lattice. For Dart 2 we will drop (3), which is the only case where `void` is considered to be a proper supertype of `Object`. Actually, I'd prefer to say that the special subtype rule for `void Function(...)` is just an explicitly specified exception; it is not helpful to actually make `void` a proper supertype of `Object`, because that in turn will require many new exceptions to "undo" that placement of `void` (such that `void` and `Object` are considered to be the same type everywhere else). https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:79: Object /// 05: error We will only get an error here if we introduce a notion of voidness preservation for overriding. (In that case the supertype `gee` receives an argument of type `void` which implies that "the argument is unused", but D.gee grabs the right to use that argument anyway). So this is not an error by the current informal spec, but we could add some simple voidness preservation rules for overriding and make it an error. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:84: A<Object> /// 06: error This parameter type was declared as `A<void>` in `A<T>`, so it's an override `A<void>` --> `A<Object>` in a contravariant position. We will only get an error here with some kind of voidness preservation for overriding, and it must be the "full" kind that takes `void` into account also when it is not the top-level type (that is, when `void` occurs in the actual type arguments of a generic type or in the argument types or return type of a function type). I can't see how it would make sense to add that kind of voidness preservation checking for overrides without also adding full voidness preservation checking for data flow (assignments, parameter passing, etc.). This should then not be an error. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:87: A<Object> /// 07: error This parameter type was declared as `A<T>` in `A<T>`, so it's an override `A<void>` --> `A<Object>` in a contravariant position: Same as line 84, should not be an error. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:115: <void> /// implicit_downcast: error This can only be an error with full support for voidness preservation, so it should not be an error at this point. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:117: A a3 = new A<void>(); /// raw_assignment: ok? Since we don't have voidness preservation at this point this is not an error in any case (considered as `A<dynamic> a3 = new A<Object>();`). I made the distinction between "strict" and "normal" voidness preservation rulesets: The strict ones would disallow assignments where `void` derived expressions were retyped as `dynamic` (e.g., `new A<void>().foo()` is `void` but `a3.foo()` is `dynamic`), but I always considered the strict systems to be for discussion only because everybody else seemed to think that this transition would obviously be OK. In that case it is not an error, even with full voidness preservation: It's `ok` now and in the future. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:118: A<dynamic> a4 = new A<void>(); /// A_dynamic_assignment: ok? Same as line 117. https://codereview.chromium.org/2707933002/diff/80001/tests/language/void_typ... tests/language/void_type4_test.dart:119: dynamic a5 = new A<void>(); /// dynamic_assignment: ok? Since we don't have voidness preservation at this point, this is not an error in any case. However, this would never be a voidness preservation violation, no matter which one of the voidness preservation rulesets we'd use. The point is that it is an upcast that eliminates the class `A` from the type of the target object (and all supertypes where its type argument is propagated, which is the empty set), so we've "forgotten the voidness provided by the type argument to `A`". It is true that `a5.foo()` is an untyped invocation of type `dynamic`, and `new A<void>().foo()` has type `void`, but we cannot consider that to be a voidness preservation violation. If we had considered it to be a violation then all data flows (assignments, parameter passings, returns, etc) from a type containing void to `dynamic` would be violations, which is even more strict than the "strict rulesets" I mentioned above. So this should be OK now and in the future. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
