Suppress unnecessary_cast to Function when the cast is allowing violation of avoid_dynamic_calls

[natebosch]

The pattern for allowing a dynamic call when the avoid_dynamic_calls is enabled is to explicitly cast to dynamic or Function in the expression that receives the dynamic call. Such a cast should suppress the unnecessary_cast diagnostic. When we cast to Function to allow the dynamic call, we end up with an unnecessary cast.

void foo(Function callback) {
  callback(); // avoid_dynamic_calls - this is a correct diagnostic, we want the dynamic function call to be obvious
  (callback as Function)(); // unnecessary_cast - this should be suppressed
}

Idiomatic usage of the lint is to keep references as Object? throughout as much of the code as possible, and introduce the dynamic only within the expression where a dynamic call is made. We assume backends will optimize a cast from Object? to dynamic. Neither dynamic or Object? is more specific than the other so we lose nothing when we change all dynamic to Object?. Function is more specific than Object?, so we would lose information changing all Function to Object or Object? just to be allowed to cast.

I think we should allow specifically casting a Function to Function when avoid_dynamic_calls is enabled.

[dart-github-bot]

Summary: The unnecessary_cast diagnostic incorrectly flags casts to Function when used to allow dynamic calls while avoid_dynamic_calls is enabled. This is because the cast is necessary to explicitly allow the dynamic call, even though it appears redundant.

[matanlurey]

A perhaps better option could be to allow as Function

[srawlins]

I think this would be a simple impl, and a good feature.

[srawlins]

I thought I understood this request, but after reading and thinking more, I think I need a lot of clarification 😁

• In the motivating example, is argument supposed to be callback?
• In the motivating example, is .something() supposed to be .call()? Or is this about extension methods?

Assuming the answers to the above are yes and yes, should we always allow foo as Function for foo with static type Function? Or conditionally on the surrounding expression? From the avoid_dynamic_calls unit tests, I see that:

• we report these situations:
  • p.call() for Function p,
  • p..call() for Function p,
• we don't report these situations:
  • p.call; for Function p,
• we don't have tests for:
  • p?.call() for Function? p,
  • p!.call() for Function? p,
  • p.call() for T-typed p bound to Function.

So it seems to be a short list of "surrounding expression situations" if we wanted the exception to be conditional.

[natebosch]

• In the motivating example, is argument supposed to be callback?

Yes.

• In the motivating example, is .something() supposed to be .call()? Or is this about extension methods?

It should have been a direct call of the function without any .. I must have copy/pasted the wrong content at some step when I was writing the example because the .something() would have been for an example related to dynamic rather than Function.

I fixed the example in the first comment.

See a real world motivating example at https://github.com/dart-lang/test/blob/9e349d0e9f6c477584ea320f7ba2f49f761d84ac/pkgs/matcher/lib/src/core_matchers.dart#L155

should we always allow foo as Function for foo with static type Function?

It looks like we always allow foo as dynamic when foo has any static type and we don't have any condition on the surrounding expression. I think we could allow foo as Function without any condition on the surrounding expression.

It is already the case that the unnecessary_cast diagnostic is only raised when foo has a static type of Function. Never raising an unnecessary_cast for as Function would solve and cause very little unintended impact. If we want to be extra cautions we could only suppress when avoid_dynamic_calls is enabled, but since we already allow as dynamic loosely I would not have any concerns about never reporting for as Function regardless of lint config.

It also is just occurring to me now that we already have the option of doing (f as dynamic)() in place of (f as Function)() which might be a sufficient workaround if there is a reason we don't want to suppress unnecessary casts on as Function.

@jakemac53 - how would you feel about using (f as dynamic)() and making it our recommended pattern for invoking functions that can't be cast to a specific type?