Sdk: Allow runtime syntax errors to be caught

[nex3]

Currently the following code throws an uncatchable syntax error:

import 'dart:async';

void badFn() {continue;}

void main() {
  runZoned(() => badFn(), onError: (error, stackTrace) {
    print("caught error");
  });
}

This doesn't print "caught error", but instead kills the isolate due to the error. However, the error is only thrown at runtime, as can be seen with this modification:

void badFn() {continue;}

void main(List<String> args) {
  if (args.isNotEmpty) badFn();
}

This only throws a syntax error if an argument is passed on the command line. Moreover, the error is recoverable; for example:

// inner.dart
import 'dart:async';

void badFn() {continue;}

void main(List<String> args) {
  new Future(() => badFn());
  badFn();
}

// outer.dart
import 'dart:isolate';

main() async {
  var isolate = await Isolate.spawnUri(
      Uri.parse("inner.dart"), [], null, paused: true);
  isolate.setErrorsFatal(false);

  var receivePort = new ReceivePort();
  isolate.addErrorListener(receivePort.sendPort);
  isolate.resume(isolate.pauseCapability);

  receivePort.listen((error) => print(error.first));
}

Running outer.dart will produce two errors, so clearly there's a way to keep the isolate running and even keep badFn callable despite this syntax error.

---

This is important for use cases like the test runner, where user code is loaded and specific functions are run. Error reporting should be associated with the functions that cause the errors, but they can only be captured globally and there's no way to link them back to the actual function that failed.

This also means that the user can't be confident that running an arbitrary function in an error zone will capture any errors that function might produce. This is useful even when not loading code dynamically; for example, an application may want to have a global error handler for itself that shows an error-reporting dialog.

[kevmoo]

@sgjesse @lrhn Thoughts on this? Straight-forward?

[lrhn]

@iposva-google

It's the VM that decides what to do with a syntax error. I believe it currently makes it a top-level error and drops the current stack - it doesn't throw an error as such.

Setting errors non-fatal and listening on isolate errors does work to see the errors, but it doesn't mean that recovery is possible. It's also problematic to drop the stack without executing finally blocks. That wasn't a problem before errors could be made non-fatal, but now it's visible.

Example:

// inner.dart
import 'dart:async';
import 'dart:isolate';

void badFn() {continue;}

void main() {
  Isolate.current.setErrorsFatal(false);
  RawReceivePort p = new RawReceivePort(print);
  Isolate.current.addErrorListener(p.sendPort);
  Timer.run(() {  // Wait for the isolate calls to be handled.
    new Future(() => badFn());
    try {
      print("1");  // Printed
      badFn();
    } finally {
      print("2");  // Not printed!
    }
  });
}

In general it's not nice to have a try/finally execute the try and not the finally when the isolate stays alive. It may cause logical problems if a mutex is entered but not released, or similar problems.

That also makes me expect that the first syntax error will rewind right through the microtask queue code and stop executing microtasks until the next event loop event. An uncaught error in a microtask will reach the event loop, but a finally block on the way reschedules the microtask loop if there are more pending microtasks. If finally blocks are ignored, that fails too.

It may be a prettier solution to throw a SyntaxError from the call-point, just as we do for other "warn-at-compile/fail-at-runtime" errors, but that's a design decision for the VM people. The spec is silent on how a syntax error must be reported.

[munificent]

cc @asadovsky

[mhausner]

Reporting syntax errors in code that isn't executed is contrary to what the goals for Dart have been since the beginning. The stated goal was to be as forgiving as possible when running code, so that fast edit-execute cycles support rapid prototyping. The language spec reflects that, by making many errors only fire at runtime, rather than at compile time, even if they are detected by the compiler.

[kevmoo]

"fire at runtime" is totally fine – we just want at way to know that an isolate failed BECAUSE of bad syntax with the related info.

[nex3]

To be clear, my ideal solution here would be: if a function with a syntax error is executed at runtime, it throws an exception (maybe a new SyntaxError type?) that works just like a normal exception.

[lrhn]

I agree with Natalie - if an Isolate _starts_ running, it should keep running and should not be torn down in the middle of executing because it calls a function that has a previously undiscovered syntax error.
Not starting at all is ok - that's what grievous syntax errors does anyway (like not being a Dart file at all). Throwing an error is also fine, that's what malformed types or undefined variables do. Letting some other syntax errors do the same is fine with me.

[goderbauer]

Is there any update on this? We are currently running into the same issue with our continuous test runner for dart. If the test scheduled to run contains a syntax error, there is no way to detect that.

[sanfordredlich]

Update just to say that this is important for browser-based tests. Right now we use timeouts, which is very brittle and not guaranteed to work well in every case.

[floitschG]

@sanfordredlich browser programs (once compiled to JavaScript) should not exhibit that problem. Obviously, if you are running your tests in Dartium, then you will be affected by this too.

[sanfordredlich]

Thanks, yes, our tests will run in Dartium until Q4 or beyond.

[mhausner]

I have a CL that implements 'catchable' syntax errors.

     1  import 'dart:async';
     2  
     3  void badFn() {continue;}
     4  
     5  void main() {
     6    runZoned(() => badFn(), onError: (error, stackTrace) {
     7      print("caught: $error");
     8    });
     9  }

Note how the onError handler is invoked to print the compilation error:

dart ~/tmp/badfn.dart
caught: 'file:///Users/hausner/tmp/badfn.dart': error: line 3 pos 15: 'continue' is illegal here
void badFn() {continue;}
              ^

