analyzer does not report an error on incorrect code in strong mode

[mraleph]

typedef T RecognizerCallback<T>();

class Test {
  T invokeCallback<T>(RecognizerCallback<T> callback) {
    return callback();
  }

  void callback() {
  }

  void test() {
    invokeCallback<String>(callback);
    invokeCallback<Null>(callback);
  }
}

╭─ ~/src/dart/sdk ‹master›
╰─$ sdk/bin/dartanalyzer --strong -v t/analysis.dart                                                                                3 ↵
Analyzing t/analysis.dart...
Using default analysis options
No issues found!

If I now move the code to top-level scope then analyzer reports an error

typedef T RecognizerCallback<T>();

  T invokeCallback<T>(RecognizerCallback<T> callback) {
    return callback();
  }

  void callback() {
  }

  void test() {
    invokeCallback<String>(callback);
    invokeCallback<Null>(callback);
  }

╭─ ~/src/dart/sdk ‹master›
╰─$ sdk/bin/dartanalyzer --strong -v t/analysis.dart
Analyzing t/analysis.dart...
Using default analysis options
  error • The function 'callback' has type '() → void' that isn't of expected type '() → String'. This means its parameter or return type does not match what is expected at t/analysis.dart:13:28 • strong_mode_invalid_cast_function
  error • The function 'callback' has type '() → void' that isn't of expected type '() → Null'. This means its parameter or return type does not match what is expected at t/analysis.dart:14:26 • strong_mode_invalid_cast_function
1 error found.

[Flutter is full of this sort of mistyped code (except they use Null instead of String) , so once this is fixed we would heed to do a pass over their code and fix it /cc @leafpetersen ]

@devoncarew could you help triage this? Thanks.

[vsmenon]

I'm guessing that is a bug with instance-method (e.g., callback here) handling.

[vsmenon]

Commenting out the isStatic check here:

https://github.com/dart-lang/sdk/blob/master/pkg/analyzer/lib/src/task/strong/checker.dart#L1214

does cause the error to be reported in Slava's first example. Not sure that is quite the right fix though.

[peter-ahe-google]

@mraleph has a workaround: Fasta produces the errors he actually need to fix.

[stereotype441]

I added this to our analyzer tracking bug (#31638) so we won't forget to work on it

[leafpetersen]

The strong mode semantics of that for both cases is an implicit downcast. That implicit downcast will in this case fail at runtime, but it is only in the toplevel case that the analyzer can prove that the downcast will definitely fail at runtime. In those cases, strong mode turns the downcast into a static error.

[vsmenon]

So the question is if Slava's original example should a static error or a runtime error?

[mraleph]

@leafpetersen ah, implicit down casts. of course - () -> void is a super type of any () -> T.

FWIW this is extremely confusing and there are >30 incorrectly typed call-sites in Flutter due to this. Fixing this one call-site at a time would take us quite a bit of time.

[vsmenon]

FYI, both DDC and DDK pass the following test:

import 'package:expect/expect.dart';

typedef T RecognizerCallback<T>();

class Test {
  T invokeCallback<T>(RecognizerCallback<T> callback) {
    return callback();
  }

  void callback() {
  }

  void test() {
    invokeCallback<String>(callback);
    invokeCallback<Null>(callback);
  }
}

class Test2 extends Test {
  Null callback() {
  }
}

void test(Test t) {
  t.test();
}

void main() {
  test(new Test2()); // Fine
  Expect.throws(() => test(new Test())); // Runtime error
}

Not sure there is anything actionable right now.

@mraleph - there is --no-implicit-casts ... and the argument that should be the default. :-)

[leafpetersen]

To expand a bit:

• Both examples will fail with a runtime error
• Strong mode disallows downcasts that are statically known to fail
  • For the instance method example, there could be an override, so it is not statically known to fail
  • For the top level example, it will definitely fail
  • I'm not 100% sure we had agreement to pull this "stupid cast" rule over strong mode from Dart 2.0, there was some opposition on the language team (@lrhn ?)
  • If we don't pull this over, it will almost certainly become a hint or a lint

@mraleph

FWIW this is extremely confusing and there are >30 incorrectly typed call-sites in Flutter due to this. Fixing this one call-site at a time would take us quite a bit of time.

Well, this is precisely what we're all signed up for in getting flutter working with the runtime checks enabled. This is why I've been banging that drum... :) But in any case, there's really no way around this - making it not a static error is ok, but that just pushes the error to runtime, and we really do need the runtime error (a void returning function can return any value, including non String values).

My guess is that this is a case where the lack of void as a type argument was hindering the ability to accurately type the code. Otherwise, I don't know why they'd be passing a void returning function to a context expecting a String returning function. I'd have to see the actual code to be sure though. If that is the case, then fixing this should be easy: just change the type argument to invokeCallback to void.

I added support to the analyzer to run with --no-implicit-casts --assignment-casts in an attempt to get flutter to be tighter with implicit downcasts, but this was still a bit too coarse to fly at the time. We could try to revisit this with @Hixie , but if nothing else this might be a useful way to audit the code for likely runtime failure locations.

[leafpetersen]

A quick test suggest that the analyzer is supporting void as a type argument now, @devoncarew can you confirm? If so, and given that the roll of flutter to the 2.0 SDK seems to be sticking, we could try to start migrating them to using it. I have negative bandwidth to spend until after DartConf, but could try to take a pass after that.

[mraleph]

@leafpetersen we can't migrate Flutter to use void as a type argument yet because legacy VM parser does not support it. We are bumping priority of this (#30516).

I don't know why they'd be passing a void returning function to a context expecting a String returning function.

They use Null there, not String. I just added string to the test.

I am closing an issue because there is no bug in the analyzer (and both DDC and VM on CFE correctly report a runtime error).