Will need runtime check to cast to type (num)

[zoechi]

With

dartanalyzer --strong --lints web/index.dart

[warning] _nextProgress (([dynamic]) → void) will need runtime check to cast to type (num) → void (/home/dart/dart-lang/polymer_elements/demo/web/paper_progress/paper_progress_demo.dart, line 50, col 38)

for this function

void _nextProgress([_]) {
    _animating = true;
    if (progressValue < progress.max) {
      set('progressValue', progressValue + (progress.step ?? 1));
    } else {
      if (++_repeat >= _maxRepeat) {
        _animating = false;
        set('buttonDisabled', false);
        return;
      }
      set('progressValue', progress.min);
    }

    dom.window.requestAnimationFrame(_nextProgress); // <== line 50
  }

in dart:html

typedef void RequestAnimationFrameCallback(num highResTime);
int requestAnimationFrame(RequestAnimationFrameCallback callback) {

With _ I indicate that I don't use and don't intend to use the parameter. I also don't see why this should indicate a problem when the passed callback allows a more generic type then what the receiver requests.

Dart VM version: 1.13.0-edge.33b037125fbf5e8319a99fd5e5700441c921efde (Mon Sep 14 18:13:24 2015) on "linux_x64"

[jmesserly]

Simplified example:

typedef void RequestAnimationFrameCallback(num highResTime);
int requestAnimationFrame(RequestAnimationFrameCallback callback) => 0;

void nextProgress(_) {
  //...

  requestAnimationFrame((x) {}); // no warning
  requestAnimationFrame(nextProgress); // warning
}

that's really strange. I don't know why the two cases are treated differently.

[vsmenon]

@jmesserly The first example is handled via inference.

I believe the second / main example would type check if _ was explicitly typed as Object. That should not be necessary though. I think this is a bug in how we're handling dynamic. The static type of nextProgress should be top -> void.

@leafpetersen

[jmesserly]

I don't think we infer parameter types of lambdas? dart-archive/dev_compiler#203

[jmesserly]

but yeah, it does sound like a top -> void vs bottom -> void issue. I just don't see why a local function is different from a top level function.

[leafpetersen]

This is a limitation of our integration with the analyzer (at least so far). Our "fuzzy arrows", which are what allow limited use of "is" and "as" checks on function types, require us to treat unknown functions (function typed variables, tearoffs, etc) which have type dynamic -> _ as really being bottom -> _ (I'm just going to talk about the unary function case here, and ignore the return types, since this is all about argument types). This induces an implied caller side check on the argument when such a function is called. So if I have:

  typedef int Callback(dynamic x);
  void foo(int f(int x)) {...};
  void main() {
     Callback f = ....;
     f(3);
     foo(f);
  }

There's an implied dcall at the call site for f, since we will allow any unary function to be assigned to f. There also has to be an implied downcast at the call to foo, since foo expects an int -> int, and will not do the required caller side check when it calls f. So we must downcast f to see if it's really an int -> int before passing it off to foo. So far so good.

Now, in the original example above, nextProgress is not a function typed variable, but an actual toplevel function. We could (and should) treat an actual function valued variable of type dynamic -> _ as actually having type dynamic -> _, rather than bottom -> _. There's no point in subsuming it into the more general bottom -> _ type before we need to. By keeping it as dynamic -> _, we can allow code like the nextProgress example above. So concretely, if I had written the example above as:

  typedef int Callback(dynamic x);
  void foo(int f(int x)) {...};
  void main() {
     int f(dynamic x) => 3;
     f(3);
     foo(f);
  }

then there should be neither an implied dcall at the call to f, nor an implied downcast at the call to foo.

This requires us to actually distinguish between the types dynamic -> _ and bottom -> _ though. In our current implementation, we just treat all uses of dynamic -> _ as bottom -> _. I think we can implement this without changing the analyzer data structures, but I think it requires us to go through very early on and rewrite the appropriate occurrences of dynamic -> _ to bottom -> _, while keeping things like the type of f in the second example as true dynamic -> _ (not rewriting to bottom).

[jmesserly]

yeah, that makes sense. Question about:

then there should be neither an implied dcall at the call to f, nor an implied downcast at the call to foo.

Consider this example:

  typedef int Callback(dynamic x);
  void foo(int f(int x)) {...};
  void main() {
     ((dynamic x) => 3)(3);
     foo((dynamic x) => 3);
  }

It's now an anonymous function. Shouldn't it behave exactly like a named local function? In other words, there should neither be an implied dcall on the first line, nor an implied downcast at the call to foo.

[leafpetersen]

Yes, that's right. Basically, we want to use the actual precise type (dynamic -> _) for function values, because we statically know that the function that the expression evaluates to typechecks at dynamic -> _. This extends to function typed variables as well, because they are immediately bound to a concrete function value of the appropriate type, and are not mutable. But for arbitrary function typed locations (variables, fields, methods) this doesn't hold.

There's a related issue with method override that we need to settle out as well. If you have a method of type dynamic -> _ in a super class, there is an implied caller side check any time someone calls the method at the super class type. So we could allow that method to be overridden "covariantly" in the subclass. That is, we could allow num -> _ in the subclass. Essentially this boils down to doing the override check against bottom -> _ instead of dynamic -> _. The tradeoff is that this disallows contravariantly overriding a method with dynamic (though you can still use Object for that). Concretely, if we went this route:

class A {
   void f(dynamic x) => ...
}

class B extends A {
  // valid override
  void f(int x) => ...
}

class C extends B {
  // invalid since int </: bottom
  void f(dynamic x) => ....
}

class D extends B {
  // Ok
  void f(Object x) => ...
}

We could choose to allow the override in C, but it would mean treating methods with dynamic argument types non-uniformly depending on their override context. I really think that's getting way too complex for a nice usable type system though.

[jmesserly]

There's a related issue with method override that we need to settle out as well. If you have a method of type dynamic -> _ in a super class, there is an implied caller side check any time someone calls the method at the super class type.

Another option might be to do the check callee side? I think a lot of folks tend to use dynamic more like top, and exchange it rather freely with Object. In the derived class, the intent is essentially a runtime check:

class Base {
  method(Object obj) {
  }
}
class Derived {
  // this method can't actually be called with anything other than Derived.
  // method(Derived obj) 
  method(Object obj) {
    var d = obj as Derived;
    ...
  }
}

That does mean Derived.method always has the covariant check, but Base.method no longer needs any checks. Also it matches how we handle generics.

[leafpetersen]

Stepping back, the rationale behind introducing the fuzzy arrows was that they are valid for use in is checks: since all functions of the appropriate arity are subtypes of them, the is check just serves as an arity/callability check, which seems to be the most common use case. They also allow for a few other use cases where you have a function typed parameter to which you want to be able to pass a function of any type.

In order to support this without wrapping, we need to do the checks on the caller side since any function can be passed into a context expecting a fuzzy arrow (or else we need to do callee side checks all function parameters, which is one of the things we're trying to avoid).

Now, we could do choose not to use fuzzy arrows for methods at all, only for functions. I think that would be fine (again, assuming that we reify fuzzy and non-fuzzy arrows appropriately during an early analyzer pass).

We could also choose to support covariant overrides for methods, using callee side checks (I think this is what you are suggesting above)? This could be done either for any type, or only for overrides of Object. I'd kind of prefer not to go there though.

[jmesserly]

We could also choose to support covariant overrides for methods, using callee side checks (I think this is what you are suggesting above)?

Ah, I was only suggesting it for dynamic. If I understood you correctly, we already support that case?

So concretely: I don't think we are marking (all?) methods with dynamic parameters as needing dsend right now. Is that a bug?

Also, does this imply that Object.== would need dsend?

sdk/sdk/lib/core/object.dart

Line 59 in e928812

bool operator ==(other) => identical(this, other);

... it takes a dynamic parameter. I'm pretty sure we aren't checking that in our == implementation. Do we want to? That seems a high cost, when the most common thing to do is to override it with either Object or dynamic. (overriding it with anything else isn't really correct anyway...)

[leafpetersen]

Yes, given our current override behavior, there is an implied dsend on all methods with dynamic argument types. I think I'm increasingly of the opinion though that at least for methods we ought to leave dynamic as top rather than bottom. We can still change it to bottom when do a tear-off, so we would get consistent behavior with functions. This does mean that we don't support the "pseudo-covariant" override pattern of overriding a method taking dynamic with one taking something concrete. I kind of feel thought that if we want to support that, we may want to have a different mechanism for it (e.g. explicitly marking the parameter position as covariant, with the resulting implied check).

[jmesserly]

+1 to all of that

[vsmenon]

Has this been fixed? I don't see a warning on @jmesserly 's example above.

[jmesserly]

I still see a warning:

typedef void RequestAnimationFrameCallback(num highResTime);
int requestAnimationFrame(RequestAnimationFrameCallback callback) => 0;

void nextProgress(_) {
  //...
  requestAnimationFrame((x) {}); // no warning

  // [warning] nextProgress ((dynamic) → void) will need runtime check to cast to type (num) → void
  requestAnimationFrame(nextProgress); // warning
}

[jmesserly]

@vsmenon had another example:

void foo(x) {
  print(x);
}

void bar(void f(int x), int x) {
  f(x);
}

void main() {
  bar(foo, 42);
}

[jmesserly]

The behavior is actually even worse now in some cases -- we will issue an "error".

[jmesserly]

Can confirm that we never set fuzzyArrows to false in the current code. Chatted with Leaf, I'll take a shot at using fuzzyArrows:false from CodeChecker

[jmesserly]

fix sent out for review: https://codereview.chromium.org/1700523002/

[zoechi]

Lots of progress 👍