-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dynamic invocation #205
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely a behavior change that we don't have tests for...
Since the matchers are using FeatureMatcher, the typedMatches method receives a typed object instead of dynamic