Allow 'void' as a type argument (fixes #28945).

Allow 'void' as a type argument (fixes #28945).

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/bd39b941c52ba9c603934feee1ec7fbd4a9239b3

[regis, 2017-05-31 00:15:38]

regis@google.com changed reviewers:
+ asiva@google.com, floitsch@google.com

[regis, 2017-05-31 00:15:39]

This cl allows void as a type argument, but patch set #1 is syntax only and I
need to know how to properly handle void in type tests (non-strong mode for
now). Do we have a spec or at least a suite of tests setting expectations?

[floitsch, 2017-05-31 16:14:42]

On 2017/05/31 00:15:39, regis wrote:
> This cl allows void as a type argument, but patch set #1 is syntax only and I
> need to know how to properly handle void in type tests (non-strong mode for
> now). Do we have a spec or at least a suite of tests setting expectations?

The dart2js CL is here:
https://codereview.chromium.org/2699073003/

However, it has many more tests for allowing `void` as a local and parameter as
well.
(`Future<void>.then` kind of wants a function that has a parameter that takes
`void`, which is why I implemented both at the same time).

The parser changes are probably good. The patch for dart2js is very similar.
It's probably a good idea to go through all the dart2js parser changes to see if
they would also apply to the VM's parser. The parsers are not exactly the same,
but in many cases they try to do similar things.
(fyi: this patch set also made `void` an identifier, which explains some of the
changes)

Thanks for working on this!

[regis, 2017-05-31 16:20:37]

On 2017/05/31 16:14:42, floitsch wrote:
> On 2017/05/31 00:15:39, regis wrote:
> > This cl allows void as a type argument, but patch set #1 is syntax only and
I
> > need to know how to properly handle void in type tests (non-strong mode for
> > now). Do we have a spec or at least a suite of tests setting expectations?
> 
> The dart2js CL is here:
> https://codereview.chromium.org/2699073003/
> 
> However, it has many more tests for allowing `void` as a local and parameter
as
> well.
> (`Future<void>.then` kind of wants a function that has a parameter that takes
> `void`, which is why I implemented both at the same time).
> 
> The parser changes are probably good. The patch for dart2js is very similar.
> It's probably a good idea to go through all the dart2js parser changes to see
if
> they would also apply to the VM's parser. The parsers are not exactly the
same,
> but in many cases they try to do similar things.
> (fyi: this patch set also made `void` an identifier, which explains some of
the
> changes)
> 
> Thanks for working on this!

Thanks for the pointer to the dart2js changes, but I'll rather wait for an
informal spec by Erik. He promised one soon.

[floitsch, 2017-05-31 16:24:31]

On 2017/05/31 16:14:42, floitsch wrote:
> On 2017/05/31 00:15:39, regis wrote:
> > This cl allows void as a type argument, but patch set #1 is syntax only and
I
> > need to know how to properly handle void in type tests (non-strong mode for
> > now). Do we have a spec or at least a suite of tests setting expectations?
> 
> The dart2js CL is here:
> https://codereview.chromium.org/2699073003/
> 
> However, it has many more tests for allowing `void` as a local and parameter
as
> well.
> (`Future<void>.then` kind of wants a function that has a parameter that takes
> `void`, which is why I implemented both at the same time).
> 
> The parser changes are probably good. The patch for dart2js is very similar.
> It's probably a good idea to go through all the dart2js parser changes to see
if
> they would also apply to the VM's parser. The parsers are not exactly the
same,
> but in many cases they try to do similar things.
> (fyi: this patch set also made `void` an identifier, which explains some of
the
> changes)
> 
> Thanks for working on this!

Forgot to answer your question...
`void` as a generic argument should be treated like `Object` dynamically.

The only difference would be for return-types of functions (but that actually
falls into place with the current rules).

```
class A<T> {
  void bar() {}
  T foo() => null;
}

print(new A().bar is Object Function());  // false, by our current rules.
print(new A<void>().foo is Object Function());  // should print false, too.
```

Otherwise, we would need to remember, where a `void` comes from, which seems to
be confusing and harder to implement.

[regis, 2017-05-31 16:44:21]

On 2017/05/31 16:24:31, floitsch wrote:
> On 2017/05/31 16:14:42, floitsch wrote:
> > On 2017/05/31 00:15:39, regis wrote:
> > > This cl allows void as a type argument, but patch set #1 is syntax only
and
> I
> > > need to know how to properly handle void in type tests (non-strong mode
for
> > > now). Do we have a spec or at least a suite of tests setting expectations?
> > 
> > The dart2js CL is here:
> > https://codereview.chromium.org/2699073003/
> > 
> > However, it has many more tests for allowing `void` as a local and parameter
> as
> > well.
> > (`Future<void>.then` kind of wants a function that has a parameter that
takes
> > `void`, which is why I implemented both at the same time).
> > 
> > The parser changes are probably good. The patch for dart2js is very similar.
> > It's probably a good idea to go through all the dart2js parser changes to
see
> if
> > they would also apply to the VM's parser. The parsers are not exactly the
> same,
> > but in many cases they try to do similar things.
> > (fyi: this patch set also made `void` an identifier, which explains some of
> the
> > changes)
> > 
> > Thanks for working on this!
> 
> Forgot to answer your question...
> `void` as a generic argument should be treated like `Object` dynamically.
> 
> The only difference would be for return-types of functions (but that actually
> falls into place with the current rules).
> 
> ```
> class A<T> {
>   void bar() {}
>   T foo() => null;
> }
> 
> print(new A().bar is Object Function());  // false, by our current rules.
> print(new A<void>().foo is Object Function());  // should print false, too.
> ```
> 
> Otherwise, we would need to remember, where a `void` comes from, which seems
to
> be confusing and harder to implement.

Thanks!

[siva, 2017-06-02 15:56:54]

Is this CL ready for review Regis?

[regis, 2017-06-02 16:01:08]

On 2017/06/02 15:56:54, siva wrote:
> Is this CL ready for review Regis?

Hi Siva. I did not work on it since I sent it. Let me consider Florian's points
today and I'll update it for review.

[regis, 2017-06-02 17:15:14]

On 2017/06/02 16:01:08, regis wrote:
> On 2017/06/02 15:56:54, siva wrote:
> > Is this CL ready for review Regis?
> 
> Hi Siva. I did not work on it since I sent it. Let me consider Florian's
points
> today and I'll update it for review.

Siva,
The current patch already behaves as Florian expects.
I checked with one more case 'baz', which prints true:
```
class A<T> {
  void bar() {}
  T foo() => null;
  baz(T x) {}
}

print(new A().bar is Object Function());  // prints false.
print(new A<void>().foo is Object Function());  // prints false.
print(new A<void>().baz is Function(Object));  // prints true.
```

For good measure, I nevertheless added a couple of checks for void that may
occur after an uninstantiated type gets instantiated to void.

So I think we can commit this cl now and fix issues later as they are unveiled
by more tests or real world programs.

Thanks,
Regis

[siva, 2017-06-05 18:08:13]

lgtm

[regis, 2017-06-05 18:11:32]

Description was changed from

==========
Allow 'void' as a type argument (fixes #28945).
==========

to

==========
Allow 'void' as a type argument (fixes #28945).

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bd39b941c52ba9c603934feee1ec7fbd4a9239b3
==========

[regis, 2017-06-05 18:11:33]

Committed patchset #2 (id:20001) manually as
bd39b941c52ba9c603934feee1ec7fbd4a9239b3 (presubmit successful).