Skip to content

Conversation

@sgrekhov
Copy link
Contributor

Description updated and also new tests added

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests do need to be adjusted a bit more.

The reason is that the @assertion is about generic methods, but none of the methods m are generic, and it is also about the need to adjust the bounds of the tear-off (because we have to replace Xj in the type parameter bounds by its actual value in the run-time type of the receiver).

@sgrekhov
Copy link
Contributor Author

I had to reread the specification. Really a type parameters of the method were meant. Thank you! Updated. PTAL.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

We should, however, make sure that the dynamic type of the function objects are as expected, too (this seems like the right place to check that, and it seems likely that no other test will check the exact same thing).

This can actually be tested very directly because <X extends T1>() {} is void Function<AnyNameYouWant extends T2>() requires T1 <: T2 as well as T2 <: T1. So for once we can just test using is and get an equality-like test.

@sgrekhov
Copy link
Contributor Author

Dynamic is test added. And code formatted via dart format. PTAL.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have a couple of "empty" tests: Language/Expressions/Property_Extraction/Instance_Method_Closurization/method_closurization_positional_params_t01.dart and Language/Expressions/Property_Extraction/Instance_Method_Closurization/method_closurization_positional_params_t02.dart.

The problem is that the description is talking about the treatment of combinations of type parameters of the method and type parameters of the enclosing class, but the method and the class are both non-generic, so nothing in the @description is actually being tested, as far as I can see.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(One more comment)

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I understand it better now: the tests _t01 are testing the non-generic case, which is fine, and we do also have cases where the two kinds of type parameter lists are present.

But it would be helpful to have an extra sentence in the @description indicating that _t01 is testing the non-generic case.

Otherwise it's really confusing to understand how the actual test can serve to test anything which is similar to the description.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving the last thread of review comments.

So all is good now, if the @description is made slightly more informative!

@sgrekhov
Copy link
Contributor Author

Descriptions updated. PTAL

@sgrekhov sgrekhov requested a review from eernstg January 28, 2025 15:54
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and thanks!

@eernstg eernstg merged commit bc52d30 into dart-lang:master Jan 29, 2025
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 31, 2025
2025-01-29 sgrekhov22@gmail.com dart-lang/co19#3054. Add covariant parameters tests (dart-lang/co19#3059)
2025-01-29 sgrekhov22@gmail.com dart-lang/co19#3054. Existing tests renamed to Instance Method Closurization (dart-lang/co19#3058)
2025-01-27 sgrekhov22@gmail.com dart-lang/co19#2162. Add static member and non-redirecting factory constructor conflict test (dart-lang/co19#3056)
2025-01-27 sgrekhov22@gmail.com dart-lang/co19#2145. Update existing "Evaluation of Implicit Variable Getters" tests (dart-lang/co19#3052)
2025-01-24 sgrekhov22@gmail.com dart-lang/co19#2138. Add more tests for `call` method/getter (dart-lang/co19#3053)
2025-01-24 sgrekhov22@gmail.com dart-lang/co19#2162. Add more constructor vs. static member conflict tests (dart-lang/co19#3055)
2025-01-17 sgrekhov22@gmail.com dart-lang/co19#2976. Add more tests for nullable and FutureOr types (dart-lang/co19#3050)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: Ibeafe23be5d8b913db4e83b23f3820cafc3802ac
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/406940
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants