Open sgrekhov opened 4 years ago
And that is why you don't use recursion on something where you can't control the depth!
Indeed, we are using recursion while building the global inference graph when visiting super and redirecting constructor initializers. We do so in order to properly mark whether this
is exposed indirectly. To remove this use of recursion we would need to instead do a quick second pass to propagate this information after the graph is built.
I have just spend some time looking into this crash before finding this issue. Doubling the stack size (by recompiling the VM) makes it pass. This test is flaky, since changes to the VM can cause the stack frame size to change. There is no VM command-line option to control the stack size.
@lrhn The Dart specification does not say much about limits to recursion. Have you considered specifying some minimum requirements for a conforming implementation, e.g. handling extends
to a certain depth, e.g., 128? Perhaps the fault is not really with recursion, but the implementation of recursion with a fixed stack. There are language implementations where there is essentially no recursion limit that is separate from an overall memory limit. There are certainly many algorithms which are much easier to write with recursion, so supporting deeper recursion would improve programmer productivity for the subset of programmers that work with these algorithms.
@mraleph A VM command-line stack-size configuration would be very useful for debugging this kind of problem, and enabling this test to pass.
@sgrekhov Is there a version of this test with a slightly less deep class hierarchy? Is there a reason that this goes to C1026
, rather than some other depth?
@rakudrama I'm afraid we don't have less deep version of this test. We do have test with the deep 3, but it's not the case. Here we check the corner case of extreme deep class hierarchy. But, it's not a problem to add 128 and 512 versions of the test. Do you want me to add these tests?
I don't think that depth of C1026
was choosen by any other reasons than simply some big number
@sgrekhov A C500
version would be great.
I notice for many of the co19 tests that dart2js generates code like Expect_isTrue(true)
.
When this happens, the tests are testing dart2js compiler optimizations rather than runtime behaviour.
This is probably happening a lot for AOT too.
I wonder if it is worthwhile to try to augment the tests so that there are cases that are easy to optimize and cases that are impossible to optimize to increase coverage to both compile-time and run-time.
The bot can't read well.
@rakudrama I've added C500
deep test. Please see https://github.com/dart-lang/co19/commit/0242ff0196d58910139a3ac6307805da0f78986c
I suspect that statements like Expect.isTrue(C500() is C);
are optimized to Expect_isTrue(true)
. @rakudrama could you please confirm that? If so, then we should find all tests that have hints like hint - Unnecessary type check; the result is always 'true'.
and add additional runtime checks there. I'll create a separate co19 issue for that
Good catch, thank you
UPD Created https://github.com/dart-lang/co19/issues/1243
dart2js fails on the following co19 test https://github.com/dart-lang/co19/blob/master/Language/Types/Interface_Types/subtype_t27.dart
Output is