add lint function_expression_with_incorrect_types #2530

a14n · 2021-03-18T20:58:18Z

Description

Use the parameter types expected by the target in function expression.

BAD:

f(void Function(int a) p) {}

f((int? a) {});
f((num a) {});

GOOD:

f(void Function(int a) p) {}

f((a) {});
f((int a) {});

coveralls · 2021-03-18T21:01:51Z

Coverage increased (+0.03%) to 94.369% when pulling e867a8a on a14n:function_expression_with_incorrect_types into 9db7999 on dart-lang:master.

bwilkerson

I have to wonder how much value this adds, but I assume you've seen this pattern in a few places or you wouldn't have thought of the rule. Can you give me an idea of how common a violation of this lint would be?

bwilkerson · 2021-03-18T21:23:24Z

lib/src/rules/function_expression_with_incorrect_types.dart

+  void visitFunctionExpression(FunctionExpression node) {
+    DartType? expectedType;
+    var parent = node.unParenthesized.parent;
+    if (parent is ArgumentList) {


Use node.unParenthesized.staticParameterElement to get the element corresponding to the argument. This will correctly handle named arguments as well as arguments not inside an argument list.

bwilkerson · 2021-03-18T21:24:42Z

lib/src/rules/function_expression_with_incorrect_types.dart

+    if (expectedType == null || expectedType is! FunctionType) return;
+
+    // check type of each parameters
+    var currentType = node.staticType! as FunctionType;


Not sure what value there is in null checking the staticType before casting to a non-nullable type. It just changes the kind of exception that will be thrown.

Without null check cast_nullable_to_non_nullable triggers.

That just points out that I didn't think this through carefully enough yesterday. The style we're adopting on the analyzer team is to avoid runtime exceptions whenever possible now that we have sound typing. So I'm going to request that we have neither the null check nor the cast by doing something like

var currentType = node.staticType; if (currentType is! FunctionType) { return; }

bwilkerson · 2021-03-18T21:28:13Z

lib/src/rules/function_expression_with_incorrect_types.dart

+    var currentType = node.staticType! as FunctionType;
+    var currentParams = currentType.parameters;
+    var expectedParams = expectedType.parameters;
+    for (var i = 0; i < min(expectedParams.length, currentParams.length); i++) {


I'm guessing that this won't work for named parameters if the parameters are defined in different orders or if the closure allows named parameters that aren't in the expected type. For example

void g(void Function({String? s, int? i}) f) {} void h() { g(({int? i, String? s}) {}); }

Not fixed at all as names for positionnal parameters can change... Sorry

a14n · 2021-03-19T09:36:48Z

I have to wonder how much value this adds, but I assume you've seen this pattern in a few places or you wouldn't have thought of the rule. Can you give me an idea of how common a violation of this lint would be?

This lint was done after flutter/flutter#78143 (review). I expected several occurance of this lint in Flutter where types are required everywhere but surprisingly it didn't come up a lot.

bwilkerson

I have to question how much value this brings. Most users will follow the guideline of not adding type annotations in closures, and if it isn't doing much for Flutter then it's adding cognitive burden for users searching for a lint for not much benefit. Perhaps we shouldn't be adding it. @goderbauer Thoughts?

bwilkerson · 2021-03-19T15:04:01Z

lib/src/rules/function_expression_with_incorrect_types.dart

+    if (expectedType == null || expectedType is! FunctionType) return;
+
+    // check type of each parameters
+    var currentType = node.staticType! as FunctionType;


That just points out that I didn't think this through carefully enough yesterday. The style we're adopting on the analyzer team is to avoid runtime exceptions whenever possible now that we have sound typing. So I'm going to request that we have neither the null check nor the cast by doing something like

var currentType = node.staticType; if (currentType is! FunctionType) { return; }

bwilkerson · 2021-03-19T15:19:07Z

lib/src/rules/function_expression_with_incorrect_types.dart

+  @override
+  void visitFunctionExpression(FunctionExpression node) {
+    DartType? expectedType;
+    var parent = node.unParenthesized.parent;


I just double checked and I don't think unParenthesized is what you want either here or below; it will return either the node or a child of the node. I think you actually want the inverse, which doesn't exist (maybe something like thisOrAncestorNotInParentheses).

I wonder how often we've made the same mistake in other places.

bwilkerson · 2021-03-19T15:22:05Z

lib/src/rules/function_expression_with_incorrect_types.dart

+
+      // if param is not found on target, no lint
+      var expectedParam =
+          expectedParams.where((e) => e.name == currentParam.name).firstOrNull;


This will work for named parameters because they are required to have the same name, but the names of positional parameters are not important. For example:

void f(void Function(int a) p) {} void m() { f((int b) {}); // OK

bwilkerson · 2021-03-19T15:24:34Z

lib/src/rules/function_expression_with_incorrect_types.dart

+      // if type are not consistent, lint!
+      if (currentParam.isRequiredPositional && currentType != expectedType ||
+          currentParam.isOptional &&
+              currentType.element != expectedType.element) {


This condition doesn't cover the case of a required named parameter. It seems like it ought to.

goderbauer · 2021-03-19T17:42:16Z

I didn't know how common this pattern was. If we think it's not that common (which the data seems to indicate) I don't feel too strongly about having this lint.

a14n · 2021-03-21T21:27:49Z

@bwilkerson I face a strange behaviour with .staticParameterElement. The result seems to have a .library at null for method with generics (eg. Map.forEach). Is it expected?

EDIT: the version of the analyzer lib is 1.2.0

bwilkerson · 2021-03-22T15:23:57Z

The result seems to have a .library at null for method with generics (eg. Map.forEach). Is it expected?

I didn't think about it earlier, but I think I know what's happening. I suspect that the element being returned is a "view on an element" (a subclass of Member). This "view" holds the actual types after type parameters have been replaced by their corresponding arguments. They used to know what library they came from, but some language changes made that sometimes be a lie, so we changed the API. You can safely ask any element (including a member) for the declaration and then ask the declaration for the library and that should give you the result you want. Note however that the declaration can return null in which case there is no reliable way to get a library.

a14n · 2021-03-22T20:29:46Z

You can safely ask any element (including a member) for the declaration and then ask the declaration for the library and that should give you the result you want.

I still get null for parameterElement.library and parameterElement.declaration.library if the target method is parameterized.

a14n · 2021-03-22T20:44:52Z

.enclosingElement is also null weither on parameterElement or parameterElement.declaration. That seems to be logic regarding ParameterElementImpl.declaration that returns this (parameterElement or parameterElement.declaration are both ParameterElementImpl for generic method or not).

I made my tests on:

class MyMap<K, V> {
  void f(void Function(int a) p) {}
  void ff(void Function(K a) p) {}
}
m() {
  MyMap().f((int? a) {}); // parameterElement.library is correctly set
  MyMap().ff((Object? a) {}); // parameterElement.library is null
}

bwilkerson · 2021-03-22T21:18:11Z

@scheglov I'm guessing more has changed than I realized.

a14n · 2021-04-06T20:22:44Z

Any update on the underlying issue about .library being null ?

bwilkerson · 2021-04-06T20:37:38Z

@scheglov ?

scheglov · 2021-04-09T17:06:42Z

I think what happens here is that when a generic method is invoked, we instantiate the corresponding FunctionType and use its parameters for the context. And ParameterElement(s) of FunctionType are in nowhere - not attached to any library.

a14n · 2021-04-09T21:04:56Z

So IIUC it is a bug that need to be fixed on the analyzer side?

scheglov · 2021-04-10T00:36:37Z

Correct, this is an issue that should be fixed in the analyzer.

a14n · 2021-05-06T06:58:13Z

@scheglov is there a specific issue already created to track this problem or do you want I create one?

scheglov · 2021-05-09T19:16:09Z

@a14n please create a new one.

a14n · 2021-05-10T06:58:01Z

@a14n please create a new one.

Done with dart-lang/sdk#45964

srawlins · 2023-04-17T19:23:01Z

Apologies for the delay in reviewing (though perhaps this is mainly blocked on dart-lang/sdk#45964).

In order to have a more focused discussion on whether this rule has enough value, would you mind, @a14n , opening a lint proposal issue, as per our new lint lifecycle guidelines?

a14n · 2023-04-18T07:19:43Z

In order to have a more focused discussion on whether this rule has enough value, would you mind, @a14n , opening a lint proposal issue, as per our new lint lifecycle guidelines?

Done with #4281

google-cla bot added the cla: yes label Mar 18, 2021

bwilkerson reviewed Mar 18, 2021

View reviewed changes

bwilkerson reviewed Mar 19, 2021

View reviewed changes

a14n mentioned this pull request May 10, 2021

[analyzer] Expression.staticParameterElement should not be null for method with generics dart-lang/sdk#45964

Open

add lint function_expression_with_incorrect_types

e867a8a

a14n force-pushed the function_expression_with_incorrect_types branch from df81ac3 to e867a8a Compare May 10, 2021 06:59

a14n mentioned this pull request Apr 18, 2023

proposal: function_expression_with_incorrect_types #4281

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add lint function_expression_with_incorrect_types #2530

add lint function_expression_with_incorrect_types #2530

a14n commented Mar 18, 2021

coveralls commented Mar 18, 2021 •

edited

Loading

bwilkerson left a comment

bwilkerson Mar 18, 2021

a14n Mar 19, 2021

bwilkerson Mar 18, 2021

a14n Mar 19, 2021

bwilkerson Mar 19, 2021

bwilkerson Mar 18, 2021

a14n Mar 19, 2021

a14n Mar 19, 2021

a14n commented Mar 19, 2021

bwilkerson left a comment

bwilkerson Mar 19, 2021

bwilkerson Mar 19, 2021

bwilkerson Mar 19, 2021

bwilkerson Mar 19, 2021

goderbauer commented Mar 19, 2021

a14n commented Mar 21, 2021 •

edited

Loading

bwilkerson commented Mar 22, 2021

a14n commented Mar 22, 2021

a14n commented Mar 22, 2021

bwilkerson commented Mar 22, 2021

a14n commented Apr 6, 2021

bwilkerson commented Apr 6, 2021

scheglov commented Apr 9, 2021

a14n commented Apr 9, 2021

scheglov commented Apr 10, 2021

a14n commented May 6, 2021

scheglov commented May 9, 2021

a14n commented May 10, 2021

srawlins commented Apr 17, 2023

a14n commented Apr 18, 2023

add lint function_expression_with_incorrect_types #2530

Are you sure you want to change the base?

add lint function_expression_with_incorrect_types #2530

Conversation

a14n commented Mar 18, 2021

Description

coveralls commented Mar 18, 2021 • edited Loading

bwilkerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a14n commented Mar 19, 2021

bwilkerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer commented Mar 19, 2021

a14n commented Mar 21, 2021 • edited Loading

bwilkerson commented Mar 22, 2021

a14n commented Mar 22, 2021

a14n commented Mar 22, 2021

bwilkerson commented Mar 22, 2021

a14n commented Apr 6, 2021

bwilkerson commented Apr 6, 2021

scheglov commented Apr 9, 2021

a14n commented Apr 9, 2021

scheglov commented Apr 10, 2021

a14n commented May 6, 2021

scheglov commented May 9, 2021

a14n commented May 10, 2021

srawlins commented Apr 17, 2023

a14n commented Apr 18, 2023

coveralls commented Mar 18, 2021 •

edited

Loading

a14n commented Mar 21, 2021 •

edited

Loading