Change in dart/sdk[master]: [VM] [proof of concept] gen_kernel throw on read file failure
Jens Johansen (Gerrit)
@Clement as I understood you, this was basically what you wanted.
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.
Jens Johansen (Gerrit)
Jens Johansen would like Johnni Winther and Clement Skau to review this change.
[VM] [proof of concept] gen_kernel throw on read file failure
This is a proof of concept of clients (gen_kernel in this case)
potentially wanting to bail early on specific errors (in this case when
trying to read a file or the sdk that doesn't exist).
With this, a call like
out/ReleaseX64/dart pkg/vm/bin/gen_kernel.dart --platform nonexisting \
pkg/compiler/bin/dart2js.dart
will give
Unhandled exception:
Error: SDK summary not found: nonexisting.
[...]
and a call like
out/ReleaseX64/dart pkg/vm/bin/gen_kernel.dart --platform \
out/ReleaseX64/vm_platform_strong.dill t.dart
(there t.dart imports a non-existing file) will give
Unhandled exception:
Crash when compiling file:///usr/local/google/home/jensj/code/dart-sdk/sdk/nonexisting.dart,
at character offset null:
t.dart:1:8: Error: Error when reading 'nonexisting.dart': No such file or directory
import 'nonexisting.dart';
^
[...]
Change-Id: I16ff440280f21e879f5c42cec241d2c8ab172ec2
---
M pkg/front_end/lib/src/api_unstable/vm.dart
M pkg/vm/lib/kernel_front_end.dart
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/pkg/front_end/lib/src/api_unstable/vm.dart b/pkg/front_end/lib/src/api_unstable/vm.dart
index dadc7d6..4bd44ea 100644
--- a/pkg/front_end/lib/src/api_unstable/vm.dart
+++ b/pkg/front_end/lib/src/api_unstable/vm.dart
@@ -2,8 +2,15 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
+export 'package:_fe_analyzer_shared/src/messages/codes.dart'
+ show Code, codeCantReadFile, codeSdkSummaryNotFound;
+
export 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'
- show DiagnosticMessage, DiagnosticMessageHandler, getMessageUri;
+ show
+ DiagnosticMessage,
+ DiagnosticMessageHandler,
+ getMessageUri,
+ getMessageCodeObject;
export 'package:_fe_analyzer_shared/src/messages/severity.dart' show Severity;
diff --git a/pkg/vm/lib/kernel_front_end.dart b/pkg/vm/lib/kernel_front_end.dart
index e9bb876..b289bd0 100644
--- a/pkg/vm/lib/kernel_front_end.dart
+++ b/pkg/vm/lib/kernel_front_end.dart
@@ -17,6 +17,7 @@
import 'package:front_end/src/api_unstable/vm.dart'
show
+ Code,
CompilerContext,
CompilerOptions,
CompilerResult,
@@ -29,6 +30,9 @@
ProcessedOptions,
Severity,
StandardFileSystem,
+ codeCantReadFile,
+ codeSdkSummaryNotFound,
+ getMessageCodeObject,
getMessageUri,
kernelForProgram,
parseExperimentalArguments,
@@ -479,6 +483,13 @@
if (message.severity == Severity.error) {
hasCompilationErrors = true;
}
+ Code messageCode = getMessageCodeObject(message);
+ if (identical(messageCode, codeCantReadFile) ||
+ identical(messageCode, codeSdkSummaryNotFound)) {
+ StringBuffer sb = new StringBuffer();
+ printDiagnosticMessage(message, sb.writeln);
+ throw sb.toString();
+ }
previousErrorHandler?.call(message);
}
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Markov (Gerrit)
DBC
1 comment:
File pkg/vm/lib/kernel_front_end.dart:
Patch Set #1, Line 491:
throw sb.toString();Throwing an exception would crash front-end server. Can we report an error more gracefully?
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.
Jens Johansen (Gerrit)
1 comment:
Patch Set #1, Line 491:
throw sb.toString();
Throwing an exception would crash front-end server. […]
This is a proof of concept of what I understood Clement wanted to do, I'm not sure what the intend is. I - for one - do not intend to land this CL.
Also, I don't think this is used from the front-end server, only from gen_kernel.
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.
Clement Skau (Gerrit)
Thanks for looking into this, Jens.
1 comment:
Patch Set #1, Line 491:
throw sb.toString();
This is a proof of concept of what I understood Clement wanted to do, I'm not sure what the intend i […]
My intent is to have gen_kernel fail as informatively as possible.
So to me the proposed changes look like a vast improvement over the previous generic "at character offset null: Bad state: Empty input given.".As for this particular throw; we might be able to replace it with a print or something else since:
- `gen_kernel` will exit with whatever `runCompiler(..)` returns.
- `runCompiler(..)` will check for `errorDetector.hasCompilationErrors` which I believe both `codeSdkSummaryNotFound` and `codeCantReadFile` set.
Again, I don't really know the intended relationship between the different components, or how a tool (e.g. gen_kernel) is supposed to handle errors in `runCompiler(..)`.
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Markov (Gerrit)
1 comment:
Patch Set #1, Line 491:
throw sb.toString();
Also, I don't think this is used from the front-end server, only from gen_kernel.
Front-end server uses compileToKernel from kernel_front_end.dart for non-incremental compilations (invoked in certain modes from Flutter tools and g3). compileToKernel uses ErrorDetector and so it uses this code. As a side note, ideally we should share even more code between front-end server and gen_kernel.
I would treat an unhandled exception thrown by the tool as a crash and a bug in the tool. If we need to interrupt compilation in case of this error, can we do that in CFE? Or maybe we can throw a custom exception and catch it in all places where ErrorDetector is used (compileToKernel, runCompiler and in protobuf_aware_tree_shaker.dart) and then gracefully exit.
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.
Jens Johansen (Gerrit)
Jens Johansen abandoned this change.
To view, visit change 142140. To unsubscribe, or for help writing mail filters, visit settings.