Skip to content

Analyzer: don't infer generic function types as type parameters #33661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MichaelRFairhurst opened this issue Jun 27, 2018 · 7 comments
Closed
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.
Milestone

Comments

@MichaelRFairhurst
Copy link
Contributor

Split out of #33343.

These should be errors:

[<T>(T){}]; // error: couldn't infer type, only candidate has free parameters T and cannot be used (?)
list.map((_) => <T>(T){}); // same error
// other cases...

Doesn't look like this will be the most straightforward thing to add to the inference engine, so I'm pushing this to dart 2.1. That said, hopefully we can get to it by then.

@eernstg @leafpetersen @jmesserly

@MichaelRFairhurst MichaelRFairhurst added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jun 27, 2018
@MichaelRFairhurst MichaelRFairhurst added this to the Dart2.1 milestone Jun 27, 2018
@eernstg
Copy link
Member

eernstg commented Jun 29, 2018

Rather than giving up whenever the inference produces a generic function type which will be used as a type argument, it might work to replace that by Function (which is the least upper bound of any set of generic function types which is admissible as a type argument).

However, the pragmatic aspect should work as well: Would that be a helpful or a confusing behavior? It does document the subtlety of the situation when it's flagged as an inference failure, and developers will then need to fix their code (and write something like Function explicitly), so it's a matter of choosing "more explicit and strict typing" vs. "making it work whenever possible". As usual. ;-)

@natebosch
Copy link
Member

If I understand correctly the proposal is to add a new static error for this in the analyzer (and I would imagine also the CFE since it doesn't currently have one either). I think we need to consider this for Dart2Stable.

cc @dgrove

@eernstg
Copy link
Member

eernstg commented Jul 10, 2018

Edit: I completely misread the function literal <T>(T){} as if it had been <T>(T t){}. Deleted a couple of paragraphs below because they were plain wrong because of that. Thanks, @jmesserly!

tl;dr This demonstrates analyzer bugs, and a CFE bug, but inferring Function is still a possible approach, and the example would then not have any errors. rd;lt

It is correct, as @MichaelRFairhurst says, that this demonstrates bugs in the analyzer: It shouldn't process types that are incorrect (such as those where there is a generic function type which is used as an actual type argument:

main() {
  var list = [<T>(T){}];
  list.isEven;
}

Here is the error message for that (dartanalyzer 2.0.0-dev.67.0):

  error • The getter 'isEven' isn't defined for the class 'List<<T>(dynamic) → Null>' at n002.dart:6:8 • undefined_getter
1 error found.

Assuming that List<<T>(dynamic) → Null> means List<Null Function<T>(dynamic)>, this shows that the declaration of the local variable list gets the inferred type List<Null Function<T>(dynamic)> (which is an error in itself, so the declaration should be rejected as a compile-time error, or inference should choose some other type argument—e.g., List<Function> would work). We can see that this is what happened when that inferred type is reported in the error message for the (correctly rejected) invocation of a non-existing getter, so that's the reason why I have list.isEven there.

[wrong stuff deleted]

Interestingly, dart reports an error :

Unhandled exception:
'file:///usr/local/google/home/eernst/lang/dart/scratch/201807/n002.dart': error: generic function type '<T>(dynamic) => Null' not allowed as type argument of type 'List<<T>(dynamic) => Null>'
#0      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:279:19)
#1      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)

A printout shows that this occurs at the declaration of list (as it should), but it occurs at run time (it should have been a compile-time error).

@jmesserly
Copy link

(which is an error in itself, so the declaration should be rejected as a compile-time error, or inference should choose some other type argument—e.g., List would work). We can see that this is what happened when that inferred type is reported in the error message for the (correctly rejected)

It infers <T>(dynamic) => Null is because the T is a parameter name, not the type. I think the example code was supposed to be like this?

main() {
  var list = [<T>(T t){}];
  list.isEven; // [error] The getter 'isEven' isn't defined for the class 'List<<T>(T) → Null>'. 
}

@leafpetersen
Copy link
Member

leafpetersen commented Jul 10, 2018

We need to have a plan for this. This code:

typedef F = T Function<T>();

main() {
  F x;
  var f = [x];
  print(f);
}

Compiles with no errors on CFE and analyzer. dart2js gives no runtime errors. VM gives a runtime error. This is potentially quite breaking to fix post Dart 2.

@eernstg
Copy link
Member

eernstg commented Jul 11, 2018

@jmesserly wrote:

It infers <T>(dynamic) => Null is because the T is a parameter name, not the type.

Oops, you're right of course! Thanks for catching that one!

However, I still believe that a type which contains a generic function type as an actual type argument should be considered as a compile-time error when written by the developer, and as a type which cannot be inferred (which may or may not cause inference to fail), rather than having such types inferred and propagated around during static analysis.

@MichaelRFairhurst MichaelRFairhurst changed the title Analyzer: it don't infer generic function types as type parameters Analyzer: don't infer generic function types as type parameters Jul 12, 2018
@MichaelRFairhurst
Copy link
Contributor Author

Fixed the extra "it" making the title read like a grammatical error :)

pending CL: https://dart-review.googlesource.com/c/sdk/+/64644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.
Projects
None yet
Development

No branches or pull requests

5 participants