Remove dynamic invocation #205
Conversation
Since the matchers are using FeatureMatcher, the typedMatches method receives a typed object instead of dynamic
|
LGTM, can you bump this to 0.12.15-dev and add a changelog entry? You can leave the message blank, since its not a user facing change. |
Fix more cases, and ignore the intentional dynamic calls. Add changelog
|
Ah sorry, I missed your previous message. Looks like the changelog entry + bump has been done already |
| @@ -14,7 +14,9 @@ class _Empty extends Matcher { | |||
| const _Empty(); | |||
|
|
|||
| @override | |||
| bool matches(Object? item, Map matchState) => (item as dynamic).isEmpty; | |||
| bool matches(Object? item, Map matchState) => | |||
| // ignore: avoid_dynamic_calls | |||
There was a problem hiding this comment.
It's too bad we don't have a better way to suppress these than the ignore. I filed dart-lang/linter#4806
There was a problem hiding this comment.
Could this be replaced with if (item is Iterable) return item.isEmpty; else if (item is String) return item.isEmpty;?
That would technically be breaking if someone defined an unknown object with an isEmpty getter. But the fact that it currently works is surprising to me. I thought the implementation relied on pattern match, not dynamic invocation
There was a problem hiding this comment.
I think supporting an isEmpty property on an arbitrary user defined class was an intentional feature and I'm not eager to try to remove it. It's not a design we'd carry forward to checks.
lib/src/expect/throws_matcher.dart
Outdated
| @@ -73,7 +73,7 @@ class Throws extends AsyncMatcher { | |||
| // function. | |||
| @override | |||
| dynamic /*FutureOr<String>*/ matchAsync(Object? item) { | |||
| if (item is! Function && item is! Future) { | |||
| if (item is! Function() && item is! Future) { | |||
There was a problem hiding this comment.
This stopped accepting values which are a Function<T>(). I think we may need to keep this as a dynamic call to handle generic methods. This is why the Future.value tear-off case is failing.
cc @jakemac53 - I have completely overlooked the fact that functions are not subtypes if they have different numbers of generic type arguments... I wonder if there are any other places we have been more strict than we should and are unintentionally blocking generic functions used as callbacks.
The refactoring started rejecting generic functions, a `Function<T>()` is not a subtype of a `Function()`.
lib/src/core_matchers.dart
Outdated
| const _ReturnsNormally(); | ||
|
|
||
| @override | ||
| bool typedMatches(Function f, Map matchState) { | ||
| bool typedMatches(Function() f, Map matchState) { |
There was a problem hiding this comment.
This is likely a behavior change that we don't have tests for...
|
Closing as the dart-lang/matcher repository is merged into the dart-lang/test monorepo. Please re-open this PR there! |
Add argument types to `typedMatches` arguments which were unnecessarily widening it back to `dynamic`. Add some missing `as dynamic` and `as Function` which is the pattern the lint uses to allow intentional dynamic calls. Ignore some dynamic calls in a test for a legacy API. Ignore a couple false positives for calling methods (as opposed to getters) on a cast expression. The false positive was fixed in https://dart-review.googlesource.com/c/sdk/+/390640 but not yet published. Replaces dart-archive/matcher#205
Since the matchers are using FeatureMatcher, the typedMatches method receives a typed object instead of dynamic