Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable avoid_dynamic_calls lint #2410

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Enable avoid_dynamic_calls lint #2410

merged 13 commits into from
Oct 29, 2024

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Oct 21, 2024 •
edited
Loading

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.

Remove the deprecated lint package_api_docs.

Replaces dart-archive/matcher#205

@natebosch
Enable avoid_dynamic_calls lint
7223169 
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
@natebosch natebosch requested a review from a team as a code owner October 21, 2024 17:05
@natebosch natebosch requested a review from jakemac53 October 21, 2024 17:05
@github-actions GitHub Actions
Copy link

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

kevmoo
kevmoo reviewed Oct 22, 2024
View reviewed changes
pkgs/matcher/lib/src/core_matchers.dart Outdated
@@ -148,9 +148,9 @@ class _ReturnsNormally extends FeatureMatcher<Function> {
const _ReturnsNormally();

@override
bool typedMatches(Function f, Map matchState) {
bool typedMatches(Object f, Map matchState) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this better? Better to ignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Either pattern is ugly.

We'd either need to ignore an unnecessary_cast or the avoid_dynamic_calls if we use an ignore.

Do we think it would be better behavior in the analyzer to suppress unnecessary_cast in these cases?
If we think the analyzer would make that change adding an // ignore: unnecessary_cast would be a good choice for now.

Copy link
Member Author

@natebosch natebosch Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

dart-lang/sdk#56939

I'll ignore the unnecessary_cast for now and if the discussion on that issue suggest differently we can rethink which pattern to use here.

jakemac53
jakemac53 approved these changes Oct 23, 2024
View reviewed changes
pkgs/matcher/lib/src/core_matchers.dart
@@ -150,7 +150,8 @@ class _ReturnsNormally extends FeatureMatcher<Function> {
@override
bool typedMatches(Function f, Map matchState) {
try {
f();
// ignore: unnecessary_cast
(f as Function)();
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do this?

Suggested change
(f as Function)();
(f as void Function())();

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not, because it works today to do expect(<T>(){}, returnsNormally);, but a <T>() -> void is not a subtype of () -> void

pkgs/matcher/lib/src/map_matchers.dart Outdated
@@ -15,6 +15,7 @@ class _ContainsValue extends Matcher {

@override
bool matches(Object? item, Map matchState) =>
// ignore: avoid_dynamic_calls
(item as dynamic).containsValue(_value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious why we can't cast this to a Map?

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have been worried about non-map types with operator [] and containsKey or containsValue, but thinking about it now that seems like an unlikely use case. I'll change this and containsValue to be FeatureMatcher<Map>

@natebosch natebosch merged commit 42495c2 into master Oct 29, 2024
61 checks passed
@natebosch natebosch deleted the matcher-dynamic branch October 29, 2024 18:53
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 31, 2024
@devoncarew
[deps] rev bazel_worker, benchmark_harness, browser_launcher, cli_uti…
ae0510d 
…l, csslib, ecosystem, glob, html, http, http_multi_server, json_rpc_2, markdown, mockito, pub_semver, source_span, sse, stack_trace, string_scanner, term_glyph, test, test_descriptor, test_process, vector_math, web_socket_channel, yaml

Revisions updated by `dart tools/rev_sdk_deps.dart`.

bazel_worker (https://github.com/dart-lang/bazel_worker/compare/aa3cc9e..7d36032):
  7d36032  2024-10-29  Moritz  Update README.md before archiving (dart-archive/bazel_worker#97)
  ffdecd3  2024-10-29  Kevin Moore  blast repo changes: drop-lint (dart-archive/bazel_worker#96)

benchmark_harness (https://github.com/dart-lang/benchmark_harness/compare/44f125a..ccdb583):
  ccdb583  2024-10-31  Moritz  Update README.md before archiving (dart-archive/benchmark_harness#112)

browser_launcher (https://github.com/dart-lang/browser_launcher/compare/e5fc5d4..f9b48d2):
  f9b48d2  2024-10-28  Moritz  Update README.md before archiving (dart-archive/browser_launcher#66)

cli_util (https://github.com/dart-lang/cli_util/compare/c36b394..6a61e53):
  6a61e53  2024-10-31  Moritz  Update README.md before archiving (dart-archive/cli_util#108)

csslib (https://github.com/dart-lang/csslib/compare/a3700b0..7f5f84e):
  7f5f84e  2024-10-31  Moritz  Update README.md before archiving (dart-archive/csslib#208)

ecosystem (https://github.com/dart-lang/ecosystem/compare/66ddc4f..5099841):
  5099841  2024-10-31  Moritz  Update instructions for trebuchet (dart-lang/ecosystem#306)
  8300570  2024-10-28  Devon Carew  fix the labeller config file (dart-lang/ecosystem#316)
  5a1f899  2024-10-28  Kevin Moore  new tweak: drop lint (dart-lang/ecosystem#315)
  50dd246  2024-10-28  Kevin Moore  trebuchet: drop unneeded lints dep (dart-lang/ecosystem#314)

glob (https://github.com/dart-lang/glob/compare/00a9c82..3e414a2):
  3e414a2  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/glob#98)

html (https://github.com/dart-lang/html/compare/6d3bc86..929b6de):
  929b6de  2024-10-31  Moritz  Update README.md (dart-archive/html#258)

http (https://github.com/dart-lang/http/compare/f5ec11c..8db0d0a):
  8db0d0a  2024-10-30  Kevin Moore  Drop outdated lint (dart-lang/http#1395)
  71d5212  2024-10-28  Brian Quinlan  Prepare to publish cupertino_http 2.0.0 (dart-lang/http#1390)
  99475f2  2024-10-28  Brian Quinlan  Switch to http_image_provider 1.0 (dart-lang/http#1391)

http_multi_server (https://github.com/dart-lang/http_multi_server/compare/e7515b5..a9e71fa):
  a9e71fa  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/http_multi_server#73)

json_rpc_2 (https://github.com/dart-lang/json_rpc_2/compare/c9b616b..cd94cef):
  cd94cef  2024-10-28  Moritz  Update README.md before archiving (dart-archive/json_rpc_2#119)

markdown (https://github.com/dart-lang/markdown/compare/207bb44..a4b1d2c):
  a4b1d2c  2024-10-30  Mack Myers  Adds export for link_reference_definition_syntax.dart (dart-lang/markdown#626)
  24412ee  2024-10-29  Kevin Moore  blast_repo fixes (dart-lang/markdown#627)

mockito (https://github.com/dart-lang/mockito/compare/57d484f..f72791d):
  f72791d  2024-10-31  Bob Nystrom  In my previous change, I tried a simpler approach in custom_mocks_test to try to not be sensitive to formatting changes before I realized that wouldn't work in auto_mocks_test and came up with something more robust.
  0d582b7  2024-10-30  Bob Nystrom  In custom_mocks_test, there were only a handful that were sensitive to formatting, so I just loosened the tests a bit manually.

pub_semver (https://github.com/dart-lang/pub_semver/compare/8cce9d0..72317ea):
  72317ea  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/pub_semver#108)

source_span (https://github.com/dart-lang/source_span/compare/ec100b7..e518512):
  e518512  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/source_span#118)

sse (https://github.com/dart-lang/sse/compare/af2c5c5..1b02011):
  1b02011  2024-10-28  dependabot[bot]  Bump actions/checkout from 4.1.7 to 4.2.0 in the github-actions group (dart-lang/sse#116)
  e4ab625  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/sse#117)

stack_trace (https://github.com/dart-lang/stack_trace/compare/115bcd9..582891c):
  582891c  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/stack_trace#164)

string_scanner (https://github.com/dart-lang/string_scanner/compare/084b201..4de83f0):
  4de83f0  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/string_scanner#83)

term_glyph (https://github.com/dart-lang/term_glyph/compare/19d8c08..d7d8d7c):
  d7d8d7c  2024-10-29  Kevin Moore  blast_repo fixes (dart-lang/term_glyph#57)

test (https://github.com/dart-lang/test/compare/fe38650..42495c2):
  42495c26  2024-10-29  Nate Bosch  Enable avoid_dynamic_calls lint (dart-lang/test#2410)
  9a267def  2024-10-29  Nate Bosch  Remove some unused testing utilities (dart-lang/test#2413)

test_descriptor (https://github.com/dart-lang/test_descriptor/compare/a3db1ef..aa99e99):
  aa99e99  2024-10-29  Kevin Moore  blast_repo fixes (dart-lang/test_descriptor#72)

test_process (https://github.com/dart-lang/test_process/compare/52ee3f5..c4986dd):
  c4986dd  2024-10-29  Kevin Moore  blast_repo fixes (dart-lang/test_process#63)

vector_math (https://github.com/google/vector_math.dart/compare/da9889f..3937447):
  3937447  2024-10-28  Kevin Moore  blast_repo fixes (google/vector_math.dart#333)

web_socket_channel (https://github.com/dart-lang/web_socket_channel/compare/40aa29f..1f15eca):
  1f15eca  2024-10-29  Kevin Moore  blast_repo fixes (dart-lang/web_socket_channel#385)

yaml (https://github.com/dart-lang/yaml/compare/e773005..6cc2745):
  6cc2745  2024-10-28  Kevin Moore  blast_repo fixes (dart-lang/yaml#171)

Change-Id: I0b3e0e1f9195bf4c1296b38a3b8839337444fd6d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/393040
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevmoo kevmoo kevmoo left review comments

@jakemac53 jakemac53 jakemac53 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@natebosch @kevmoo @jakemac53