Initial commit of avoid_dynamic_calls. #2417

matanlurey · 2021-01-13T02:30:46Z

This is a pretty difficult lint to get "right". I tried my best with the following principals:

Simple existence of dynamic or a dynamic-ly typed expression should not cause a lint.
There are many places where a dynamic expression can be used in a way that does not "cause" a dynamic call:
- Some types of expressions, such as as, is, ??, or x! are non-virtual, and are not dynamic calls.
- Some trickier cases like members that exist on Object? (==, hashCode, runtimeType, toString).
Implicit downcasts from dynamic to non-dynamic, while bad, is not a dynamic call (it's a runtime cast).
There are other lints and analysis modes that can lint these.
While not explicitly typed dynamic, a static type of Function being invoked is a dynamic call.

Open to any and all suggestions. I'm also happy landing this is an experimental state for others to give more feedback. One thing I'd like to avoid is splitting this into like, half-a-dozen different lints or too many ways to opt-out yet; I think // ignore: avoid_dynamic_call isn't great, but we can look at the UX of opting-out independent of landing this lint.

matanlurey · 2021-01-13T02:32:47Z

/cc @leafpetersen

srawlins

I think this looks great.

lib/src/rules/avoid_dynamic_calls.dart

test/rules/experiments/nnbd/rules/avoid_dynamic_calls.dart

srawlins · 2021-01-13T04:57:51Z

test/rules/experiments/nnbd/rules/avoid_dynamic_calls.dart

+}
+
+void equalityExpressions(dynamic a, dynamic b) {
+  a == b; // LINT


What a bummer. If anything, this check might really slow uptake of this rule internally in Google. Ideally though, I'd like to keep this check, because you're right.

Actually, members of Object are a special case. See overall comment.

I think it's a special case, but only for the VM, and not worth special, special-casing :P

test/rules/experiments/nnbd/rules/avoid_dynamic_calls.dart

srawlins · 2021-01-13T05:04:45Z

I'm passing the review to @pq; implementation looks great; this will be much hailed inside Google.

askeksa-google

This is great!

lib/src/rules/avoid_dynamic_calls.dart

test/rules/experiments/nnbd/rules/avoid_dynamic_calls.dart

lib/src/rules/avoid_dynamic_calls.dart

matanlurey · 2021-01-14T00:59:24Z

PTAL, and thanks for all the motivating +1s and great ideas to expand testing and correctness.

matanlurey · 2021-01-14T01:50:02Z

Thanks! @pq, as lint 👑 , I want to give you a chance to do a once-over before merging. Thanks in advance!

askeksa-google

I dug into the == issue. As it turns out, the CFE treats invocations of Object members as non-dynamic even on receivers of type dynamic. This means that all of the following are OK:

void foo(dynamic a, dynamic b) {
  a == b;
  a.hashCode;
  a.noSuchMethod;
  a.noSuchMethod(Invocation());
  a.runtimeType;
  a.toString;
  a.toString();
}

For the methods (noSuchMethod and toString) this only holds when the argument list matches the declaration in Object. Thus, the following should lint:

void foo(dynamic a, dynamic b) {
  a.noSuchMethod();
  a.noSuchMethod(Invocation(), "extra");
  a.toString(radix: 16);
}

This is also in line with the language specification, which treats Object members specially w.r.t. the type of the invocation expression.

askeksa-google · 2021-01-14T10:53:38Z

test/rules/experiments/nnbd/rules/avoid_dynamic_calls.dart

+}
+
+void equalityExpressions(dynamic a, dynamic b) {
+  a == b; // LINT


Actually, members of Object are a special case. See overall comment.

pq

🎉

Super awesome!

lib/src/rules/avoid_dynamic_calls.dart

pq · 2021-01-14T15:23:27Z

lib/src/rules/avoid_dynamic_calls.dart

+
+**DO** avoid method calls or accessing properties on an object that is either
+explicitly or implicitly statically typed "dynamic". Dynamic calls are treated
+slightly different in every runtime environment and compiler, but most


=> differently?

lib/src/rules/avoid_dynamic_calls.dart

bwilkerson · 2021-01-14T16:01:05Z

As it turns out, the CFE treats invocations of Object members as non-dynamic even on receivers of type dynamic.

I'm guessing that by "non-dynamic" you mean that they don't incur the cost of "dynamic" calls (because they do still perform a lookup to find the right method to invoke).

If that's true, then the question becomes: Do we want to flag all invocations on an expression whose type is dynamic as stated in the docs, or do we only want to flag invocations that have an associated cost using todays compiler? The former is more consistent and requires less maintenance (with the second answer we need to change this lint every time the implementation of these cases changes), so I would lean in that direction.

matanlurey · 2021-01-14T17:41:13Z

void foo(dynamic a, dynamic b) {
  a == b;
  a.hashCode;
  a.noSuchMethod;
  a.noSuchMethod(Invocation());
  a.runtimeType;
  a.toString;
  a.toString();
}

@askeksa-google:

I think the CFE might have a different definition of "dynamic call" than what this lint is trying to address. Consider:

void foo(dynamic a, dynamic b) {
  a == b;
  a.hashCode;
  a.noSuchMethod;
  a.runtimeType;
  a.toString;
  a.toString();
}

void main() {
  foo(Example(), {});
}

class Example {
  operator==(_) {
    print('>>> A.operator==');
    return true;
  }
}

... this prints >>> A.operator==, which to me, means it dispatched, on an otherwise untyped object, the == operator, because it could be user-definable. I'm not very aware of how maybe the CFE and VM implements this, but at least for Dart2JS and DDC, this is a non-zero cost and requires interceptors, something we are trying to avoid with this lint.

askeksa-google · 2021-01-14T17:45:45Z

I'm guessing that by "non-dynamic" you mean that they don't incur the cost of "dynamic" calls (because they do still perform a lookup to find the right method to invoke).

Yes, what I mean is that they are treated exactly the same as if the receiver had type Object? rather than dynamic.

I would definitely say we should not treat these calls as dynamic for the purpose of the lint. It is not so much a matter of what the current compilers are able to optimize (there are lots of other cases where a dynamic call can be eliminated in practice). It is a situation which specifically called out in the spec as an exception to the rules about member access on dynamic.

The spec does not define what is a dynamic call or not (since that is implementation terminology), but it distinguishes between member access which is statically typed and member access which is not. And in this distinction, accessing an Object member on a dynamic receiver falls on the "statically typed" side.

matanlurey · 2021-01-14T17:46:22Z

I could see dropping the Object.<method> defined bits before landing this lint, especially if means adoption will be higher, so feel free to push-back if you think ==, .runtimeType, .hashCode, and .toString should be special-cased and I'll make an adjustment.

lib/src/rules/avoid_dynamic_calls.dart

askeksa-google · 2021-01-14T18:02:55Z

... this prints >>> A.operator==, which to me, means it dispatched, on an otherwise untyped object, the == operator, because it could be user-definable. I'm not very aware of how maybe the CFE and VM implements this, but at least for Dart2JS and DDC, this is a non-zero cost and requires interceptors, something we are trying to avoid with this lint.

That dispatch happens for a receiver of type Object (or any other concrete type) as well (after checking both operands against null), so this is not the distinction we are after, as I see it.

I could see dropping the Object. defined bits before landing this lint, especially if means adoption will be higher, so feel free to push-back if you think ==, .runtimeType, .hashCode, and .toString should be special-cased and I'll make an adjustment.

Yes, I think that would make the lint both more practically useful and more in line with how the spec describes dynamic.

Another way to see it is that for a dynamic x the spec basically defines x.objectAccessor to be treated as (x as Object?).objectAccessor where as Object? is a no-op at runtime (but has static type implications).

matanlurey · 2021-01-14T18:07:29Z

I loosened checks around members that exist on Object? and tightened up some docs.

scheglov · 2021-01-14T18:15:49Z

lib/src/rules/avoid_dynamic_calls.dart

+  @override
+  void visitPrefixExpression(PrefixExpression node) {
+    // Implemented as: node?.staticType?.isDynamic == true
+    if (_lintIfDynamic(node.operand)) {


Thank you for the clarification.

I think we want to to such switch on the operator to explicitly express what we are checking and when. Doing one generic _lintIfDynamic when for ++x the type is null does not make this code as easy to read as it could be.

And for ++ and -- we want to check CompoundAssignmentExpression.readType directly. Because this is the type on which we will call operator+ or operator-. Just like we do for assignments.

Similarly for postfix expressions.

Thanks, that's a solid idea. I've consolidated and de-duplicated the code into _lintPrefixOrPostfixExpression(node, node.operand);. PTAL.

askeksa-google

With comments addressed, this looks good.

askeksa-google · 2021-01-14T18:14:30Z

lib/src/rules/avoid_dynamic_calls.dart

@@ -193,6 +195,11 @@ class _Visitor extends SimpleAstVisitor<void> {

  @override
  void visitMethodInvocation(MethodInvocation node) {
+    final methodName = node.methodName.name;
+    if (methodName == 'noSuchMethod' || methodName == 'toString') {


Also check the number of arguments. In order to be ignored by the lint, there should be 1 positional parameter for noSuchMethod and 0 for toString, and in both cases no named parameters.

askeksa-google · 2021-01-14T18:16:29Z

lib/src/rules/avoid_dynamic_calls.dart

@@ -216,6 +235,11 @@ class _Visitor extends SimpleAstVisitor<void> {

  @override
  void visitPrefixedIdentifier(PrefixedIdentifier node) {
+    final property = node.identifier.name;
+    if (property == 'hashCode' || property == 'runtimeType') {


A tearoff of noSuchMethod or toString is also not considered dynamic.

askeksa-google · 2021-01-14T18:21:02Z

test/rules/experiments/nnbd/rules/avoid_dynamic_calls.dart

+  a.hashCode; // OK
+  a.runtimeType; // OK
+  a.noSuchMethod(null as Invocation); // OK
+  a.toString(); // OK


Also test tearoffs of noSuchMethod and toString (OK) and calling these with the wrong number of arguments, or with named arguments (LINT).

I did the most I could. I think being more comprehensive than this is likely infeasible, as the analyzer issues compile-time errors already for the wrong number of arguments on these methods, including tear-offs, and I'm not able to lint them as a result.

askeksa-google · 2021-01-14T18:48:03Z

lib/src/rules/avoid_dynamic_calls.dart

+
+... these members are dynamically dispatched in the web-based runtimes, but not
+in the VM-based ones. Additionally, they are so common that it would be very
+punishing to disallow `any.toString()` or `any == true`, for example.


I think this description is misleading. AFAIU, dart2js does not distinguish between dynamic and non-dynamic calls at all (except for possibly inserting extra type checks). In DDC, the calls in

String foo1(dynamic x) => x.toString(); String foo2(Object? x) => x.toString(); String foo3(Object x) => x.toString();

are all translated identically, but actual dynamic calls, like

String bar1(dynamic x) => x.toString(color: "fuchsia"); String bar2(dynamic x) => x.nothing();

are translated differently from that (using dart.dsend, giving the name of the method as a string).

So the categorization of these calls as non-dynamic holds equally well across all of our compilers.

That can't possibly be true, because undefined.toString() andnull.toString() in JS is a runtime error. So there would need to be special handling/interceptors that is not based entirely on virtual dispatch.

In either case I think we can tweak the description later. Merging!

dart-archive/linter#2417

Initial commit of avoid_dynamic_calls.

b05d1f7

matanlurey requested review from pq, srawlins and askeksa-google January 13, 2021 02:30

google-cla bot added the cla: yes label Jan 13, 2021

A few more tweaks for is/as.

7087d91

Regenerate example/all.yaml.

3de41ce

srawlins reviewed Jan 13, 2021

View reviewed changes

askeksa-google approved these changes Jan 13, 2021

View reviewed changes

bwilkerson reviewed Jan 13, 2021

View reviewed changes

lib/src/rules/avoid_dynamic_calls.dart Show resolved Hide resolved

lib/src/rules/avoid_dynamic_calls.dart Show resolved Hide resolved

lib/src/rules/avoid_dynamic_calls.dart Show resolved Hide resolved

lib/src/rules/avoid_dynamic_calls.dart Show resolved Hide resolved

Update and expand test cases.

91facb4

Implement Brian's helpful suggestions.

5863ace

bwilkerson approved these changes Jan 14, 2021

View reviewed changes

Spelling is hard.

8230145

askeksa-google suggested changes Jan 14, 2021

View reviewed changes

pq approved these changes Jan 14, 2021

View reviewed changes

matanlurey requested a review from askeksa-google January 14, 2021 17:42

Address feedback.

a4e5399

scheglov reviewed Jan 14, 2021

View reviewed changes

lib/src/rules/avoid_dynamic_calls.dart Outdated Show resolved Hide resolved

Update due to comments.

b4f4222

scheglov reviewed Jan 14, 2021

View reviewed changes

Reduce code de-duplication, use .readType.

f937cdd

scheglov approved these changes Jan 14, 2021

View reviewed changes

askeksa-google approved these changes Jan 14, 2021

View reviewed changes

Add expand allowed Object? members.

0bdf7dc

matanlurey merged commit 6a49a01 into dart-archive:master Jan 14, 2021

matanlurey deleted the avoid-dynamic-calls branch January 14, 2021 19:44

askeksa-google mentioned this pull request Jan 21, 2021

False positive of avoid_dynamic_calls on .call() dart-lang/sdk#58309

Closed

mockturtl added a commit to mockturtl/tidy that referenced this pull request Mar 3, 2021

feat: avoid_dynamic calls

3851d65

dart-archive/linter#2417

Initial commit of avoid_dynamic_calls. #2417

Initial commit of avoid_dynamic_calls. #2417

Uh oh!

Conversation

matanlurey commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matanlurey commented Jan 13, 2021

Uh oh!

srawlins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srawlins commented Jan 13, 2021

Uh oh!

askeksa-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matanlurey commented Jan 14, 2021

Uh oh!

matanlurey commented Jan 14, 2021

Uh oh!

askeksa-google left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwilkerson commented Jan 14, 2021

Uh oh!

matanlurey commented Jan 14, 2021

Uh oh!

askeksa-google commented Jan 14, 2021

Uh oh!

matanlurey commented Jan 14, 2021

Uh oh!

Uh oh!

askeksa-google commented Jan 14, 2021

Uh oh!

matanlurey commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

askeksa-google left a comment

Choose a reason for hiding this comment

matanlurey commented Jan 13, 2021 •

edited

Loading

askeksa-google left a comment •

edited

Loading

matanlurey Jan 14, 2021 •

edited

Loading