If we were to adopt this change, the VM and dart2js diverge, which is not desirable. Note how dart2js does not invoke the error handler:

dart ./pkg/compiler/lib/src/dart2js.dart --package-root=xcodebuild/DebugX64/packages/ ~/tmp/badfn.dart
/Users/hausner/tmp/badfn.dart:3:15:
Error: 'continue' statement not inside loop.
void badFn() {continue;}
              ^^^^^^^^^
Error: Compilation failed.

Also, many co19 tests are failing because they catch all errors, so expected compilation errors get swallowed.

If dart2js implements this change, we can move ahead (and update co19).

[nex3]

I don't think it necessarily makes sense for dart2js to implement this error. As I understand it, the error is detected lazily in the VM for performance reasons; the fact that dart2js detects them at compile-time is a desirable feature. And it's a feature that already diverged from the VM, since dart2js would never execute any part of the program in question and the VM would.

[mhausner]

There is a difference between performance being different, and program semantics being different. If you want the same behavior (and performance) as dart2js, start the tests with --compile_all and you the syntax error will get caught before the program starts (matching dart2js).

I do not think the VM should implement this if dart2js doesn't match.

[nex3]

But the VM already doesn't match dart2js. It seems wrong to make dart2js intentionally suppress useful errors just to match the VM.

We should have a PM weigh in on this. @kevmoo? @anders-sandholm?

[kevmoo]

If I'm reading this right, the concern is that the compile-time behavior/error emitted by dart2js is different than the new runtime behavior of the VM?

I consider this very much NOT an issue – we are introducing a very useful, but very VM-specific behavior IMHO.

We want to keep a very VM-specific feature – lazy parsing – which is very different than what we do in dart2js. We just want it to do something more helpful than it does now.

[a-siva]

There are a slew of co19 tests which fail when we enable this feature because the co19 tests have a catch block which does nothing, yet the tests are expected to fail with a compilation error.

One could say that the co19 test status files could be patched and the failing tests could be ignored, that is not a right thing to do. These tests need to be modified so that they continue to test the compilation errors that they were written to test. I would think a consistent behavior of the tools (dart2js and the VM runtime) is necessary in order for the co19 folks to get this right, otherwise you end up with one set of tests written explicitly for the way dart2js works and another for the way the runtime VM works making it more complex.

[goderbauer]

I would also like to be able to catch syntax errors in Dartium. One thing I don't understand in the discussion on this issue: We can already catch syntax errors in the stand-alone Dart VM outside of Dartium reasonably well (package:test demonstrates that here for example). Why can the same functionality not be brought to Dartium? Personally, I find this difference between executing dart code in the stand-alone dart vm and executing dart code in Dartium very strange.

[a-siva]

Dartium implements something called DOM Isolates which are different from the stand-alone VM isolates and I don't believe these isolates were wired to implement the new Isolate API (ability to add an error listener) and treat errors as non Fatal which your test uses. Maybe @terrylucas can comment on whether this could be done in Dartium if that is all you need.

[goderbauer]

Any update on this?

The number 1 complained from the users of our continuous web test runner is that the runner cannot recover from syntax errors due to this issue. We simply cannot detect that there are syntax errors in the test file we are trying to run in Dartium.

[kevmoo]

We've agreed to do this. We're discussing implementation details at the moment. Stay tuned...

[kevmoo]

Pending CL: https://codereview.chromium.org/2044753002/

[goderbauer]

Exciting. Thanks for working on it.

[a-siva]

This issue is blocked on https://github.com/dart-lang/co19/issues/69

[kevmoo]

Trying to figure out what the next steps are with https://github.com/dart-lang/co19/issues/69 so we can get this landed...

[mit-mit]

Bumping this to 1.20, while we are close we are not quite there

[mit-mit]

https://github.com/dart-lang/co19/issues/69 landed! @mhausner anything else you need to be unblocked?

[mhausner]

The co19 changes must be integrated into the sdk repo, if they aren't already.

[mit-mit]

@whesse did you look at doing that integration?

[whesse]

That is the co19 roll I did today and rolled back. With the status updates from this commit, I can land it permanently tomorrow.

[kevmoo]

Started https://codereview.chromium.org/2044753002/

[kevmoo]

Fixed in 3e0d13bc287cc89ebc15969376a90bd7f67bef6a – Woo hoo!

Created a sample with pkg/test

Before

00:00 +0 -1: loading test/io_server_test.dart
  Failed to load "test/io_server_test.dart":
  line 16 pos 15: 'continue' is illegal here
  void badFn() {continue;}
                ^
00:00 +0 -1: Some tests failed.

After

00:00 +0 -1: serves HTTP requests with the mounted handler
  'file:///Users/kevmoo/source/github/shelf/test/io_server_test.dart': error: line 16 pos 15: 'continue' is illegal here
  void badFn() {continue;}
                ^

  test/io_server_test.dart 30:20  main.<fn>.<async>.<fn>
  dart:async                      runZoned
  test/io_server_test.dart 30:5   main.<fn>.<async>
  dart:async                      _SyncCompleter.complete
  package:http/http.dart 167:5    _withClient.<async>

00:00 +2 -1: Some tests failed.

I can see where the bad call happened. The rest of the tests ran.

Awesome!

[goderbauer]

Very cool! Thank you! Cannot wait to get my hands on it.