Move $ContextStack to $ErrorData, and append location data for syntax errors #461
Conversation
| @@ -18,9 +18,9 @@ import traverse from "babel-traverse"; | |||
| import { parse } from "babylon"; | |||
| import type { BabelNodeFile } from "babel-types"; | |||
|
|
|||
| export default function (realm: Realm, code: string, filename: string, sourceType: SourceType = "script"): BabelNodeFile { | |||
| export default function (realm: Realm, code: string, filename: string, sourceType: SourceType = "script", startLine: number = 1): BabelNodeFile { | |||
There was a problem hiding this comment.
I also needed the startLine option to model contextify's ability to shift source location numbers. @kittens is there a way to do the same for columns in babylon?
There was a problem hiding this comment.
We should try to make sure that everything we add to the code base has test coverage, so it would be better if startLine can be added in a separate pull request that includes a use case and test for it.
There was a problem hiding this comment.
The goal of this string of PRs is to break out everything from #397 with changes existing code. To make it easier to review that one. I could just leave it in #397 as one big blob but that makes rebasing and reviewing harder. I could make up some artificial test case here but I'm not quite sure how to do a pure unit test since we don't really have any infra set up for unit tests. I'd have to wire up a lot of things just to test this.
There was a problem hiding this comment.
Fair enough. It is usually possible to do an integration test that exercises new code, but in this case we'd need a unit testing story.
sebmarkbage
force-pushed
the
syntaxerrorlocation
branch
2 times, most recently
from
662e039 to
4c3aae5
Compare
src/utils/parse.js
Outdated
| } | ||
| if (error.$ErrorData) { | ||
| // If the constructed Error object has the built-in error data append |
There was a problem hiding this comment.
why "if"? My expectation is that Construct will always provide a value for it. Consequently, I would prefer to state this in an invariant.
There was a problem hiding this comment.
I originally had it as an invariant but then I wondered if this could be messed with at runtime. Since this can get called after something has messed with internal constructors and prototypes.
I don't think there is any mechanism to change the allocation strategy here at the moment (even if you change the prototype), but if there was a reflective mechanism to change the constructor or if this constructor was later specified to invoke a super call of its prototype (like the inheritance mechanism is set up) then it would be possible.
That was my defensive rationale at least. What do you think?
There was a problem hiding this comment.
My understanding of the code is that there is no way for user code to cause $ErrorData to be missing from an Error object. If a change to the prepack code causes $ErrorData to be absent, it is very likely an unintended and undesirable change, so an invariant is a far better way to deal with the absence than relying on a unit test to fail.
| @@ -18,9 +18,9 @@ import traverse from "babel-traverse"; | |||
| import { parse } from "babylon"; | |||
| import type { BabelNodeFile } from "babel-types"; | |||
|
|
|||
| export default function (realm: Realm, code: string, filename: string, sourceType: SourceType = "script"): BabelNodeFile { | |||
| export default function (realm: Realm, code: string, filename: string, sourceType: SourceType = "script", startLine: number = 1): BabelNodeFile { | |||
There was a problem hiding this comment.
We should try to make sure that everything we add to the code base has test coverage, so it would be better if startLine can be added in a separate pull request that includes a use case and test for it.
…rrors
I believe the $ErrorData slot is meant to be an internal host provided
object that keeps things like the context stack on it. So we don't need
to come up with another slot to store it. We might want to minimize
internal fields to create monomorphic data structures as a perf
optimization.
I also added another field to $ErrorData that stores the source location
of syntax errors when the parser causes an error.
This is used inside hosting environments such as Node to print a nicer
error message with the location of the syntax error. This is different
from the contextStack since this is not actually part of the execution
stack.
This can be used to print messages with an error showing the location of
the syntax error.
```
foo.js:3
x ++ y
^
SyntaxError: Unexpected identifier
```
sebmarkbage
force-pushed
the
syntaxerrorlocation
branch
from
4c3aae5 to
2994c52
Compare
sebmarkbage
dismissed
hermanventer’s stale review
I switched to using invariant. I pushed a branch but didn't submit another PR to avoid bloating the PRs since we can't delete them. However you can see this branch passing here: https://circleci.com/gh/facebook/prepack/289
|
Oh because I pushed the same commit I guess it shows as passed here too. Cool. |
I believe the $ErrorData slot is meant to be an internal host provided object that keeps things like the context stack on it. So we don't need to come up with another slot to store it. We might want to minimize internal fields to create monomorphic data structures as a perf optimization.
I also added another field to $ErrorData that stores the source location of syntax errors when the parser causes an error.
This is used inside hosting environments such as Node to print a nicer error message with the location of the syntax error. This is different from the contextStack since this is not actually part of the execution stack.
This can be used to print messages with an error showing the location of the syntax error.
You can see how I use it to model the contextify module in node.js which uses this data to append to the stack of an error if it bubbles up to the evaluation of a script boundary (a contextify script).
We could do the same thing in the Repl for example to show where syntax errors occurred.