Skip to content
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

Completer doesn't respect generic type... #20108

Closed
DartBot opened this issue Jul 18, 2014 · 9 comments
Closed

Completer doesn't respect generic type... #20108

DartBot opened this issue Jul 18, 2014 · 9 comments
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer closed-obsolete Closed as the reported issue is no longer relevant library-async P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Jul 18, 2014

This issue was originally filed by adrian.avil...@gmail.com


.complete<T> should be match Completer<T> and disallow any other type.

What steps will reproduce the problem?

Completer<String> pCompleter = new Completer<String>();
pCompleter.complete(123); //<-- This should be an error but the compiler permits it.

What is the expected output? What do you see instead?

An error from the compiler

What version of the product are you using?

Dart 1.5.3

On what operating system?

Windows 8.1

@floitschG
Copy link
Contributor

Added Area-Library, Library-Async, Triaged labels.

@lrhn
Copy link
Member

lrhn commented Jul 18, 2014

The compiler, sadly, has to accept it.
The "complete" function can take both a "T" argument and a "Future<T>" argument, so it can't statically have any other type than "dynamic".

If it also gets through at runtime, in checked mode, then we should add an extra type assertion somewhere.

@DartBot
Copy link
Author

DartBot commented Jul 20, 2014

This comment was originally written by @Emasoft


I don't understand why it cannot be done.
Just implement a multiple types check, like this (in pseudocode):

if(generic_class is instanced for type T)
   if(object is T || object is Future<T>)
   then compile
   else rise Error "generic_class can accept only " + typeof(T) + " or Future<" + typeof(T) + "> objects"

?

@lrhn
Copy link
Member

lrhn commented Jul 20, 2014

The problem is that the signature of Completer<T>.complete is "dynamic->void".

There is nothing in that type that allows the compiler to reject an integer argument, and nothing in the specification that special-cases completers, so the analyzer is not allowed to give a warning (warnings are restricted to the warnings specified by the language specification).

The compiler cannot stop you from doing this, because there is no way to tell the compiler that it is wrong.

If you run the code in checked mode, it will fail with a TypeError.

The best thing we can do is to let the analyzer have a special case for completers, so that it can give a hint if you pass a non-S and non-Future<S> to Completer<S>.complete.


Removed Type-Defect, Area-Library labels.
Added Type-Enhancement, Area-Analyzer labels.

@DartBot
Copy link
Author

DartBot commented Jul 20, 2014

This comment was originally written by @Emasoft


If it can give an hint, it can also give an error, right? And it should do, so it will spare us from a billion bugs just waiting to happen.

@bwilkerson
Copy link
Member

If it can give an hint, it can also give an error, right?

We make a strong distinction between the words "error", "warning" and "hint". Probably stronger than you care about. So, yes, we can produce an error (in the generic sense) for this case.


Removed Priority-Unassigned label.
Added Priority-Medium, Analyzer-Hint labels.

@DartBot
Copy link
Author

DartBot commented Jul 31, 2014

This comment was originally written by @Emasoft


Then you should.
This is a serious issue, that can make or break the adoption of Dart in large projects. Allowing an int or any other type to be added to a string list without throwing an ERROR is a recipe for a disaster. At the very least an explicit cast should be required.

Related to:
https://code.google.com/p/dart/issues/detail?id=20102
https://code.google.com/p/dart/issues/detail?id=20297

@DartBot
Copy link
Author

DartBot commented Nov 15, 2014

This comment was originally written by adrian.avil...@gmail.com


Honestly, I simple warning would be nice.

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Mar 1, 2016
@matanlurey matanlurey added the closed-obsolete Closed as the reported issue is no longer relevant label Feb 17, 2018
@matanlurey
Copy link
Contributor

This is fixed by Dart2+FutureOr<T>! Hooray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer closed-obsolete Closed as the reported issue is no longer relevant library-async P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants