Strong mode gives warning about { return 1 } but not => 1

[Hixie]

typedef int Callback();
Callback c;
void main() {
  c = () => 1; // no warning 
  c = () { return 1; }; // warning
}

gives this message:

[warning] () {return 1;} (() → dynamic) will need runtime check to cast to type () → int (.../test.dart, line 5, col 7)

Note how this only applies to one of the lines, not both.

[sethladd]

"will need runtime check to cast to type" sounds like a warning that presumes the user will run DDC (I could be wrong?) Can we make that an optional piece of feedback, for users that want strong mode but don't expect to compile with DDC? Thanks!

[sethladd]

cc @vsmenon @leafpetersen

[leafpetersen]

The warning is an instance of dart-archive/dev_compiler#308 . Basically we're successfully inferring the return type of the first closure but not the second (because we don' t handle general statement bodies yet).

In the general case, that warning is a sign that something could be wrong, but it could also be a false positive. Sorting out what things should be infos/warnings/errors is on our radar. I'd like to avoid having a proliferation of options and warnings, but a few well designed modes of use in which certain categories of warnings are suppressed might be appropriate.

[sethladd]

Thanks for the quick response!

I bring up configurability because not all warnings apply in all cases. We're grepping out warnings that don't make sense on the VM, for example.

In any case, the feedback is hard to understand for me.

"will need" -- when? Now?

"to cast" -- what if I never cast?

I'm used to feedback/warnings/hints that apply right on the line where the feedback is reported. This message, though, sounds like more of a "head's up, doing a cast later will need a runtime check". Giving a warning about what happens later is interesting, because what if I never case later? How do I silence this now and here?

[bwilkerson]

Basically we're successfully inferring the return type of the first closure but not the second (because we don' t handle general statement bodies yet).

Are you planning on inferring block function bodies?

I might be wrong, but I don't think you're inferring the return type of the first closure either. The specification explicitly defines the return type of closures with an expression body, so no inference is needed.

[leafpetersen]

@sethladd Ok, so three things there.

First is that the message is clearly unclear. :) From the DDC standpoint, that message is (supposed to be) precisely targeted. It is telling you that DDC will insert a runtime cast at that exact assignment to c. In general, that kind of cast (function type to function type) tends to fail, and in that specific case, it will always fail - the reified runtime type will be different from the target type of the cast. So we should make that message clearer - in general, we are painfully aware that our error messages need improvement.

The second is that if you're using strong mode independently of DDC (which is absolutely something I'd like to encourage), that message is not phrased in relevant way. If you care about the warning at all, it's because strong mode has found a potential type error - the fact that DDC would have inserted a cast to protect against the type error at runtime is irrelevant and confusing. So we need to either phrase the message in a DDC agnostic way, issue a different error message when we're compiling with DDC, or split this into two warnings (which seems overly verbose).

The third is that if you're using strong mode independently of DDC (essentially using it as a linter), you may not wish to see downcast warnings at all (even though they might signal a bug). For this we would need a flag. This is probably reasonable, though given our current fairly liberal policy on inserting implicit casts, this may reduce the utility of strong mode as a linter a fair bit.

[leafpetersen]

@bwilkerson You are correct that the spec (and hence the analyzer) already infers the correct return type for this simple example. For more complicated code, strong mode does more inference here than the spec demands/allows. For example, in this code:

typedef List<int> Callback();
Callback c;
void main() {
  c = () => new List();
}

strong mode will infer the type argument for the call to the List constructor, and infer that the return type of the closure is List<int> instead of just List.

I do plan to implement this for statement bodies. Note though that the inference here is downwards, not upwards. If the type of c was dynamic, no inference would be done - I'm not looking at the body of the function and figuring out its type, but rather am asking the question "can this function body be typed at the type which is required by the context of occurrence".

[bwilkerson]

Right! I had forgotten about downward inference. Thanks!

[Hixie]

FWIW I would want all warnings that might indicate a bug, I just wish I could have a way to say (on an expression-by-expression basis) that in this instance the analyzer is wrong, and it's not a bug.

This bug, though, was specifically about the fact that the two lines of code which are semantically identical don't both get the message (or both not get the message). As far as I can tell, usage of => vs {} should just be a syntactic choice, not a semantic one.

The issue about this warning not being useful for non-DDC users is #24542 .

[bwilkerson]

As far as I can tell, usage of => vs {} should just be a syntactic choice, not a semantic one.

For what it's worth, the specification actually treats them somewhat differently. While the run-time semantics of => is expressed in terms of {}, the spec defines the runtime type of a closure differently depending on the syntactic form. I think that's largely to simplify the specification.

I think it could, for example, define the return type of a block-bodied closure to be the least-upper-bound of the types of the expressions in return statements (probably with some special handling of return statements without expressions and return statements whose expression is a literal null).

[jmesserly]

This was fixed by Leaf's downwards inference