Don't use => on void functions.

Don't use => on void functions.

Fixes #26740
BUG= http://dartbug.com/26740
R=sra@google.com

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

[floitsch, 2016-07-08 16:14:36]

floitsch@google.com changed reviewers:
+ alanknight@google.com

[floitsch, 2016-07-08 16:14:37]

First part of a fix for http://dartbug.com/26740.

Please add a test and then land it (but I can land the fix already now if you
wish).

[floitsch, 2016-07-08 16:15:03]

Description was changed from

==========
Don't use => on void functions.

Fixes #26740
BUG= http://26740
==========

to

==========
Don't use => on void functions.

Fixes #26740
BUG= http://dartbug.com/26740
==========

[Alan Knight, 2016-07-08 16:35:50]

This seems reasonable enough by itself, but when did this become an error? There
are dozens of others occurrences in html_dartium.dart and I assume these haven't
changed in a long time.

This method looks like it is exercised in tests/html/audiocontext_test.dart,
which does not appear to be suppressed.

[floitsch, 2016-07-08 16:37:50]

On 2016/07/08 16:35:50, Alan Knight wrote:
> This seems reasonable enough by itself, but when did this become an error?
There
> are dozens of others occurrences in html_dartium.dart and I assume these
haven't
> changed in a long time.
> 
> This method looks like it is exercised in tests/html/audiocontext_test.dart,
> which does not appear to be suppressed.

This was always the case. A function marked as 'void' must not return anything
else than 'null' (in checked mode).

What is missing is a test-case that tries to run a non-null through the void (as
in the bug-report).

[floitsch, 2016-07-08 18:02:32]

https://codereview.chromium.org/2118633002/diff/1/sdk/lib/web_audio/dart2js/w...
File sdk/lib/web_audio/dart2js/web_audio_dart2js.dart (right):

https://codereview.chromium.org/2118633002/diff/1/sdk/lib/web_audio/dart2js/w...
sdk/lib/web_audio/dart2js/web_audio_dart2js.dart:479: void _connect(destination,
[int output, int input]) native;
Fwiw: the "_connect" return type is probably not correct anymore, since the
value here is not 'null'.

Probably should be changed here to the correct type.
We are just lucky that dart2js doesn't check the return type for native
functions.

[sra1, 2016-07-08 18:12:03]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2016-07-08 18:12:03]

lgtm

https://codereview.chromium.org/2118633002/diff/1/sdk/lib/web_audio/dart2js/w...
File sdk/lib/web_audio/dart2js/web_audio_dart2js.dart (right):

https://codereview.chromium.org/2118633002/diff/1/sdk/lib/web_audio/dart2js/w...
sdk/lib/web_audio/dart2js/web_audio_dart2js.dart:479: void _connect(destination,
[int output, int input]) native;
On 2016/07/08 18:02:32, floitsch wrote:
> Fwiw: the "_connect" return type is probably not correct anymore, since the
> value here is not 'null'.
> 
> Probably should be changed here to the correct type.
> We are just lucky that dart2js doesn't check the return type for native
> functions.

The return type of native functions is assumed to be correct.

Change this to something that is correct (e.g. dynamic), then the => would have
been a warning.

[floitsch, 2016-07-11 12:20:04]

https://codereview.chromium.org/2118633002/diff/1/sdk/lib/web_audio/dart2js/w...
File sdk/lib/web_audio/dart2js/web_audio_dart2js.dart (right):

https://codereview.chromium.org/2118633002/diff/1/sdk/lib/web_audio/dart2js/w...
sdk/lib/web_audio/dart2js/web_audio_dart2js.dart:479: void _connect(destination,
[int output, int input]) native;
On 2016/07/08 18:12:03, sra1 wrote:
> On 2016/07/08 18:02:32, floitsch wrote:
> > Fwiw: the "_connect" return type is probably not correct anymore, since the
> > value here is not 'null'.
> > 
> > Probably should be changed here to the correct type.
> > We are just lucky that dart2js doesn't check the return type for native
> > functions.
> 
> The return type of native functions is assumed to be correct.
> 
> Change this to something that is correct (e.g. dynamic), then the => would
have
> been a warning.

