Add lint implicit_call_tearoffs

[natebosch]

In an effort to simplify the language we may consider removing the
implicit call tearoff which can coerce an instance of any class which
defines a call method into a Function type. Authors should
explicitly add the .call so there is less magic.

[natebosch]

I'm having trouble with this lint. I don't see any of the ImplicitCallReference nodes that I expect.

Testing this locally requires a dependency override on analyzer with https://dart-review.googlesource.com/c/sdk/+/254465

The reason I expect to see an ImplicitCallReference in the AST is I can see that the _insertImplicitCallReference path is being hit (for at least some of the expressions I expected), and the NodeReplacer.replace call returns true.
https://github.com/dart-lang/sdk/blob/104d43fa59866f4d250d3c2c108a008ef8d45a7b/pkg/analyzer/lib/src/generated/resolver.dart#L2636-L2642

However when the test added in this PR fails and it dumps the AST, I don't see ImplicitCallReference anywhere in the output.

For instance in the test I have the line callIt(c) for which the c expression should be replaced with an ImplicitCallReference, but in the dump I see:

          ExpressionStatementImpl [callIt(c);] //LINT
            MethodInvocationImpl [callIt(c)] 
              SimpleIdentifierImpl [callIt] 
              ArgumentListImpl [(c)] 
                SimpleIdentifierImpl [c] 

I expected this to be

          ExpressionStatementImpl [callIt(c);] //LINT
            MethodInvocationImpl [callIt(c)] 
              SimpleIdentifierImpl [callIt] 
              ArgumentListImpl [(c)] 
                ImplicitCallReferenceImpl [c]
                  SimpleIdentifierImpl [c] 

cc @stereotype441

[bwilkerson]

@srawlins @scheglov

Is it possible that the ImplicitCallReference node is being dropped by either writing or reading the summary file?

[stereotype441]

It turns out that the utility function that dumps the AST in the event of a test failure does a fresh parse of the AST (see https://github.com/dart-lang/sdk/blob/f45353eafd1a28b5b33ff227a5c46c657c0b550c/pkg/analyzer/lib/src/lint/util.dart#L57), so that's why you weren't seeing ImplicitCallReference nodes in your AST dump.

[natebosch]

@bwilkerson - do you expect that an expression like Callable() as Function should get a ImplicitCallReference node in the AST?

I can see that we check whether to insert an implicit call reference, however the contextType argument is always null, and the parent won't be an AssignmentExpression, so it will bail before inserting an implicit call reference

Is it possible there is a bug that is causing some implicit call tearoffs to not be represented?

[bwilkerson]

I don't know. I think @srawlins added that node, so he's likely to be a better source for answers than I am.

[srawlins]

I can see that we check whether to insert an implicit call reference, however the contextType argument is always null, and the parent won't be an AssignmentExpression, so it will bail before inserting an implicit call reference

Hmm I doubt that the contextType passed in to visitAsExpression on line 983 is buggy, as it is used in flow analysis, etc. But maybe we should treat as Function as an (explicit) implicit tear-off of .call? Since subclassing Function is not allowed. I don't think I accounted for as Function when writing the feature.

[srawlins]

Is it possible there is a bug that is causing some implicit call tearoffs to not be represented?

Most definitely, since it's not used a lot in the wild.

[natebosch]

Hmm I doubt that the contextType passed in to visitAsExpression on line 983 is buggy, as it is used in flow analysis, etc.

In what situation would you expect it to be non-null? When I see this method hit in the code I'm parsing I only have seen null.

But maybe we should treat as Function as an (explicit) implicit tear-off of .call? Since subclassing Function is not allowed. I don't think I accounted for as Function when writing the feature.

I had assumed that you did account for as Function base on the handling of implicit call references in visitAsExpession. Can you help me understand the intent of that call if we aren't accounting for as Function with implicit call references?

[srawlins]

In what situation would you expect it to be non-null? When I see this method hit in the code I'm parsing I only have seen null.

I would expect it to be non-null for something like the AsExpression here:

T f(Callable callable) => callable as Function;

I think the contextType there should be T (like int or Function or something).

I had assumed that you did account for as Function base on the handling of implicit call references in visitAsExpession. Can you help me understand the intent of that call if we aren't accounting for as Function with implicit call references?

No they're just sort of accounted for in most expressions. I'm not sure the list, but for example in this test case we rewrite c1 ?? c2 to be an ImplicitCallReference, which I think takes place in visitBinaryExpression

There are more tests below that which show which expressions we'll rewrite as an implicit call tear-off.

I actually can't get a similar test for an AsExpression to generate an ImplicitCallReference node; it might be because inference is never used when computing the static type of an as-expression (the static type is just always the right side if the as-expression).

[natebosch]

Ah, I see now that as Function does not have an implicit call tearoff like I thought it did. It throws at runtime instead.

[coveralls]

Coverage increased (+0.005%) to 95.791% when pulling e940a65 on avoid-implicit-call-tearoffs into 22cc957 on main.

[natebosch]

@pq - this lint is ready for review.

[kevmoo]

@natebosch – we should add this to lints/core.yaml – right? Do we have an issue open to track that?

[natebosch]

we should add this to lints/core.yaml – right?

Yes

Do we have an issue open to track that?

dart-lang/language#2399 tracks the language change. I noted there that we'll also want to include this line in the core set.

[natebosch]

@pq - do you have an opinion on whether this should be named avoid_implicit_call_tearoffs instead? I had that originally and renamed it, IIRC because I saw something that indicated we wanted to move away from that naming pattern, but now I can't find the reference.

[pq]

@pq - do you have an opinion on whether this should be named avoid_implicit_call_tearoffs instead?

I don't feel super strongly to be honest but we've been trending away from prefixes like avoid so implicit_call_tearoffs feels more consistent.

[bwilkerson]

+1 to not using "avoid".

[pq]

This looks great. Could we rename it to implicit_call_tearoffs (plural)?

[srawlins]

I think it's great. Love the tests.

[pq]

Awesome. Thanks!