Throw FatalError after reporting generic introspection error #812
Conversation
| try { | ||
| let realmOptions = { serialize: true, compatibility, uniqueSuffix: "" }; | ||
| let realm = construct_realm(realmOptions); | ||
| initializeGlobals(realm); | ||
| let serializerOptions = { initializeMoreModules: speculate, delayUnsupportedRequires, internalDebug: true }; | ||
| let serializer = new Serializer(realm, serializerOptions); | ||
| let sources = [{ filePath: name, fileContents: code }]; | ||
| let serialized = serializer.init(sources, false, onError); | ||
| let serialized = serializer.init(sources, false); |
There was a problem hiding this comment.
I think I had introduced the onError parameter to the serializer just for dealing with Introspection errors here. Can the concept be removed in the serializer code if there's no other user for it?
There was a problem hiding this comment.
Ignore, I saw you already did it!
| this.nativeStack = nativeStack || new Error().stack; | ||
| } | ||
|
|
||
| nativeStack: string; | ||
| } | ||
| export class IntrospectionThrowCompletion extends ThrowCompletion { |
src/serializer/modules.js
Outdated
| let [message, stack] = this.modules.getMessageAndStack(effects); | ||
| console.log(`delaying require(${moduleIdValue}): ${message} ${stack}`); | ||
| if (result instanceof PossiblyNormalCompletion) { | ||
| console.log(`delaying require(${moduleIdValue}`); |
There was a problem hiding this comment.
Remove this line, as we'll output the same message below.
There was a problem hiding this comment.
ah yes, will do.
| @@ -97,8 +91,6 @@ class ModuleTracer extends Tracer { | |||
| return undefined; | |||
| } else { | |||
| let result; | |||
| let oldErrorHandler = realm.errorHandler; | |||
| realm.errorHandler = () => "Fail"; | |||
There was a problem hiding this comment.
By deleting this code, the messages will appear on the screen as errors, not differentiating to our confused users that this is in case recoverable by delaying the require() call?
There was a problem hiding this comment.
This is indeed a problem. I'll address it today in a followup pull request.
| @@ -461,6 +393,9 @@ export class Modules { | |||
| if (escapes) return undefined; | |||
|
|
|||
| return compl; | |||
| } catch (err) { | |||
| if (err instanceof FatalError) return undefined; | |||
There was a problem hiding this comment.
Looks like you sneaked in an actual bug fix?
There was a problem hiding this comment.
It got uncovered by throwing FatalError for all failures and therefore needs fixing here.
|
@hermanventer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@hermanventer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Instead of throwing peculiarly behaving introspection error completions, we now report all errors via the error handler, using CompilerDiagnostic objects that are just normal host objects. Following a report, we always throw a FatalError.
Eventually all of the places where generic introspection errors are thrown will be replaced by specific errors and may have site specific error recovery logic.