I don't know how to change this.
This part seems to be generated. I'm guessing it is extracted from the DOM
specification which has changed.
This probably was correct for older versions of Chrome, but isn't anymore.

[floitsch, 2016-07-11 12:20:16]

Description was changed from

==========
Don't use => on void functions.

Fixes #26740
BUG= http://dartbug.com/26740
==========

to

==========
Don't use => on void functions.

Fixes #26740
BUG= http://dartbug.com/26740
R=sra@google.com

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

[floitsch, 2016-07-11 12:20:18]

Committed patchset #1 (id:1) manually as
badabcc53a3256547e6365326798f808d39a343c (presubmit successful).

[floitsch, 2016-07-11 12:22:39]

I landed this CL and I will do a cherry-pick for the next release.
However, there is still a test missing.
@Alan: could you please add a test to the html test-suite so this can't regress.
thanks.

[Alan Knight, 2016-07-11 23:08:44]

On 2016/07/11 12:22:39, floitsch wrote:
> I landed this CL and I will do a cherry-pick for the next release.
> However, there is still a test missing.
> @Alan: could you please add a test to the html test-suite so this can't
regress.
> thanks.

The underlying problem here is that the Chrome IDL has changed from version 46,
and connectNode no longer has a void return type. This fix seems like it's just
introducing a bug for a later date that we discard a valid return value.

If I revert the change, then html/audiocontext_test already fails in
dart2js/chrome/checked locally. However, it crashes under drt, and is suppressed
there, and it seems we only run checked tests under drt. If I attempt to write
something that uses the value, so it could fail not in checked mode, then it
fails to compile. 

After discussions with Stephen, I'm thinking the actions are
 - File a bug to have the compiler enforce void, that is automatically discard a
return value. Maybe only in the case of calling native things.
 - File a bug to remove this fix once we regenerate dart:html
 - Ask to have checked mode run on Chrome on the bots, not just drt.

[floitsch, 2016-07-12 12:51:34]

On 2016/07/11 23:08:44, Alan Knight wrote:
> On 2016/07/11 12:22:39, floitsch wrote:
> > I landed this CL and I will do a cherry-pick for the next release.
> > However, there is still a test missing.
> > @Alan: could you please add a test to the html test-suite so this can't
> regress.
> > thanks.
> 
> The underlying problem here is that the Chrome IDL has changed from version
46,
> and connectNode no longer has a void return type. This fix seems like it's
just
> introducing a bug for a later date that we discard a valid return value.
> 
> If I revert the change, then html/audiocontext_test already fails in
> dart2js/chrome/checked locally. However, it crashes under drt, and is
suppressed
> there, and it seems we only run checked tests under drt. If I attempt to write
> something that uses the value, so it could fail not in checked mode, then it
> fails to compile. 
> 
> After discussions with Stephen, I'm thinking the actions are
>  - File a bug to have the compiler enforce void, that is automatically discard
a
> return value. Maybe only in the case of calling native things.
>  - File a bug to remove this fix once we regenerate dart:html
>  - Ask to have checked mode run on Chrome on the bots, not just drt.

There is more to it: yes, the code was broken because of an IDL change. However,
the templates are hand-written and advertise a `void` return value. When
updating to a new IDL there was nothing that could have prevented us to manually
fix things.
- either we don't return (which is what this CL does). In this case we need to
change the return type of the template and return the "new" value.
- we return (before this CL), but due to the 'void' return type on the template,
it crashes in checked mode.

Imho, there shouldn't be a bug to "remove this fix", but to "update the
template". After all, the template was written under different assumptions.
Clearly, we should also have more tests (or run them more).