dart2js: Make functions that appear to be unreachable throw.

Make functions that appear to be unreachable throw.

Functions are unreachable if their parameters have an empty type. We would generate bogus code for the function due to the empty type so we might as well generate a throw. This can prevent spurious inclusion of more code referenced only in the unreachable code.

It is usually a bug if we generate unreachable methods but it can happen for legitimate reasons.  One is that type inference is sometimes more precise than type propagation in the optimizer. Another is that we call a function that has an uninstantiated parameter type. This usually happens due the infeasibility of the path being hidden by a correlation between the type and some variable's value.

There is a tail of bugs that need to be flushed out before we can enable this by default.

Enable with:

  DART_VM_OPTIONS=-Dunreachable-throw=true  <compile>


language/cyclic_default_values_test fails, with foo and foo2 falsely unreachable:

DART_VM_OPTIONS=-Dunreachable-throw=true tools/test.py -mrelease -cdart2js -rd8  language/cyclic_default_values_test


I have added a test for the cause - a type inference bug with default argument values.

DART_VM_OPTIONS=-Dunreachable-throw=true tools/test.py -mrelease dart2js/simple_inferrer_const_closure_default_test
R=sigmund@google.com

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

[sra1, 2015-09-04 02:28:21]

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

[sra1, 2015-09-04 03:25:02]

Patchset #1 (id:1) has been deleted

[sra1, 2015-09-04 03:25:09]

Patchset #1 (id:20001) has been deleted

[sra1, 2015-09-04 03:25:15]

Patchset #1 (id:40001) has been deleted

[sra1, 2015-09-04 04:03:44]

Patchset #1 (id:60001) has been deleted

[sra1, 2015-09-04 04:05:37]

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

[sra1, 2015-09-04 04:05:38]

Stephan - IIRC you fixed a bug like this (type inference of default values). Do
you have any ideas where the problem lies?

[Siggi Cherem (dart-lang), 2015-09-04 17:46:23]

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:2398: JS_EFFECT(() {
When we declare JS_EFFECT, does it mean that we ignore the rest of the possible
effects in the body of this function during type-inference?

Or we add these because we can't otherwise see the effects within the `JS`
blocks below?

(I assume it's more the latter? If so, would it be worth moving the JS_EFFECT
block down and have one next to each of the expressions that causes the effect?)

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:2402:
BoundClosure.receiverOf(JS('BoundClosure', '0'));
what's the reason you need to change `void 0` to 0?

https://codereview.chromium.org/1320673004/diff/80001/tests/compiler/dart2js/...
File tests/compiler/dart2js/simple_inferrer_const_closure_default_test.dart
(right):

https://codereview.chromium.org/1320673004/diff/80001/tests/compiler/dart2js/...
tests/compiler/dart2js/simple_inferrer_const_closure_default_test.dart:74:
checkArgument('method1', compiler.typesTask.uint31Type);
which of these fail today? We could turn this into a multi-test and mark the
failures explicitly so you can submit it to the repo.

[sra1, 2015-09-04 21:01:53]

PTAL.  Added test suppressions and traccking issue.

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:2398: JS_EFFECT(() {
On 2015/09/04 17:46:23, Siggi Cherem (dart-lang) wrote:
> When we declare JS_EFFECT, does it mean that we ignore the rest of the
possible
> effects in the body of this function during type-inference?

No.

> 
> Or we add these because we can't otherwise see the effects within the `JS`
> blocks below?

Yes.

> 
> (I assume it's more the latter? If so, would it be worth moving the JS_EFFECT
> block down and have one next to each of the expressions that causes the
effect?)

JS_EFFECT pretends the code is called in resolution and type inference but omits
the code in building the IR.
It is almost the same as writing  if (false) { ... }
but I assume that resolution and type inference might one day see through that.

I'd be happy to use a different mechanism but it is less painful that writing
special case code to have the same effect in resolution, and type inference.

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:2402:
BoundClosure.receiverOf(JS('BoundClosure', '0'));
On 2015/09/04 17:46:23, Siggi Cherem (dart-lang) wrote:
> what's the reason you need to change `void 0` to 0?

I don't need to.  'void 0' is just as arcane as '0'.
I can change them back.

[Siggi Cherem (dart-lang), 2015-09-04 21:04:30]

lgtm

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1320673004/diff/80001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:2402:
BoundClosure.receiverOf(JS('BoundClosure', '0'));
On 2015/09/04 21:01:52, sra1 wrote:
> On 2015/09/04 17:46:23, Siggi Cherem (dart-lang) wrote:
> > what's the reason you need to change `void 0` to 0?
> 
> I don't need to.  'void 0' is just as arcane as '0'.
> I can change them back.

no need, just wanted to make sure I didn't miss anything important.

[sra1, 2015-09-04 22:11:22]

Committed patchset #2 (id:100001) manually as
cbaae993a156a04c07a45db2be51295d3a753a58 (presubmit successful).

[herhut, 2015-09-08 19:54:34]

On 2015/09/04 04:05:38, sra1 wrote:
> Stephan - IIRC you fixed a bug like this (type inference of default values).
Do
> you have any ideas where the problem lies?

I took a look and have a half baked patch but am off for a week of vacation. The
issue is that type inference uses normal inference rules for closures that have
been successfully traces but are passed to Function.apply. It should in that
case just assume dynamic as currently Function.apply is not understood by
inference.

The second issue is that closures that are passed as default arguments to other
closures should bail out in tracing. This works for closures that are passed
directly as arguments but it seems default values have been missed in the
original design.

If this can wait a little, I will finish the patch and fix it after I return.

[sra1, 2015-09-08 22:35:50]

On 2015/09/08 19:54:34, herhut wrote:
> On 2015/09/04 04:05:38, sra1 wrote:
> > Stephan - IIRC you fixed a bug like this (type inference of default values).
> Do
> > you have any ideas where the problem lies?
> 
> I took a look and have a half baked patch but am off for a week of vacation.
The
> issue is that type inference uses normal inference rules for closures that
have
> been successfully traces but are passed to Function.apply. It should in that
> case just assume dynamic as currently Function.apply is not understood by
> inference.
> 
> The second issue is that closures that are passed as default arguments to
other
> closures should bail out in tracing. This works for closures that are passed
> directly as arguments but it seems default values have been missed in the
> original design.
> 
> If this can wait a little, I will finish the patch and fix it after I return.

That would be great. I will assign you #24297 as a reminder.