Analyzer doesn't catch that method argument has wrong signature

[Hixie]

The analyzer doesn't catch this static type error:

void foo(bool callback()) { }

class Bar {
  void callFoo() {
    foo(method); // <-- this call is illegal ("method" has the wrong signature)
  }

  void method() { }
}

void main() {
  new Bar().callFoo();
}

Analyzer output:

  lint • Document all public members at test.dart:1:6 • public_member_api_docs
  lint • Document all public members at test.dart:3:7 • public_member_api_docs
  lint • Document all public members at test.dart:4:8 • public_member_api_docs
  lint • Document all public members at test.dart:8:8 • public_member_api_docs

VM output (checked mode):

Unhandled exception:
type '() => void' is not a subtype of type '() => bool' of 'callback'
#0      foo (test.dart:1:15)
#1      Bar.callFoo (test.dart:5:5)
#2      main (test.dart:12:13)
#3      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:265)
#4      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:151)

The analyzer does catch the problem if I make callFoo and method top-level functions or statics on the Bar class. It's only if they're methods that it's a problem.

[leafpetersen]

This is working as intended. Passing method to foo is an implicit downcast. Without additional flags (--no-implicit-cast) strong mode allows implicit downcasts between subtype related types unless it can prove that there is no way for the cast to succeed. If you make method top level, then strong mode knows that the cast will fail and issues an error. But without making closed world assumptions, we cannot know that there is not a subclass of Bar that overrides method at a the type () -> bool, which would allow the cast to succeed.

[Hixie]

Isn't that why we have covariant?

This is extremely confusing (especially that it only applies to methods). I don't see how the code above could ever be correct (it will always throw). Bar isn't abstract. method isn't abstract.

I think we should change our intention here. If someone wants to write code like this they should have to explicitly opt-in the same way we do with accepting subclasses with covariant.

[matanlurey]

I was under the impression it was Flutter that wanted strong-mode-implicit-downcasts disabled by default?

[leafpetersen]

No this isn't related to covariant. Here's an example program where that code won't ever throw:

void foo(bool callback()) { }

class Bar {
  void callFoo() {
    foo(method); // <-- this call is illegal ("method" has the wrong signature)
  }

  void method() { }
}

class Baz extends Bar {
  bool method() { return true; }
}

void main() {
  new Baz().callFoo();
}

Overriding method with something that returns bool is perfectly valid in Dart (it's ok to return something in a context that is just ignoring whatever gets returned).

An the override here means that the implicit downcast in callFoo works at runtime.

If we have a closed world assumption (that is, we assume that we can see all of the classes that will ever be defined all at once), then we can know whether or not method is ever overridden and could issue a warning that the implicit downcast is guaranteed to fail. We don't want to require this at the language level, since we need to support modular tools that can't make closed world assumptions.

--no-implicit-cast would catch this, and it's still on my plate to add a --assignment-cast flag to allow you to make progress with trying to catch more of these statically.

[Hixie]

That would be fine (well, sort of, it seems really dubious to me still) if Bar was abstract, but it's not, so you should be able to use it, and you can't, because it'll throw.

@matanlurey It's a lot more nuanced than that. :-) I think most of the places where we want downcasts are now annotated with covariant. Most of the remaining places are assignments and on the long run we'll move all of those to using as, I think. We don't use as today because we don't want the type check in release builds and assignment lets us get around that.

What I want personally is for the language to be intuitive, and I feel that the case here is a great example of unintuitive behavior.

[leafpetersen]

What I want personally is for the language to be intuitive, and I feel that the case here is a great example of unintuitive behavior.

Agreed. My personal preference is to avoid all implicit downcasts, but I'm very open to identifying specific subsets of them that are especially pernicious. Do you have something systematic in mind here? Implicit downcasts of callbacks? Or just making the strong mode "impossible cast" warning fire for tearoffs as well, even though those aren't actually truly impossible casts?

cc @floitschG @lrhn @eernstg @munificent

[munificent]

I'm not totally clear on what behavior you want to have. I see a couple of options for your example:

1. Don't report an error statically, and don't report an error at runtime.

I don't think this is feasible because it violates soundness. Consider:

void foo(bool callback()) {
  bool b = callback(); // <--
}

class Bar {
  void callFoo() {
    foo(method);
  }

  void method() { }
}

class SubBar extends Bar {
  int method() => 123;
}

void main() {
  new SubBar().callFoo();
}

On the marked line, b would end up with value 123. That's not good.

2. Don't report an error statically, but check it at runtime.

This is what we do now. As Leaf noted, there are some ways the code could potentially be valid where the runtime check will succeed. Personally, I don't like that the static checker assumes this will succeed. I'd rather you get an error and require an explicit cast. I think you feel the same way?

3. Report an error statically.

If we didn't do the implicit downcast, then you get an error here:

void foo(bool callback()) { }

class Bar {
  void callFoo() {
    foo(method); // Can't assign a value of type () -> void to a parameter of type () -> bool.
  }

  void method() { }
}

You could fix the error by doing a manual downcast:

void foo(bool callback()) { }

class Bar {
  void callFoo() {
    foo(method as bool Function());
  }

  void method() { }
}

Is that what you have in mind?

That would be fine (well, sort of, it seems really dubious to me still) if Bar was abstract, but it's not, so you should be able to use it, and you can't, because it'll throw.

I don't follow how abstract affects things. Can you walk me through it?

[Hixie]

@munificent #3 is what I had in mind for this particular case. Abstractness matters only insofar as if the class was abstract, then it's possible the contract would be for the subclass to always override it? But even then it'd be pretty weird.

@leafpetersen I would approach the problem the other way. Downcasts bad, except for specific cases. One of those cases is when an argument is marked as covariant. Another is in the as expression. What are other cases?

[lrhn]

The problem comes from the fact that bool Function() is assignable to void Function().
Because of that, you can write.

bool foo() => true;
void Function() bar = foo;  // allowed
bool Function() baz = bar;  // implicit down-cast that *will* succeed at runtime.

The potentially unsafe down-cast is where strong mode puts the runtime-check.

Now, an analyzer can be smarter than that. Leaf showed an example where the code works, but if the analyzer has global knowledge, it can know that no subclass of Bar overrides method to something that actually works.
The language allows the implicit down-cast because, well, it has implicit down-casts. They are very convenient in many cases and we like them. They do hide some type-errors, but removing implicit down-cast would make you need casts, which duplicates the type, for cases like:

List<String> list = iterable as List<String>;