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 2b95ed1, 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();
}

[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 ***@***.***> 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 <#21938 (comment)>, 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 #21938 (comment), 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 e8091b2:

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 #21938 (comment) behaviour is correct?

[lrhn]

Looks good to me.

[leafpetersen]

LGTM.