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.

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

avoid_types_on_closure_parameters false positive with implicit-dynamic #58695

Closed
rrousselGit opened this issue Apr 5, 2022 · 5 comments
Closed
Assignees
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-positive P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@rrousselGit
Copy link

Describe the issue
Some SDK functions type their error callback parameter as Function to accept both Function(Object) and Function(Object, StackTrace):

Future future;
future.then(..., onError: <this callback>);

As such, writing the following will trigger implicit-dynamic:

Future future;
future.then(..., onError: (err, stack) {
  // err and stack are both dynamic
});

The solution should be to explicitly write the type:

Future future;
future.then(..., onError: (Object err, StackTrace stack) {
  // fixed implicit-dynamic
});

but doing this now causes avoid_types_on_closure_parameters on both parameters

Expected behavior

I would expect avoid_types_on_closure_parameters to detect that Object/StackTrace are types more specific than dynamic, and therefore allow it.

Additional context

Possibly related to #57586 and #58183

@srawlins
Copy link
Member

srawlins commented Apr 5, 2022

I think this would be a special case we'd have to hard-code into the linter. The type of that callback is Function, since we don't have union types. The real type (as enforced by the analyzer, in special case code) is void Function(Object)|void Function(Object, StackTrace), I think.

I believe special casing in this lint rule would be a prudent, practical, beneficial policy.

@rrousselGit
Copy link
Author

Assuming the special casing is based on Function not Future.then(onError:) then I think that's fine

Because ultimately folks may write utilities around such as:

void doSomething(Function fn) {
  Stream().handleError(fn);
}

// will warn
doSomething((Object err, StackTrace stack) {

});

@a14n
Copy link
Contributor

a14n commented Apr 7, 2022

In my opinion the issue is more general and arises at several places (assignments, arguments, declarations...) where a dynamic, Object, Function is expected. For instance in the following case I don't think the lint should trigger:

  dynamic f1;
  f1 = (int a) {}; // LINT
  Function f2;
  f2 = (int a) {}; // LINT
  Object f3;
  f3 = (int a) {}; // LINT
  var l = [
    (int a) => 1, // LINT
  ];

@srawlins srawlins added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) linter-false-positive labels Jun 29, 2022
@NANASHI0X74
Copy link

NANASHI0X74 commented Jul 27, 2022

this is quite a pain point for me, because I do want the lint to prevent me and my colleagues from putting type annotations on closures wherever possible, but I also don't want to have to pepper the code with // ignore: comments whereever I use provider's SelectContext.select method: https://pub.dev/documentation/provider/latest/provider/SelectContext/select.html

context.select((SomeModel m) => m.attr), //LINT

@bwilkerson bwilkerson added the P3 A lower priority bug or feature request label Nov 7, 2022
@srawlins srawlins self-assigned this May 24, 2024
copybara-service bot referenced this issue May 25, 2024
Fixes https://github.com/dart-lang/linter/issues/1099
Fixes https://github.com/dart-lang/linter/issues/3330

We just add a check that the (approximate) context type is a function type.

Cq-Include-Trybots: luci.dart.try:flutter-analyze-try,analyzer-win-release-try,pkg-win-release-try
Change-Id: I56fe14ff8852375754fdaf6b92b3c632b7df9c95
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/367982
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
@srawlins
Copy link
Member

Fixed with f07e042

@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-positive P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants