Sdk: Front end does not warn on function calls on Object

[gbracha]

The analyzer gives no warning on function calls on the types Object and Function.

main() {
  Object x;
  Function f;
  x(); // no warning
  x(3); // no warning
  f(5,2); // no warning
  x.call(); //does warn correctly
  f.call ; // no warning
}

Note that neither Object nor Function have a call method, and name completion on Function knows this. Function does not have a call because one does not know what signature it would have; hence it makes no sense to assume that Function implies any particular function signature.

[bwilkerson]

_Removed Priority-Unassigned label._
_Added Priority-High label._

[bwilkerson]

_Added this to the 1.10 milestone._

[stereotype441]

_Set owner to @stereotype441._
_Added Started label._

[stereotype441]

Unfortunately, there are several packages in third_party that have come to rely on this behavior. (At the very least: collection, html5lib, matcher, stack_trace, and unittest. There may be others). So we have a bit of a chicken and egg problem: we need to modify those packages and push out new versions of them before we can fix the analyzer, but we need a fixed analyzer in order to figure out where the packages need to be modified.

I'll proceed as follows:

1. Introduce a fix to analyzer behind a flag, so that we can enable and disable the fix at will.
2. Work with the maintainers of the third_party packages to get those packages fixed.
3. Remove the flag so that analyzer always does the warning checks correctly.

[bwilkerson]

_Removed this from the 1.10 milestone._
_Added this to the 1.11 milestone._

[lrhn]

Should this be a language change?
I know I've used "Function" as a parameter or field type when I didn't want to specify a specific function type, and called the function without casting it to dynamic first. I can see that it's (spec-wise) bad, but it's also very convenient.

Maybe the "Function" type should be special-cased to allow it to act as a "dynamic for function calls" - no warning if you call something typed Function or gets its call method. It's not trivial, because you do want a warning if the static type is a subtype of Function.

---

cc @gbracha.

[gbracha]

I think we should look at this carefully, especially at Erik's proposal to generify Function so that it could support a sensible call method.

[stereotype441]

The situation has changed as a result of commit 2b95ed1ae1110e1166a89f61b195322d71866887, which changed the language spec so that the type Function is treated as though it had a method call satisfying all possible signatures. However, analyzer behavior is still not correct, since analyzer permits Objects to be called without a warning.

Unfortunately I don't think it's feasible to fix this for 1.12. This needs to be addressed early in a release cycle so that we have time to track down and fix any warnings that are introduced by the change.

[sethladd]

Might want to take off the "P1 high" label? This is a pretty old issue. cc @kevmoo

Also, the spec has been updated to allow a .call() on Function. This program analyzes cleanly:

typedef void CallMe();

void main() {
  CallMe callme = () => print("Called.");
  callme();
  callme.call();

  callme?.call();
}

[vsmenon]

@leafpetersen @floitschG - seems like at least some of these ought to be strong mode errors?

[leafpetersen]

The Object ones should clearly be errors. My take on Function is that it should be treated like dynamic when you call it, or use the call method (unless it is something which extends Function and has a call method perhaps?).

[lrhn]

If the static type of something is exactly Function, then calling it is treated as a dynamic call. That is, treat is as dynamic, but only for calls (and probably o.call(...) method invocations).

[vsmenon]

BTW, currently, with strong mode, the behavior is identical to the original comment except that the warning on x.call() is an error.

Sound like the consensus is that all these operations on x (Object) should be an error, but the ones on f should be fine:

main() {
  Object x;
  Function f;
  x(); // no warning -> should be error
  x(3); // no warning -> should be error
  f(5,2); // no warning
  x.call(); // error
  f.call ; // no warning
}

sgtm

[leafpetersen]

It looks like there is an enableStrictCallChecks option in the analyzer now (--enable-strict-call-checks) that turns all of these into errors. Flutter uses this option.

As far as I can see, the fact that there is no error on Object calls is just a bug, and should be moved out from under this flag.

The calls to Function are more debatable. These could be made Dart 2.0 errors, or this could be made an extra static analysis restriction.

@lrhn @eernstg @floitschG @munificent thoughts?

@Hixie I'm guessing you guys will want the warnings on calls to Function in some way, but don't care whether it's a language thing or a lint thing?

[eernstg]

Starting with the treatment that I would find "natural":

I would expect an entity of type Function to be callable, with no warnings or errors, with all argument list shapes and actual arguments of any type. That is, it would be "dynamic for functions".

Member lookups on an entity of type Function would be treated the same as member lookups for Object, that is, almost everything is a compile-time error.

The exception would be call, to the extent that we have the equivalence "calling an entity f with arguments (a1..ak) is the same thing as the instance method invocation f.call(a1..ak)" (skipping named arguments for brevity).

If we _do_ have that equivalence then all invocations of the form f.call(...) should be allowed when f has type Function, and the tear-off f.call would be allowed and have type Function (and it might simply return f itself).

If we switch over to the static model where classes implementing call can be invoked, but only because there is static semantic sugar for expanding an invocation like g(...) to g.call(...) then we can reasonably claim that functions and call have nothing to do with each other, we just allow for the creation of "function-like" objects where invocations of a method called call can be expressed concisely. In that case we should not allow for f.call(...) when f has type Function, and similarly for tear-offs.

Considering that we mostly agreed to go the static way already a while ago, I'd suggest that we keep the support for call as limited as possible.

But, other than that, letting Function play the role as "dynamic for functions" seems useful and meaningful to me.

[Hixie]

I would be fine with dropping support for call entirely.

[matanlurey]

+1 on dropping call - this has been confusing to at least _me_ for a long time.

[mit-mit]

@Hixie but you are fine with this for Flutter, right? (see https://github.com/flutter/flutter/issues/13146 for a concrete Flutter instantiation)

main() {
  Function f;
  f(5,2); // no warning
}

[Hixie]

For the framework, we never use Function (it's barely any better than dynamic), and would be fine with dropping Function entirely too (in favour of typedefs). For our developers, I guess they want to be able to do that, yes.

[munificent]

As far as I can see, the fact that there is no error on Object calls is just a bug, and should be moved out from under this flag.

+1. If you want, you can resurrect my duped old bug for that.

@lrhn @eernstg @floitschG @munificent thoughts?

My intuition about what I expect Dart to do based on my historical understanding of Dart lines up with @eernstg. In terms of what we want to do going forward, I am not attached at all to either of:

• Having Function as a type. It's only barely marginally more information-conveying than dynamic. It probably has negative value because it encourages people to not type their functions very precisely.

• call() as a magic named method. It was always weird. I don't know why they gave it a user-accessible method name when all of the other operators don't have one.

I'd be happy to see them killed, but if other people really care about them, it's not a big deal to me.

[anders-sandholm]

Having discussed this today with folks here, my - possibly oversimplified - understanding is as follows:

1) Object calls are not OK. It should be a warning in Analyzer (and an error in Dart 2). This has been a bug for a long time and is what this issue is really about and also what Bob's issue #28284 covers.

2) Function calls are OK. There should be no warning in Analyzer and we don't expect this to become an error in Dart 2. This also seems to be in line with user expectations (flutter/flutter#13146).

Assuming we agree on the above I'd like to propose that we remove the "--enable-strict-call-checks" flag (at least for Flutter users) once this issue has been fixed in Analyzer.

Raising prio - we need this fixed.

[leafpetersen]

LGTM

[mit-mit]

cc @devoncarew, who is the right assignee for this?

[vsmenon]

@kmillikin @sigmundch - the CFE would need to mark this (f() where f is Object) as an error as well, right?

[lrhn]

It is an error, so yes, everybody reporting strong mode errors should do the same thing.

[devoncarew]

Would the fix for this live in the analyzer or in the CFE? I assume the CFE, as we get our 2.0 language errors from it, and it sounds like this wound be a strong mode / 2.0 error.

[stereotype441]

@devoncarew We definitely should implement the proper rules in the common front end. I have it on my list to do that soon.

As to whether we need to do a parallel fix in the analyzer, that depends on the urgency of this bug. If we want to get this behavior change in customers' hands soon (and I assume we do, since it's marked "P1 high"), we'll need to do a parallel fix in the analyzer.

[devoncarew]

My guess is the P1-ness of the issue is more about 'we should do this' rather than 'we should do it this week'. Without further input from @anders-sandholm (who bumped to P1), I'd say that getting this in the CFE would be enough to close out the issue.

