Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737933002:
Add tests for generic methods in strong mode (Closed)
|
Created:
3 years, 9 months ago by Dmitry Stefantsov Modified:
3 years, 9 months ago CC:
reviews_dartlang.org, Leaf Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd tests for generic methods in strong mode
The tests cover some basic cases of generic methods usage, so some tests
for corner cases may be added later. The tests are put into
'tests/language_strong' directory, and they are currently skipped by the
test system as described in the file 'language_strong.status'.
R=asgerf@google.com, eernst@google.com, floitsch@google.com, leafp@google.com
Committed: https://github.com/dart-lang/sdk/commit/e2c823a33939b340c9a3469bcb1921e54b8cb883
Patch Set 1 #
Total comments: 82
Patch Set 2 : Fixes according to the received comments #Messages
Total messages: 15 (3 generated)
dmitryas@google.com changed reviewers: + asgerf@google.com, eernst@google.com, floitsch@google.com, karlklose@google.com, kmillikin@google.com
Hi everybody, In this CL I described some examples of generic methods usage, as discussed with Florian and Erik. I also received some valuable comments from Asger and Karl. I'd like to receive an LGTM from the language team before committing, because the tests here describe some parts of the language. Also, it would be great to receive comments from Asger and Karl if I missed some important functionality for strong mode, and need to add some more tests. Additionally, I put Leaf to CC list. Any remarks are welcome! As for the language_strong.status file, I just marked the new tests as Skip. In reality, 14 of them run into Runtime Error, and 2 of them are currently passing (but may be broken in future). If you have a better idea about the status file, please let me know.
leafp@google.com changed reviewers: + leafp@google.com
Thanks for writing these up! A few comments, otherwise lgtm. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_boundaries_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:21: main() { I don't know what the conventions are for these tests, but I find it super useful when reading tests if they include a short comment describing what they are testing, e.g. /* Test that a dynamic call to a generic function checks the type argument against its bound */ https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); I think this one should be true. The reified type of `f` is `Object -> int` under the current proposal, if I recall correctly, which is a subtype of `Object -> Object`. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:33: Expect.isTrue(g is! Object2Object); As above. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_named_parameters_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_named_parameters_test.dart:20: Expect.isTrue(funObjObj is FunObjObj); Any reason not to also assert here that funObjObj is FunXY for all X and Y? https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_optional_parameters_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_optional_parameters_test.dart:20: Expect.isTrue(funObjObj is FunObjObj); As in the previous test, maybe check funObjObj against all of the types? https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_overloading_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:21: return super.fun<B>(x as B); Cast is unnecessary in strong mode. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_unused_parameter_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_unused_parameter_test.dart:39: Fun varFunReq = funTypReq; I think the LHS types here are wrong. Presumably should be Fun, FunReq, ....?
A few comments: https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_boundaries_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:18: int fun<T extends A>(T t) => t.answer(); Remove answer and replace it with some integer. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:21: main() { On 2017/03/09 05:51:18, Leaf wrote: > I don't know what the conventions are for these tests, but I find it super > useful when reading tests if they include a short comment describing what they > are testing, e.g. > > /* Test that a dynamic call to a generic function checks the type argument > against its bound */ Agreed, please add a short comment in the beginning of the file and/or individual lines. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_closure_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:11: void bubbleSort<T extends Comparable<T>>(List<T> list) { Could you make this test a bit simpler? I don't think we actually need to do a sort here (and below). https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:20: Expect.isTrue(a is! S); // fails if S is substituted with dynamic Please remove references to substitution with `dynamic` where it is no longer relevant (for strong mode) (also below). https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_dynamic_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_dynamic_test.dart:21: Use multi-tests? c.fun<A>(b); /// 01: compile-time error https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_dynamic_test.dart:22: Expect.throws(() => obj.fun<A>(b), (e) => e is TypeError); () => obj.fun<A>(b); /// 02: runtime error https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); On 2017/03/09 05:51:19, Leaf wrote: > I think this one should be true. The reified type of `f` is `Object -> int` > under the current proposal, if I recall correctly, which is a subtype of `Object > -> Object`. Why is it not `int -> int`? https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_function_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_function_test.dart:11: void bubbleSort<T extends Comparable<T>>(List<T> list) { Maybe you can merge this test with 'tests/language_strong/generic_methods_closure_test.dart'. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_variable_declaration_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:14: @override Consider leaving out '@override'.
https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); On 2017/03/09 09:17:00, karlklose wrote: > On 2017/03/09 05:51:19, Leaf wrote: > > I think this one should be true. The reified type of `f` is `Object -> int` > > under the current proposal, if I recall correctly, which is a subtype of > `Object > > -> Object`. > > Why is `f` not `int -> int`? Or do you mean that 'int -> int' is a subtype of 'Object -> Object'?
I think these tests look great! LGTM https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_overloading_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:21: return super.fun<B>(x as B); On 2017/03/09 05:51:19, Leaf wrote: > Cast is unnecessary in strong mode. It seems to me that 'B' is not more specific than 'T'. How does type promotion work here? https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/langu... File tests/language_strong/language_strong.status (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/langu... tests/language_strong/language_strong.status:280: generic_methods_unused_parameter_test: Skip This section is for tests that are not yet strong-mode clean, i.e. tests that are wrong in strong mode and need to be updated. The status lines relevant for dartk are in language_strong_kernel.status, and these line should be moved there.
LGTM. just a few small nits. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_closure_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:19: A a = new A(); // A does not extend Comparable<A>, so A != S Finish with "." https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:20: Expect.isTrue(a is! S); // fails if S is substituted with dynamic Start comment with uppercase. Finish it with ".". https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:36: Expect.isTrue(list[0] == 1); Expect.listEquals([1, 2, 3, 4, 5], list); https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); On 2017/03/09 09:17:00, karlklose wrote: > On 2017/03/09 05:51:19, Leaf wrote: > > I think this one should be true. The reified type of `f` is `Object -> int` > > under the current proposal, if I recall correctly, which is a subtype of > `Object > > -> Object`. > > Why is it not `int -> int`? Because of covariant generics. We have to support: A<Object> tt = x; Object Function(Object) f = tt.fun; We don't want to do a dynamic check every time a method is torn off, which means that we will dynamically make the tear-off be `int Function(Object)`. The function will dynamically make sure that the given argument has the correct type. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_function_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_function_test.dart:36: Expect.isTrue(list[0] == 1); Expect.listEquals https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_variable_declaration_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:14: @override On 2017/03/09 09:17:00, karlklose wrote: > Consider leaving out '@override'. Also note that not writing any type here would infer the type from super. You could just write `generate() {`. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:38: Expect.isTrue(anotherName is! T); // fails if T is substituted with dynamic Start comments with uppercase and finish them with ".". https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_recursive_bound_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_recursive_bound_test.dart:45: Expect.isTrue(list[0].x == 1); Expect.listEquals https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_recursive_bound_test.dart:53: // int does not extend Ordered<int> Try to start with upper case, and finish with ".". https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_shadowing_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_shadowing_test.dart:33: Expect.isTrue(t is S); // S is passed as T below, so T = S here Finish with ".".
LGTM. The most crucial issue is probably to clarify whether we do in fact enforce that generic methods have no variance on their type parameter bounds (so, for instance, `int Function<T extends num>(T)` and `int Function<T extends Object>(T)` have no subtype relation to each other, and cannot override each other. Another issue is whether we want tests in language_strong to specifically check whether they are being run in Dart 1 (non-strong) mode, and fail. I do not think this is very useful, so I recommended that the statements which serve no other purpose (as far as I can see) should be deleted. That said, it is of course important to check that type arguments are properly reified. But this can be checked in ways that are much more precise (such that we do not just get a test failure when the type argument is erased to `dynamic`, we get the failure if the type argument is wrong in many other ways as well). There are tests where reification is checked. E.g., in generic_methods_dynamic_test.dart the actual type argument `A` must be passed at runtime such that the instance method `fun` can compute the actual argument type `A` and detect that the actual value argument doesn't have that type. But it might be useful to double check that reification is actually checked reasonably precisely, also in the the simple and static cases. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_boundaries_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:23: Expect.throws(() => obj.fun<B>(new B()), (e) => e is TypeError); We give type argument B which is a violation of the bound, and since malbounded instance creation (`new SomeClass<WrongTypeArguments>()`) is a compile-time error, I would expect malbounded method calls to be a compile-time error if detectable, and dynamic cases (like this one) to be run-time errors (though https://github.com/dart-archive/dev_compiler/blob/master/doc/STATIC_SAFETY.md doesn't make this explicit). We should never reach the body of `fun`, so it does not matter that the B actually has an `answer` and that the invocation in `fun` statically implies that we are calling `A.answer` or an overriding implementation thereof. I think it would be useful to adjust the test to focus on the malbounded invocation and then cover both the static and the dynamic case. And I agree that it would be better if the test stated this purpose explicitly. Then there could be other test statements focusing on passing a `B` to an invocation of `fun<A>` (checking that the given type argument is used when checking actual arguments, statically and dynamically), and possible more. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_closure_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:20: Expect.isTrue(a is! S); // fails if S is substituted with dynamic I'm not sure about the purpose of this check: It could be a trap which forces Dart 1 (non-strong mode) executions to fail, but I don't see why we would have a strong mode test which is (among other things) designed specifically to fail when run in non-strong mode. It's usually an organizational issue that we keep this file in a certain (language_strong) directory, and it doesn't matter whether any of those tests are guaranteed to fail in Dart 1, they just need to test strong mode behavior correctly. We would have a separate (and possibly quite different) test for Dart 1. If this is intended to confirm that the class A does not satisfy `A extends Comparable<A>` then A needs to be given as a type argument where the check would be performed statically (for instance creation and statically known generic function calls) or dynamically (only for dynamic function invocations passing actual type arguments). In this position it will just test that A is not a subtype of the particular S which was passed in the given invocation, i.e., it would still fail even in the case where `A extends Comparable<A>`. So the test does not make much sense in strong mode. I'd recommend that lines 19-20 are removed. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_dynamic_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_dynamic_test.dart:22: Expect.throws(() => obj.fun<A>(b), (e) => e is TypeError); This tests one of the cases I mentioned above in generic_methods_boundaries_test.dart (that the actual type argument is taken into account when checking arguments to a dynamic invocation of a generic function), so we might as well do it here and drop it up there. It would seem natural to have the positive dynamic case as well: `fun<B>(b)` should succeed and return b. And a case where the type argument is used in a non-trivial manner: `List<T> fun2<T>(Iterable<T> t) => <T>[t.first];`, such that the dynamic check must discover that the type argument is B and the value argument must then be an Iterable<B>. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:23: A z = new A(); This one depends on instantiate-to-bound for a raw type, which is currently dynamic (we discussed Object, but I'll assume we stick to dynamic). In that case `h` will have reified type `dynamic Function(Object)`. I think it would be useful to mention these reified types here (as a comment on f, g, and h), such that it is easier to see the intentions behind each case below, if the test breaks at some point. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_variable_declaration_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:38: Expect.isTrue(anotherName is! T); // fails if T is substituted with dynamic This is again a check that seems to be justified solely because it will force Dart 1 executions to fail, so it should be treated like other checks of the same kind (possibly removing it). https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_named_parameters_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_named_parameters_test.dart:23: Expect.isTrue(funTypObj is! FunObjObj); Similarly here and below: Might as well have is/is! for all the function types. In particular, funTypObj is a FunTypTyp but not a FunObjTyp, so the missing cases are not just "all the same" or otherwise trivial. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_optional_parameters_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_optional_parameters_test.dart:23: Expect.isTrue(funTypObj is! FunObjObj); Same comment as in the previous test from me, too. ;-) https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_overloading_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:5: library generic_methods_boundaries_test; Might as well use a library name that matches the file name, like the other tests, and the file name might as well avoid the word 'overloading' (which is usually associated with 'static', and Dart does not have static overloading in the usual sense --- multiple members in the same scope with the same name, but different types). Maybe 'generic_methods_overriding_test.dart'? https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:19: String fun<T extends A>(T x) { I believe we have maintained that generic function subtyping does not allow for differences in the bounds on type arguments. I would be surprised if this shouldn't apply to overriding as well. In that case this is a compile time error, the bound on T must be B. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:29: String fun<T>(B x) => "E"; This would again be a compile time error, there must be a bound on T and it must be B. (But using `B` as the parameter type is no problem: we always know that T <: B, so this is just contravariance). https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_shadowing_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_shadowing_test.dart:5: library generic_methods_shadowing; Not sure what makes this test 'shadowing'. A few words about the topic of this test would again be nice. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_unused_parameter_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_unused_parameter_test.dart:38: Fun varFun = funTyp; Agreeing with Leaf, I'll assume that you meant to use the types Fun (in line 38), FunReq (line 39), etc. If so, this is all about obtaining an instantiation of the generic functions (`varFun` refers to `funTyp<dynamic>`, etc). A comment explaining this in a few words would be good for readability and maintenance.
https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); On 2017/03/09 11:27:17, floitsch wrote: > On 2017/03/09 09:17:00, karlklose wrote: > > On 2017/03/09 05:51:19, Leaf wrote: > > > I think this one should be true. The reified type of `f` is `Object -> int` > > > under the current proposal, if I recall correctly, which is a subtype of > > `Object > > > -> Object`. > > > > Why is it not `int -> int`? > > Because of covariant generics. > We have to support: > A<Object> tt = x; > Object Function(Object) f = tt.fun; > > We don't want to do a dynamic check every time a method is torn off, which means > that we will dynamically make the tear-off be `int Function(Object)`. The > function will dynamically make sure that the given argument has the correct > type. Is the writeup of that ready to share yet? https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_overloading_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:19: String fun<T extends A>(T x) { On 2017/03/09 14:55:03, eernst wrote: > I believe we have maintained that generic function subtyping does not allow for > differences in the bounds on type arguments. I would be surprised if this > shouldn't apply to overriding as well. > > In that case this is a compile time error, the bound on T must be B. I had forgotten we'd agreed to this, but it seems like we did. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:21: return super.fun<B>(x as B); On 2017/03/09 10:36:51, asgerf wrote: > On 2017/03/09 05:51:19, Leaf wrote: > > Cast is unnecessary in strong mode. > > It seems to me that 'B' is not more specific than 'T'. > > How does type promotion work here? Currently in strong mode, if we have a type variable `T` with bound `A`, and a variable `x : T extends A`, then `x is B` where `B <: A`, promotes `x` to `x : T extends B`. It's a little weird, since it associates bounds with uses of type variables instead of just with the declaration, but the analyzer does that already, and it makes some examples like this work out better.
https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_overloading_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:21: return super.fun<B>(x as B); On 2017/03/09 19:17:26, Leaf wrote: > On 2017/03/09 10:36:51, asgerf wrote: > > On 2017/03/09 05:51:19, Leaf wrote: > > > Cast is unnecessary in strong mode. > > > > It seems to me that 'B' is not more specific than 'T'. > > > > How does type promotion work here? > > Currently in strong mode, if we have a type variable `T` with bound `A`, and a > variable `x : T extends A`, then `x is B` where `B <: A`, promotes `x` to `x : T > extends B`. > > It's a little weird, since it associates bounds with uses of type variables > instead of just with the declaration, but the analyzer does that already, and it > makes some examples like this work out better. > Ah, I remember seeing that when adding strong mode to dartk, but I thought it was just a quirk in the analyzer. I have some more questions about this, but I'll start an email thread instead of discussing it here. I still think it makes sense to keep the downcast to make the test independent of type promotion.
Still LGTM, just added a clarifying remark. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); On 2017/03/09 19:17:26, Leaf wrote: > On 2017/03/09 11:27:17, floitsch wrote: > > On 2017/03/09 09:17:00, karlklose wrote: > > > On 2017/03/09 05:51:19, Leaf wrote: > > > > I think this one should be true. The reified type of `f` is `Object -> > int` > > > > under the current proposal, if I recall correctly, which is a subtype of > > > `Object > > > > -> Object`. > > > > > > Why is it not `int -> int`? > > > > Because of covariant generics. > > We have to support: > > A<Object> tt = x; > > Object Function(Object) f = tt.fun; > > > > We don't want to do a dynamic check every time a method is torn off, which > means > > that we will dynamically make the tear-off be `int Function(Object)`. The > > function will dynamically make sure that the given argument has the correct > > type. > > Is the writeup of that ready to share yet? We did that already: https://github.com/dart-lang/sdk/blob/master/docs/language/informal/covariant...
https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_boundaries_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:18: int fun<T extends A>(T t) => t.answer(); On 2017/03/09 09:16:59, karlklose wrote: > Remove answer and replace it with some integer. Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:21: main() { On 2017/03/09 09:16:59, karlklose wrote: > On 2017/03/09 05:51:18, Leaf wrote: > > I don't know what the conventions are for these tests, but I find it super > > useful when reading tests if they include a short comment describing what they > > are testing, e.g. > > > > /* Test that a dynamic call to a generic function checks the type argument > > against its bound */ > > Agreed, please add a short comment in the beginning of the file and/or > individual lines. Yes, that's a good idea. I've added comments that describe the purpose of each test. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_boundaries_test.dart:23: Expect.throws(() => obj.fun<B>(new B()), (e) => e is TypeError); On 2017/03/09 14:55:02, eernst wrote: > We give type argument B which is a violation of the bound, and since malbounded > instance creation (`new SomeClass<WrongTypeArguments>()`) is a compile-time > error, I would expect malbounded method calls to be a compile-time error if > detectable, and dynamic cases (like this one) to be run-time errors (though > https://github.com/dart-archive/dev_compiler/blob/master/doc/STATIC_SAFETY.md > doesn't make this explicit). > > We should never reach the body of `fun`, so it does not matter that the B > actually has an `answer` and that the invocation in `fun` statically implies > that we are calling `A.answer` or an overriding implementation thereof. > Yes, I agree. I removed the 'answer' methods, because they don't add anything to the test itself and is merely an artifact of the test writing history. > I think it would be useful to adjust the test to focus on the malbounded > invocation and then cover both the static and the dynamic case. And I agree that > it would be better if the test stated this purpose explicitly. > Done. And agree about the comments. > Then there could be other test statements focusing on passing a `B` to an > invocation of `fun<A>` (checking that the given type argument is used when > checking actual arguments, statically and dynamically), and possible more. Yes, agree on the usefulness of such tests. There are some examples of such checks in other files. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_closure_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:11: void bubbleSort<T extends Comparable<T>>(List<T> list) { On 2017/03/09 09:17:00, karlklose wrote: > Could you make this test a bit simpler? I don't think we actually need to do a > sort here (and below). Yep, makes sense. Originally I was thinking about how some situations may happen in real life, and the simplest case I could think of was sorting. But I guess it's better to reduce these examples to something more abstract, so that the checks are more evident for future readers. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:19: A a = new A(); // A does not extend Comparable<A>, so A != S On 2017/03/09 11:27:17, floitsch wrote: > Finish with "." Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:20: Expect.isTrue(a is! S); // fails if S is substituted with dynamic On 2017/03/09 09:17:00, karlklose wrote: > Please remove references to substitution with `dynamic` where it is no longer > relevant (for strong mode) (also below). Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:20: Expect.isTrue(a is! S); // fails if S is substituted with dynamic On 2017/03/09 11:27:17, floitsch wrote: > Start comment with uppercase. Finish it with ".". Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:20: Expect.isTrue(a is! S); // fails if S is substituted with dynamic On 2017/03/09 14:55:02, eernst wrote: > I'm not sure about the purpose of this check: It could be a trap which forces > Dart 1 (non-strong mode) executions to fail, but I don't see why we would have a > strong mode test which is (among other things) designed specifically to fail > when run in non-strong mode. It's usually an organizational issue that we keep > this file in a certain (language_strong) directory, and it doesn't matter > whether any of those tests are guaranteed to fail in Dart 1, they just need to > test strong mode behavior correctly. We would have a separate (and possibly > quite different) test for Dart 1. > > If this is intended to confirm that the class A does not satisfy `A extends > Comparable<A>` then A needs to be given as a type argument where the check would > be performed statically (for instance creation and statically known generic > function calls) or dynamically (only for dynamic function invocations passing > actual type arguments). In this position it will just test that A is not a > subtype of the particular S which was passed in the given invocation, i.e., it > would still fail even in the case where `A extends Comparable<A>`. So the test > does not make much sense in strong mode. > > I'd recommend that lines 19-20 are removed. Yes, that's Dart 1 specific, so I think I should remove these lines. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_closure_test.dart:36: Expect.isTrue(list[0] == 1); On 2017/03/09 11:27:17, floitsch wrote: > Expect.listEquals([1, 2, 3, 4, 5], list); Thanks! I should have done that. Anyway, after the simplification suggested by Karl, these lines are gone. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_dynamic_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_dynamic_test.dart:21: On 2017/03/09 09:17:00, karlklose wrote: > Use multi-tests? > > c.fun<A>(b); /// 01: compile-time error Thanks! I put the checks for both compile-type error and runtime error then. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_dynamic_test.dart:22: Expect.throws(() => obj.fun<A>(b), (e) => e is TypeError); On 2017/03/09 09:17:00, karlklose wrote: > () => obj.fun<A>(b); /// 02: runtime error Probably, it should be 'obj.fun<A>(b);', otherwise it's just an expression of a closure type that isn't called anywhere and doesn't produce any runtime errors. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_dynamic_test.dart:22: Expect.throws(() => obj.fun<A>(b), (e) => e is TypeError); On 2017/03/09 14:55:02, eernst wrote: > This tests one of the cases I mentioned above in > generic_methods_boundaries_test.dart (that the actual type argument is taken > into account when checking arguments to a dynamic invocation of a generic > function), so we might as well do it here and drop it up there. > Done. > It would seem natural to have the positive dynamic case as well: `fun<B>(b)` > should succeed and return b. > Agreed and done. > And a case where the type argument is used in a non-trivial manner: `List<T> > fun2<T>(Iterable<T> t) => <T>[t.first];`, such that the dynamic check must > discover that the type argument is B and the value argument must then be an > Iterable<B>. Agreed and done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_generic_class_tearoff_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:23: A z = new A(); On 2017/03/09 14:55:02, eernst wrote: > This one depends on instantiate-to-bound for a raw type, which is currently > dynamic (we discussed Object, but I'll assume we stick to dynamic). > > In that case `h` will have reified type `dynamic Function(Object)`. > > I think it would be useful to mention these reified types here (as a comment on > f, g, and h), such that it is easier to see the intentions behind each case > below, if the test breaks at some point. Thank you! Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:28: Expect.isTrue(f is! Object2Object); On 2017/03/09 11:27:17, floitsch wrote: > On 2017/03/09 09:17:00, karlklose wrote: > > On 2017/03/09 05:51:19, Leaf wrote: > > > I think this one should be true. The reified type of `f` is `Object -> int` > > > under the current proposal, if I recall correctly, which is a subtype of > > `Object > > > -> Object`. > > > > Why is it not `int -> int`? > > Because of covariant generics. > We have to support: > A<Object> tt = x; > Object Function(Object) f = tt.fun; > > We don't want to do a dynamic check every time a method is torn off, which means > that we will dynamically make the tear-off be `int Function(Object)`. The > function will dynamically make sure that the given argument has the correct > type. Thank you! That explains a lot. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_generic_class_tearoff_test.dart:33: Expect.isTrue(g is! Object2Object); On 2017/03/09 05:51:18, Leaf wrote: > As above. Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_function_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_function_test.dart:11: void bubbleSort<T extends Comparable<T>>(List<T> list) { On 2017/03/09 09:17:00, karlklose wrote: > Maybe you can merge this test with > 'tests/language_strong/generic_methods_closure_test.dart'. Yes. This makes sense. Moved these checks to the closure test. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_function_test.dart:36: Expect.isTrue(list[0] == 1); On 2017/03/09 11:27:17, floitsch wrote: > Expect.listEquals Good idea. As explained above, these tests are abstracted out, so we don't need this check for correctness of sorting anymore. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_local_variable_declaration_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:14: @override On 2017/03/09 09:17:00, karlklose wrote: > Consider leaving out '@override'. Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:14: @override On 2017/03/09 11:27:17, floitsch wrote: > On 2017/03/09 09:17:00, karlklose wrote: > > Consider leaving out '@override'. > > Also note that not writing any type here would infer the type from super. > You could just write `generate() {`. Thanks! https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:38: Expect.isTrue(anotherName is! T); // fails if T is substituted with dynamic On 2017/03/09 11:27:17, floitsch wrote: > Start comments with uppercase and finish them with ".". Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_local_variable_declaration_test.dart:38: Expect.isTrue(anotherName is! T); // fails if T is substituted with dynamic On 2017/03/09 14:55:02, eernst wrote: > This is again a check that seems to be justified solely because it will force > Dart 1 executions to fail, so it should be treated like other checks of the same > kind (possibly removing it). That's right. It's Dart 1 specific, so I'm removing it. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_named_parameters_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_named_parameters_test.dart:20: Expect.isTrue(funObjObj is FunObjObj); On 2017/03/09 05:51:19, Leaf wrote: > Any reason not to also assert here that > > funObjObj is FunXY for all X and Y? None at all :) Thanks! Also added checks for other functions, so that now there's a check for each function against each of the types. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_named_parameters_test.dart:23: Expect.isTrue(funTypObj is! FunObjObj); On 2017/03/09 14:55:02, eernst wrote: > Similarly here and below: Might as well have is/is! for all the function types. > In particular, funTypObj is a FunTypTyp but not a FunObjTyp, so the missing > cases are not just "all the same" or otherwise trivial. Agreed and done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_optional_parameters_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_optional_parameters_test.dart:20: Expect.isTrue(funObjObj is FunObjObj); On 2017/03/09 05:51:19, Leaf wrote: > As in the previous test, maybe check funObjObj against all of the types? Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_optional_parameters_test.dart:23: Expect.isTrue(funTypObj is! FunObjObj); On 2017/03/09 14:55:02, eernst wrote: > Same comment as in the previous test from me, too. ;-) Agree here as well :) Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_overloading_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:5: library generic_methods_boundaries_test; On 2017/03/09 14:55:03, eernst wrote: > Might as well use a library name that matches the file name, like the other > tests, and the file name might as well avoid the word 'overloading' (which is > usually associated with 'static', and Dart does not have static overloading in > the usual sense --- multiple members in the same scope with the same name, but > different types). > > Maybe 'generic_methods_overriding_test.dart'? Yep, the library name is wrong here. Thanks for the 'overloading -> overriding' suggestion. Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:19: String fun<T extends A>(T x) { On 2017/03/09 19:17:27, Leaf wrote: > On 2017/03/09 14:55:03, eernst wrote: > > I believe we have maintained that generic function subtyping does not allow > for > > differences in the bounds on type arguments. I would be surprised if this > > shouldn't apply to overriding as well. > > > > In that case this is a compile time error, the bound on T must be B. > > I had forgotten we'd agreed to this, but it seems like we did. Then I change it to multi-test to check for compile-time error in case of '<T extends A>' and for normal behavior in case of '<T extends B>'. Thanks! Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:21: return super.fun<B>(x as B); On 2017/03/10 09:20:24, asgerf wrote: > On 2017/03/09 19:17:26, Leaf wrote: > > On 2017/03/09 10:36:51, asgerf wrote: > > > On 2017/03/09 05:51:19, Leaf wrote: > > > > Cast is unnecessary in strong mode. > > > > > > It seems to me that 'B' is not more specific than 'T'. > > > > > > How does type promotion work here? > > > > Currently in strong mode, if we have a type variable `T` with bound `A`, and a > > variable `x : T extends A`, then `x is B` where `B <: A`, promotes `x` to `x : > T > > extends B`. > > > > It's a little weird, since it associates bounds with uses of type variables > > instead of just with the declaration, but the analyzer does that already, and > it > > makes some examples like this work out better. > > > > Ah, I remember seeing that when adding strong mode to dartk, but I thought it > was just a quirk in the analyzer. > > I have some more questions about this, but I'll start an email thread instead of > discussing it here. > > I still think it makes sense to keep the downcast to make the test independent > of type promotion. I think I reorganize this, so that we have both versions (with and without the downcast) in the multi-test. Thanks! Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_overloading_test.dart:29: String fun<T>(B x) => "E"; On 2017/03/09 14:55:03, eernst wrote: > This would again be a compile time error, there must be a bound on T and it must > be B. > > (But using `B` as the parameter type is no problem: we always know that T <: B, > so this is just contravariance). Marked it as a compile-time error in multi-test. Also implemented an 'ok' version 'String fun<T extends B>(B b)'. Thanks! Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_recursive_bound_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_recursive_bound_test.dart:45: Expect.isTrue(list[0].x == 1); On 2017/03/09 11:27:17, floitsch wrote: > Expect.listEquals Yes. My comments from above apply here as well. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_recursive_bound_test.dart:53: // int does not extend Ordered<int> On 2017/03/09 11:27:17, floitsch wrote: > Try to start with upper case, and finish with ".". Done. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_shadowing_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_shadowing_test.dart:5: library generic_methods_shadowing; On 2017/03/09 14:55:03, eernst wrote: > Not sure what makes this test 'shadowing'. A few words about the topic of this > test would again be nice. Agree, I didn't include the comments the first time, unfortunately. I fixed it now. Also, I rewrote the test, so that it's more compact and demonstrates the intent clearly. The idea was to define a generic function inside another generic function that has a type parameter with the same name. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_shadowing_test.dart:33: Expect.isTrue(t is S); // S is passed as T below, so T = S here On 2017/03/09 11:27:17, floitsch wrote: > Finish with ".". I rewrote the test, so that it's more compact. And I watched my comments this time :) https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... File tests/language_strong/generic_methods_unused_parameter_test.dart (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_unused_parameter_test.dart:38: Fun varFun = funTyp; On 2017/03/09 14:55:03, eernst wrote: > Agreeing with Leaf, I'll assume that you meant to use the types Fun (in line > 38), FunReq (line 39), etc. > > If so, this is all about obtaining an instantiation of the generic functions > (`varFun` refers to `funTyp<dynamic>`, etc). > > A comment explaining this in a few words would be good for readability and > maintenance. Yes, I put the wrong types in lines 39-43 here. Also agree about comments. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/gener... tests/language_strong/generic_methods_unused_parameter_test.dart:39: Fun varFunReq = funTypReq; On 2017/03/09 05:51:19, Leaf wrote: > I think the LHS types here are wrong. Presumably should be Fun, FunReq, ....? Yes, exactly. https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/langu... File tests/language_strong/language_strong.status (right): https://codereview.chromium.org/2737933002/diff/1/tests/language_strong/langu... tests/language_strong/language_strong.status:280: generic_methods_unused_parameter_test: Skip On 2017/03/09 10:36:51, asgerf wrote: > This section is for tests that are not yet strong-mode clean, i.e. tests that > are wrong in strong mode and need to be updated. > > The status lines relevant for dartk are in language_strong_kernel.status, and > these line should be moved there. Now I see. Thanks for the suggestion! I've moved the lines there, including the new ones that we now have because of multi-tests. Also, I put there the outcomes that the tests generate currently.
Description was changed from ========== Add tests for generic methods in strong mode The tests cover some basic cases of generic methods usage, so some tests for corner cases may be added later. The tests are put into 'tests/language_strong' directory, and they are currently skipped by the test system as described in the file 'language_strong.status'. ========== to ========== Add tests for generic methods in strong mode The tests cover some basic cases of generic methods usage, so some tests for corner cases may be added later. The tests are put into 'tests/language_strong' directory, and they are currently skipped by the test system as described in the file 'language_strong.status'. R=asgerf@google.com, eernst@google.com, floitsch@google.com, leafp@google.com Committed: https://github.com/dart-lang/sdk/commit/e2c823a33939b340c9a3469bcb1921e54b8cb883 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as e2c823a33939b340c9a3469bcb1921e54b8cb883 (presubmit successful). |