[stereotype441]

Ok, I'm recategorizing (and retitling) this as a front end bug, and assigning to myself; I am working on it now.

[anders-sandholm]

@devoncarew yes P1 as in we should do this because it seems to be important (not as in P1 because it is super urgent). Just wondering, when would this land in the hands of our customers if we don't do the parallel fix?

@stereotype441 thanks for moving it to CFE and taking this on.

[devoncarew]

when would this land in the hands of our customers if we don't do the parallel fix?

They's see it as the --preview-dart-2 flag starts rolling out, with the opt-in / opt-out / flag removal timeline; so, gradually over the next few months.

[leafpetersen]

It's possible that this might be a small enough change to the analyzer that it implementing it in parallel would be a net win by pulling more of the migration cost forward. On the other hand, I don't know if this actually comes up much.

[devoncarew]

Yeah, if it's cheap to do, it would help give us visibility into the impact of the change.

[mit-mit]

Yes, I would love to see that. Landing that change earlier via the analyzer
into Flutter would be one less thing to worry about later.

On Fri, Dec 1, 2017 at 1:32 AM, Devon Carew [e-mail]
wrote:

Yeah, if it's cheap to do, it would help give us visibility into the
impact of the change.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/sdk/issues/21938#issuecomment-348355123,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANAxitHHP8NLjLj1NuX4h2Wewp8KKGAIks5s7zr6gaJpZM4Fs_K7
.

[stereotype441]

Ok, since I've taken over this bug for the front end work I'll file a separate one for landing the change in analyzer.

[mit-mit]

@leafpetersen @lrhn as the present bug is being used as the 'specification' for the implementation bugs, can you confirm this is the behaviour we decided on? This is similar to what @vsmenon had in https://github.com/dart-lang/sdk/issues/21938#issuecomment-271341191, but with f.call marked as an error.

main() {
  Object x;
  Function f;
  x(); // no warning -> should be error
  x(3); // no warning -> should be error
  f(5,2); // no warning
  x.call(); // error
  f.call ; // error
}

[eernstg]

If anyone worries about making f.call an error: I believe it is a non-problem.

Whenever someone evaluates f.call where f has type Function, it should be semantically equivalent (and simpler, both textually and possibly at run time) to evaluate f instead. So there is no incentive to use such an expression (and hopefully almost nobody ever did that), and the fix would be to delete .call, which is quick and local.

[lrhn]

I would not make f.call an error. I don't worry about it, I just don't want to.

It's a dynamic access, similar to what would happen if f had type dynamic.
We know that f is a function, and all functions in Dart have a call method.
The static type of f.call should be Function again (the unknown function type).
I see no reason not to do that.

There are practical uses for accessing the call member of a function, the primary being f?.call().

Another case used to be sa a way to extract a function from a callable object, so you won't expose the entire object. I guess that won't be a problem in Dart 2, but I see no reason to break existing code.

[eernstg]

We know that f is a function, and all functions in Dart have a call method.

Well, the proposal to make f.call an error was explicitly presented as a subproposal to the language change where call is static semantic sugar only (so when we know by the static type of e that it has a call method we allow e(...) and it will be compiled into e.call(...), and it is _not_ guaranteed that all functions have a call method). If we don't change call to be sugar in this sense, the situation is of course different.

[lrhn]

If functions do not have a call method, then Function x; x.call; would be an error, yes.

[stereotype441]

For the record, here is the behavior I implemented in e8091b27b12f78df00f41b4b535e5560ea20864f:

void test(dynamic d, Object o, Function f) {
  d(); //# 01: ok
  o(); //# 02: compile-time error
  f(); //# 03: ok
  d.call; //# 04: ok
  o.call; //# 05: compile-time error
  f.call; //# 06: ok
  d.call(); //# 07: ok
  o.call(); //# 08: compile-time error
  f.call(); //# 09: ok
}

If this is incorrect, please feel free to re-open this bug.

[mit-mit]

@leafpetersen @lrhn can you confirm that the https://github.com/dart-lang/sdk/issues/21938#issuecomment-349365719 behaviour is correct?

[lrhn]

Looks good to me.

[leafpetersen]

LGTM.