Being able to parse JavaScript is becoming more important(and challenging). Returning a simplified AST from spidermonkey would allow projects to not worry about a parser arms-race between analysis tools and javascript engines.

This is a big enough task that it probably needs a design document. It'd be nice to understand better what this is going to be used for, what the alternatives we've considered are, and what priority that is relative to the other stuff we'd like to do.

There was discussion on the es-discuss mailing list recently about exposing a JavaScript AST as well: https://mail.mozilla.org/pipermail/es-discuss/2009-December/010226.html

See also Bug 449467 - Expose jsdIScript byte codes

Started adding some notes here: https://wiki.mozilla.org/JavaScript:SpiderMonkey:Parser_API Dave

I currently have some work on this for jshydra. It doesn't do proper AST codecs, yet, nor does it do E4X properly.

In more detail: Presently, I have a jshydra script which takes a SpiderMonkey JSParseNode tree and converts it into a JS AST tree (it's not quite a 1-to-1 correspondence, especially when a + b + c gets involved). The output I give is roughly in the following format: { type: "Program", location: "0:0", sourceElements: [ { type: "FunctionDeclaration", location: "1:1", precedence: Infinity, name: null, arguments: [], body: [ ... ] } ] } A more complete listing is at <http://hg.mozilla.org/users/Pidgeot18_gmail.com/jshydra/file/f0ed09aa0c87/utils/astml.js#l48>. This differs from the JsonMLAST format in a few ways: 1. Type names are different. Mine were taken from a mixture of the specification itself, the visitor format, and the top of my head for things I didn't see in the spec (e.g., array comprehensions). 2. Each node is an object, not an array; all of the named properties are in the object itself, not in element two of the array, and child nodes are merely special properties in the output. Some property names are also different. I'm not terribly attached to this format; it was just much easier for me to work with. It seems that my API focuses more heavily on the nodes than what the ES wiki proposal had. I would like to point out that converting from this format to the JsonML format is rather straightforward, so it's possible to use both of them at once. In particular, the format I'm using makes more sense for a C or C++ API to the AST (more static checking). 3. Location values are currently in the form "startline:startcol"; I don't have ending locations yet, nor do I have the source file. 4. The visitor API is taken more from pork than from the other proposal. Basically, the visitor has these methods: bool visit<NodeType>(node); void postvisit<NodeType>(node); When node.visit(visitor) is called, it does roughly the following: if (!visit<NodeType>(this)) { visit subchildren; } postvisit<NodeType>(this); (it doesn't error if you fail to implement any of the functions). The exact source code is at <http://hg.mozilla.org/users/Pidgeot18_gmail.com/jshydra/file/f0ed09aa0c87/utils/astml.js#l100>; the decompiler visitor is itself at <http://hg.mozilla.org/users/Pidgeot18_gmail.com/jshydra/file/f0ed09aa0c87/scripts/decompile.js>. The return type is structured such that you pretty much get the same operation as the other proposal if you merely implement the visit nodes, although I do structure the parameters differently and function names are not quite the same. Now, onto other comments: This is currently written entirely in JavaScript, using JSHydra to get at the SpiderMonkey parse tree. I did this because memory management is less painful in JS and I can change structure a lot more easily. Once the last few pieces are added and the thing is checked for more completeness, it could probably be rewritten in C or (my preference) C++ without too many difficulties. I would argue that it should be possible to convert between any two of the three representations with a single method call, although I am willing to allow the AST -> source link to not try to be fully precise in locations. Indeed, it may be desirable to choose how that link works, from a "compress the source code" option to a "pretty print the output nicely using these formatting rules for whitespace, brace style, etc." I would also argue that all three of source (via string), parse nodes, and ast nodes should be reflected in JavaScript in some fashion. In addition, the AST nodes may probably want to be part of the exported API, so that people can build tools that link SpiderMonkey as a shared library and not as a static library. Some results of the tool so far: First off, it destroys the parse tree in one part (not too hard to change, though). I know it breaks on the following constructs: * let {a: x, b: y} = baz() * (i for (i in l)) <- generator, NOT array comprehension * let (x = 5, y = 12) {} (I'm just not getting a tree for some reason) * Anything involving E4X Running it on some js/src/tests/js1_5/decompilation files gave it two notable issues: 1. regress-350666.js... TOK_FILTERs strewn everywhere. 2. regress-349484.js... I got a NULL JSParseNode tree. I don't know how correct the output is, but running it with a closer eye on other JS scripts in jshydra seemed to produce mostly correct output. (Just my like that bash decides to head for a decompilation test suite first when listing all JS files). Finally, congratulations on reading my entire comment. My apologies on the length.

Should this bug now depend on bug 558437, since bug 548461 was obsoleted by it?

Atul: sure thing-- added. I left bug 548461 in there just to keep the history. Thanks, Dave

> This differs from the JsonMLAST format in a few ways: I'd like to avoid too much thrashing on API design, so I am aiming for a more generic API that allows people to create custom formats. I would like to expose a simple parse function that creates some reasonable representation by default, but takes an optional builder[1] argument, which allows people to create their own representation. > I would also argue that all three of source (via string), parse nodes, and ast > nodes should be reflected in JavaScript in some fashion. What's your distinction between parse nodes and AST nodes? Parse nodes are 1:1 with source and AST nodes are more abstracted? I think we should just offer a minimal API with maximal data, and let people do what they want with it on the JS side -- eg, they can abstract however they like. I don't, however, think it'll be easy to wrap persistent ParseNode instances in JS objects (instead of cloning the data and then disposing of the ParseNode), because they're tied in with the arena mechanism. I know it's been frustrating to you trying to track a changing codebase. It'll hopefully be much less maddening for you if we get this functionality into the SM codebase itself. But you'll be customer number O(1) so I'll make sure we provide all the data you need. Dave

Whoops, dangling reference. --Dave [1] http://en.wikipedia.org/wiki/Builder_pattern

(In reply to comment #10) > > This differs from the JsonMLAST format in a few ways: > > I'd like to avoid too much thrashing on API design, so I am aiming for a more > generic API that allows people to create custom formats. I would like to expose > a simple parse function that creates some reasonable representation by default, > but takes an optional builder[1] argument, which allows people to create their > own representation. I'm not too deadset on the actual JS format; this was just easier for me to work with and makes more sense for a more structured C++ backend. Making an optional builder is an interesting idea, though. > What's your distinction between parse nodes and AST nodes? Parse nodes are 1:1 > with source and AST nodes are more abstracted? I think we should just offer a > minimal API with maximal data, and let people do what they want with it on the > JS side -- eg, they can abstract however they like. Parse node == JSParseNode; AST node is closer to the actual AST. Parse nodes are the least important of the triad, though. > I don't, however, think it'll be easy to wrap persistent ParseNode instances in > JS objects (instead of cloning the data and then disposing of the ParseNode), > because they're tied in with the arena mechanism. The format I've used to publish JS objects does throw away the parse node instances. > I know it's been frustrating to you trying to track a changing codebase. It'll > hopefully be much less maddening for you if we get this functionality into the > SM codebase itself. But you'll be customer number O(1) so I'll make sure we > provide all the data you need. Unfortunately, time is at a bit of a premium for me right now (end of semester). A rapidly-moving codebase doesn't present much of an issue to me, though, as long as I can figure out how to fix the calls. The more annoying change is when the parse tree is restructured; one of my todo lists is to fix the jsparse.h comment to reflect reality.

Dave, What is the status of this work? Sounds like AMO has an intern that may be able to test this out(assuming it's far enough along). Is there an ETA?

(In reply to comment #13) > Dave, > What is the status of this work? Sounds like AMO has an intern that may be able > to test this out(assuming it's far enough along). Is there an ETA? Sorry for the lack of progress. This bug is bubbling up towards the top of my queue. We've got a TC39 meeting coming up in a week, but by mid-next week I hope to attack this head on. I don't expect it to be that hard, just a fair number of cases to cover. (I'll try to post partial patches as I go.) Dave

My plan: 1. Expose a simple plain-objects-and-arrays representation of AST's directly via Reflect.parse. The JsonMLAST format is really inconvenient to traverse, so I'll do something with just a tiny bit more structure (e.g., nodes are objects with a "type" property rather than arrays), similar to jcranmer's representation. I'll have a draft of this very soon, hopefully later today (minus some extensions like sharp variables and E4X). 2. Generalize this with a C++ JSParseNode visitor that's parameterized by a builder. 3. Expose this to JS by extending Reflect.parse to take an optional builder argument that defaults to the current. 4. Implement a JsonMLAST builder. (But I'm not sure how much we really should care about this. It's not a high priority to me.) Each of these can be done as a separate bug. Dave

Sounds good to me. With respect to the JSONMLAst, given that the current format is only a Strawman proposal, it may be a good idea to propose the other representation and let competition choose the more useful format. Since I expect that the primary utility of having an AST representation is to be able to traverse it, settling on an easier-to-traverse format would probably be better all around.

First draft. All the basic functionality is there. The next things it needs are: - to be configured off everywhere except the shell - more tests Dave

BTW, the patch is for the jaegermonkey tree. It should be pretty painless to move it to a different tree, though, since it's mostly new files and doesn't really touch anything that's been changing too drastically lately. Dave

This version is only available to the shell. Dave

First draft of documentation at the URL. Second version of patch on its way, with lots more tests, a couple bugfixes, and little tweaks to match the docs. Dave

Lots more tests, couple little bugfixes, and tweaked to match the documented API. Dave

Looking forward to this. :)

This patch has more or less complete coverage for all the features of Mozilla SpiderMonkey, unless there are any that I don't know about. :) Since Chris is on vacation and hasn't had a chance to finish the review, I figured I'd just go ahead and finish rather than do this in bug 569485. This patch also fixes a few bugs in the previous patch and makes a few changes to the public API. I'll update the wiki soon with the updated API. I still need to add more test cases for the new stuff. (It's a little hard to reproduce some of the E4X cases because of the aggressive XML constant-folding, but I'll see what I can come up with.) Dave

> This patch has more or less complete coverage for all the features of Mozilla > SpiderMonkey, unless there are any that I don't know about. Er, I mean, "all the Mozilla-specific features of JavaScript." As you were. Dave

This version has many more tests. Ready for review. Dave

Thanks Dave. We're blocked on the new add-on validator without this, so looking forward to it landing. :)

Some general cleanup based on cdleary's suggestions, and switched over to the tracemonkey tree, so it should be pretty current. Still ready for review. Dave

Comment on attachment 451149 [details] [diff] [review] some style improvements based on private suggestions by cdleary I really like this patch because it shows exactly the use case the the AST visitor pattern should account for. Refactoring the parse node datastructure to make this stuff easy (i.e. removing almost all of the code from js::Serializer) seems like the right direction. Oh yeah, and all that "practical use" stuff is nice too. ;-) r=me addressing this stuff however you see fit. - labeledStatement, not labelledStatement - makeNode1/2 can be overloads of makeNode - label-like public/private get a half-indent - Style in NodeBuilder::setObjectProperties - No JS_ASSERTS on some of the node builders, like expressionStatement, versus labelledStatement - in NodeBuilder::tryStatement if one block gets curlies, they all do (also TOK_RB case in statement serializer, Serializer::functionArgsAndBody else clause) - for consideration: is it really better for tryStatement to unbox the catch node handlers list than make an array with 0-to-n nodes inside it? - Check range on VarDeclKind in NodeBuilder::variableDeclaration - We line up member idents at the star (js::Serializer) - Serializer may be too general a name for the js namespace -- ASTSerializer? *shrug* - Serializer class-definition-inline methods have brace on same line (rules are in style guide) - Should use sizeof(buf1) in snprintf within Serializer::failAt to avoid duplication. If you're understandably wary of taking sizeof a now-fixed-length-but-maybe-not-later array, I usually break out a static const size_t. :-) - I think curlies go at same indent level as case (half-indent) when blocking out a switch case. - Ternary is redundant in comprehensionBlock for isForEach -- compiler enforces that bool variables always hold false or true (no other values) - In Serializer::literal, JS_NewNumberValue can fail (unchecked) - Our definitions for thunk and closed seem wishy-washy... closed excludes the lexically enclosing global scope (TOK_UPVARS isn't the be-all indicator), and thunk can still refer to |arguments| as it's currently coded. What's the use case for these meta-attributes? They seem easy to define/understand incorrectly. - In reflect_parse, nice shortcut is |(JSString*)->chars()| and |(JSString*)->length()|

> I really like this patch because it shows exactly the use case the the AST > visitor pattern should account for. Thanks-- yeah, I see this as a hopefully a step along the way to a more general visitor. > - No JS_ASSERTS on some of the node builders, like expressionStatement, versus > labelledStatement I think I'll just remove them, since they don't help us in production and, given the nullability comments, they're redundant as documentation. > - for consideration: is it really better for tryStatement to unbox the catch > node handlers list than make an array with 0-to-n nodes inside it? This was a wee bit of future-proofing for potential standardization; since multiple catch-clauses are Mozilla-specific, a standard representation would be more likely to be BlockStatement-or-null. So I extended that in a compatible way. It's just a tiny bit messier, but still easy to destructure: if (x === null) { noCatch() } else if (x.type === "BlockStatement") { oneCatch(x) } else { multipleCatches(x) } So I'm inclined to leave this as is. > - Serializer may be too general a name for the js namespace -- ASTSerializer? > *shrug* That was bugging me -- thanks for the push. ASTSerializer WFM. > - Ternary is redundant in comprehensionBlock for isForEach -- compiler enforces > that bool variables always hold false or true (no other values) I had this in a couple other places, too. Thx. > - Our definitions for thunk and closed seem wishy-washy... closed excludes the > lexically enclosing global scope (TOK_UPVARS isn't the be-all indicator), and > thunk can still refer to |arguments| as it's currently coded. What's the use > case for these meta-attributes? They seem easy to define/understand > incorrectly. More to the point: YAGNI. You're absolutely right. I'm stripping out the "meta" property, eliminating "closed" and "thunk" and just putting "generator" and "expression" as direct properties of the node object. (FWIW, while in theory "closed" is used to mean "no free variables whatsoever," in practice, some languages, including Scheme and JS, distinguish between free lexical variables and top-level variables, so there's a useful notion of "closed wrt lexical variables." But there's no point exposing this unless we know someone cares.) Thanks, Dave

Comment on attachment 451149 [details] [diff] [review] some style improvements based on private suggestions by cdleary >+#ifdef DEBUG_dherman >+ fprintf(stderr, "JS_EnumerateStandardClasses!\n"); >+#endif >+ This doesn't belong in the tree. Review should catch stuff like this. >+ * >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): Year + contributor. >+/* >+ * Bug 569487: generalize builder interface >+ * - use jsval instead of JSObject * >+ * - use JSVAL_HOLE to represent "no value" >+ */ Avoid JSVAL_HOLE at all cost. Maybe JSVAL_VOID? >+ jsval intern(const char *s) { return STRING_TO_JSVAL(JS_InternString(cx, s)); } Why do you use JS_InternString here? >+JSObject * >+NodeBuilder::makeObject(NodePropertySpec *props) >+{ >+ JSObject *obj = makeObject(); >+ RNULL_UNLESS(obj); obj is unrooted here (gc hazard). Not sure this is exploitable. JS_DefineProperty probably doesn't trigger GCs. >+ RNULL_UNLESS(setObjectProperties(obj, props)); >+ return obj; >+} >+ >+JSObject * >+NodeBuilder::makeObject(const char *childName, jsval child) >+{ >+ NodePropertySpec props[] = { >+ PS(childName, child), >+ PS_END >+ }; >+ return makeObject(props); >+} >+ >+JSObject * >+NodeBuilder::makeObject(const char *childName1, jsval child1, >+ const char *childName2, jsval child2) >+{ >+ NodePropertySpec props[] = { >+ PS(childName1, child1), >+ PS(childName2, child2), >+ PS_END >+ }; >+ return makeObject(props); >+} >+ >+bool >+NodeBuilder::setObjectProperties(JSObject *obj, NodePropertySpec *props) >+{ >+ for (size_t i = 0; props[i].key; i++) { >+ RFALSE_UNLESS(setObjectProperty(obj, props[i].key, props[i].val)); >+ } >+ return true; >+} >+ >+JSObject * >+NodeBuilder::makeNode(ASTType type, TokenPos *pos) >+{ >+ if (type <= AST_ERROR || type >= AST_LIMIT) >+ return internalError(); >+ >+ JSObject *node = JS_NewObject(cx, &js_ASTNodeClass, NULL, NULL); >+ RNULL_UNLESS(node); >+ RNULL_UNLESS(setNodeLoc(node, pos)); Now this is definitely a GC hazard. Several allocations inside of it. node is unrooted. Also plenty of GC hazards in setNodeLoc. I think you should back out the patch and get a 2nd review from someone who understands the rooting protocol, or wait until we are certain that our stack scanner works as expected and then re-land without manual rooting.

(In reply to comment #30) > I think you should back out the patch and get a 2nd review from someone who > understands the rooting protocol, or wait until we are certain that our stack > scanner works as expected and then re-land without manual rooting. That was extremely dumb of me -- I think my mind was in conservative stack scanning mode. Until that patch lands, pretty much everywhere multiple |make*| are called sequentially (anywhere in the sub- call graph) needs be fixed. To keep your nice symmetry you can have your |make*| take an |AutoValueRooter&| from the creating frame and allocate into that.

Also doesn't look like the external file parsing approach is going to be easy given that the test infrastructure barfs on other platforms: TEST-UNEXPECTED-FAIL | trace-test.py | e:\builds\moz2_slave\tracemonkey-win32\build\js\src\trace-test\tests\reflect\parse.js: e:\builds\moz2_slave\tracemonkey-win32\build\js\src\trace-test\tests\reflect\parse.js:164: Error: can't open data/flapjax.txt: No such file or director http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1276634740.1276647820.19025.gz Dave, despite the rush, before next one we should do a sanity push to try server: https://wiki.mozilla.org/Build:TryServer

Faulty changeset: http://hg.mozilla.org/tracemonkey/rev/fecc8ed9e813 Backed out changeset: http://hg.mozilla.org/tracemonkey/rev/c287fcea6684

> Year + contributor. I put my name on the new files; I didn't include it in shell/js.cpp since I didn't edit it significantly. > >+/* > >+ * Bug 569487: generalize builder interface > >+ * - use jsval instead of JSObject * > >+ * - use JSVAL_HOLE to represent "no value" > >+ */ > > Avoid JSVAL_HOLE at all cost. Maybe JSVAL_VOID? Probably more appropriate to discuss over in bug 569487, but JSVAL_VOID is no good for a general API that allows arbitrary jsval results. It wants some kind of disjoint union (jsval | nothing); jorendorff proposed JSVAL_HOLE on #jsapi. I'm open to alternatives. > >+ jsval intern(const char *s) { return STRING_TO_JSVAL(JS_InternString(cx, s)); } > > Why do you use JS_InternString here? Seemed appropriate, on the expectation that code using this API would use these strings frequently. Is there a particular reason why I shouldn't? I can easily change it. > Now this is definitely a GC hazard. Several allocations inside of it. node is > unrooted. Also plenty of GC hazards in setNodeLoc. I appreciate the scrutiny; I should've talked to someone about the memory management before submitting the patch. Mea culpa. I will fix ASAP. Dave

It's not necessary to update Contributor comments in the license block. /be

(In reply to comment #34) > > >+/* > > >+ * Bug 569487: generalize builder interface > > >+ * - use jsval instead of JSObject * > > >+ * - use JSVAL_HOLE to represent "no value" > > >+ */ > > > > Avoid JSVAL_HOLE at all cost. Maybe JSVAL_VOID? > > Probably more appropriate to discuss over in bug 569487, but JSVAL_VOID is no > good for a general API that allows arbitrary jsval results. It wants some kind > of disjoint union (jsval | nothing); jorendorff proposed JSVAL_HOLE on #jsapi. > I'm open to alternatives. I don't think we must hide JSVAL_HOLE from friend APIs, but we do hide JSVAL_HOLE from jsapi.h and its includes -- JSVAL_HOLE is defined in jsbool.h which is not a public header. Best to be fatval-ready and consult with Luke on this. > > >+ jsval intern(const char *s) { return STRING_TO_JSVAL(JS_InternString(cx, s)); } > > > > Why do you use JS_InternString here? Note that this is a fallible API but a null return meaning failure will not be caught, instead bogusly tagged as a string. > Seemed appropriate, on the expectation that code using this API would use these > strings frequently. Is there a particular reason why I shouldn't? I can easily > change it. We avoid JS_* calls in the engine where possible -- DLL linkage makes them costly and they are usually layered on internal API to use instead. In the case of JS_InternString the internal API is js_Atomize or a sibling. Moreover, for well-known strings we have pre-interned atoms at fixed offsets in the runtime's atomState (this should be driven from a jsatom.tbl include file but it's not -- easy cleanup bug). What is the set of strings potentially interned here? /be

> I don't think we must hide JSVAL_HOLE from friend APIs, but we do hide > JSVAL_HOLE from jsapi.h and its includes -- JSVAL_HOLE is defined in jsbool.h > which is not a public header. > > Best to be fatval-ready and consult with Luke on this. Sure thing. > Note that this is a fallible API but a null return meaning failure will not be > caught, instead bogusly tagged as a string. OK, thanks. > Moreover, for well-known strings we have pre-interned atoms at fixed offsets in > the runtime's atomState (this should be driven from a jsatom.tbl include file > but it's not -- easy cleanup bug). That may be the better solution here anyway. The interned strings are all parser API constants: operator names ("++", "--", "<=", etc), lighweight enums ("init" | "get" | "set"), etc. So I guess I should factor them out as constants. Dave

Note getAtom, setAtom, etc. in JSAtomState. /be

I've redone the API with rooted jsval* outparams, and use AutoGCRooters everywhere for memory correctness. I also went ahead and updated a couple out-of-date jsparse.h comments and eliminated an instance of dead logic in jsparse.cpp. Dave

One thing I noticed: js> Reflect.parse('this') ({start:{line:0, column:0}, end:{line:1, column:4}, type:"Program", body:[ {start:{line:1, column:0}, end:{line:1, column:4}, type:"ExpressionStatement", expression:{ start:{line:1, column:0}, end:{line:1, column:4}, type:"Literal", value:false } }] }) Shouldn't this be returning a ThisExpression instead of a false Literal?

Thanks, yeah, just a missed case. Updating patch in a sec. I'm too tired to go looking for more missed cases tonight but I'll try to some time tomorrow. Dave

Added missing case for "this". Dave

backed out

Comment on attachment 456405 [details] [diff] [review] added missing case for "this" I like the format you've chosen for NodeBuilder to construct. JsonMLAST is not an especially fluent design; I'd *much* rather write |node.type| than |node[0]|. Do the operators in binary, logical, update, and assignment nodes need to be their own nodes? The fact that the type of the expression node dictates the type of the operator node ("BinaryExpression" means "BinaryOperator", etc.) suggests to me that they don't. Wouldn't it suffice for the "operator" property to simply be the string that would be the "token" property of the operator node? There seem to be no unit tests for "loc" properties. My experience has led me to believe that it's just wacked to commit code that isn't covered by unit tests. It doesn't seem that you've addressed Brendan's comments about using pre-interned atoms, or about avoiding JS_* calls in engine code: https://bugzilla.mozilla.org/show_bug.cgi?id=533874#c36 The comment on https://developer.mozilla.org/en/SpiderMonkey/Parser_API about getters and setters being SpiderMonkey-specific is mysterious; ES5 has getters and setters in object literals, but it's not clear from the docs what they would look like, though --- is the 'value' Expression a function expression? SM has additional syntax for describing them, which may permit forms ES5 does not... anyway, this needs to be explained better. I'd like to re-review this patch after we've addressed these comments. --- a/js/src/js.msg +++ b/js/src/js.msg @@ -330,3 +330,5 @@ MSG_DEF(JSMSG_CSP_BLOCKED_FUNCTION, 24 +MSG_DEF(JSMSG_BAD_PARSE_NODE, 251, 2, JSEXN_INTERNALERR, "unimplemented parse node (type {0}, at {1})") +MSG_DEF(JSMSG_BAD_INTERNAL_ARG, 252, 0, JSEXN_INTERNALERR, "unexpected argument") I mention below that the uses of JSMSG_BAD_INTERNAL_ARG should be calls to JS_ASSERT, so JSMSG_BAD_INTERNAL_ARG should go away. diff --git a/js/src/jsast.tbl b/js/src/jsast.tbl new file mode 100644 --- /dev/null +++ b/js/src/jsast.tbl @@ -0,0 +1,125 @@ +/* + * These must be listed in exactly this order so the AST_xxx fields match up + * with their integer definition in jsreflect.cpp. Is this comment still true? It seems like the .tbl file is the sole definition of those values, their names, etc. diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp --- a/js/src/jsparse.cpp +++ b/js/src/jsparse.cpp + * comma expression that we synthesized to body. If the body is a return + * node, we must make a special TOK_SEQ node, to prepend the destructuring + * code without bracing the decompilation of the function body's lexical + * scope. Do you want to fix the second reference to "lexical scope" here as well? +/* + * Bug 569487: generalize builder interface + * - NodeVector may want to hide AutoValueVector + * - template-ize ASTSerializer + */ It seems to me that to-do lists like this should go in the bug, not in the code. + + +/* + * Builder class that constructs JavaScript ASTNode objects. Include a link to the devmo wiki page here? + * + * The constructor is infallible. I think constructors must always be infallible, because they can't return a value. Anything fallible should be moved into an initialization method. So this needn't be said. + * + * All methods with pointer or bool return type are fallible. This is almost universal SpiderMonkey practice. Nobody will be surprised to see these checks. Instead, you should have comments on any boolean- or pointer-valued functions that are *in*fallible. + * + * All fallible methods set a pending exception in the associated + * JSContext before returning. Again, no need to document universal practice. + */ +class NodeBuilder +{ + JSContext *cx; + char const *src; /* nullable */ This member's comment should say what the member is, not simply that it is nullable. + + public: + NodeBuilder(JSContext *c, char const *s) + : cx(c), src(s) { + } + + private: + JSAtom *atomize(const char *s) { return js_Atomize(cx, s, strlen(s), 0); } You should use the runtime's atomState here. + + bool internalError() { It looks like the calls to internalError should be calls to JS_ASSERT. + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_INTERNAL_ARG); + return false; + } + + bool newObject(JSObject **dst, JSClass *clasp = NULL) { + JSObject *nobj = JS_NewObject(cx, clasp, NULL, NULL); + if (nobj) { Nit: standard practice is to return early for errors; the spine of the function should be the non-error code (which can thus be less indented). +bool +NodeBuilder::newNode(ASTType type, TokenPos *pos, JSObject **dst) +{ + if (type <= AST_ERROR || type >= AST_LIMIT) + return internalError(); + + JSAtom *atom = atomize(nodeTypeNames[type]); + if (!atom) + return false; + + JSObject *node = JS_NewObject(cx, &js_ASTNodeClass, NULL, NULL); + if (!node) + return false; + + if (!setNodeLoc(node, pos) || setNodeLoc allocates, and |node| isn't rooted here. + !setProperty(node, "type", STRING_TO_JSVAL(ATOM_TO_STRING(atom)))) There's a macro ATOM_TO_JSVAL that does this; there are several other places below that could use it, too. +bool +NodeBuilder::newArray(NodeVector &elts, jsval *dst) +{ + JSObject *array = JS_NewArrayObject(cx, 0, NULL); + if (!array) + return false; + + const size_t len = elts.length(); + for (size_t i = 0; i < len; i++) { + jsval val = elts[i]; + + /* ASTSerializer represents "no node" as JSVAL_HOLE; we represent it as JSVAL_NULL. */ + if (val == JSVAL_HOLE) + val = JSVAL_NULL; + + if (!JS_SetElement(cx, array, i, &val)) + return false; + } Could you simply pass elts.length() and elts.begin() to JS_NewArrayObject here (after making a pass over elts to fix up the holes)? Among other things, I can't figure out whether JS_SetElement could ever GC, which matters because |array| isn't rooted. (Also, this should use js_NewArrayObject.) +bool +NodeBuilder::setNodeLoc(JSObject *node, TokenPos *pos) +{ + if (!pos) + return setProperty(node, "loc", JSVAL_NULL); + + AutoObjectRooter tvr(cx); + AutoValueRooter tvr2(cx); + + return newObject(tvr.addr()) && + setProperty(node, "start", OBJECT_TO_JSVAL(tvr.object())) && + JS_NewNumberValue(cx, (jsdouble)pos->begin.lineno, tvr2.addr()) && Nit: are the casts to jsdouble really necessary? I think C++ will happily do this silently, and it's guaranteed there's no loss of precision, since both lineno and jsdouble are fixed-width. +bool +NodeBuilder::getTokenPtr(TokenPtr *dst, bool *foundp, JSObject *posn) +{ + JSBool has; + + if (!JS_HasProperty(cx, posn, "line", &has)) + return false; + + if (!has) { + *foundp = false; + return true; + } + + if (!JS_HasProperty(cx, posn, "column", &has)) + return false; + + if (!has) { + *foundp = false; + return true; + } + + AutoValueRooter linev(cx), columnv(cx); + + if (!JS_GetProperty(cx, posn, "line", linev.addr()) || + !JS_GetProperty(cx, posn, "column", columnv.addr())) + return false; Since JS_GetProperty stores JSVAL_VOID when the property doesn't exist, it seems to me the calls to JS_HasProperty here and in NodeBuilder::getTokenPos could be removed with no visible change in behavior. + + if (!JSVAL_IS_NUMBER(linev.value()) || !JSVAL_IS_NUMBER(columnv.value())) { + *foundp = false; + return true; + } + + uint32 line, column; + if (!JS_ValueToECMAUint32(cx, linev.value(), &line) || + !JS_ValueToECMAUint32(cx, columnv.value(), &column)) Perhaps you could just use JSVAL_IS_INT above and use JSVAL_TO_INT in the assignments to dst's members below. +bool +NodeBuilder::getNodeLoc(TokenPos *dst, bool *foundp, jsval node) +{ + if (!JSVAL_IS_OBJECT(node)) { I believe JSVAL_IS_OBJECT returns true for JSVAL_NULL. Better perhaps to use |Valueify(node).isObject()|, here and throughout. + *foundp = false; + return true; + } + + JSObject *obj = JSVAL_TO_OBJECT(node); + + JSBool has; + if (!JS_HasProperty(cx, obj, "loc", &has)) As above, the JS_GetProperty call below will do the job for you, I think. +class ASTSerializer +{ + JSContext *cx; /* ! */ + NodeBuilder builder; /* _ */ What do these comments mean? Is this ES4 type notation? I think it would probably be best to write them out in English. + + /* (pn: ?) */ + bool internalError(JSParseNode *pn) { Are you sure this is an error which we should bother letting the user handle? I think I'd use JS_ASSERT for this, as well. + if (!pn) { + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_PARSE_NODE, + "unknown", "unknown location"); + return false; + } + + static const size_t nbytes = 128; + + char buf1[nbytes]; + JS_snprintf(buf1, nbytes, "%d", pn->pn_type); + + char buf2[nbytes]; + JS_snprintf(buf2, nbytes, "%d:%d - %d:%d", + pn->pn_pos.begin.lineno, pn->pn_pos.begin.index, + pn->pn_pos.end.lineno, pn->pn_pos.end.index); + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_PARSE_NODE, buf1, buf2); Could this error message include a filename? +bool +ASTSerializer::program(JSParseNode *pn, jsval *dst) +{ + if (!pn) { Is there some reason to check this here, but not in the other productions? +/* Reflect class */ + +JSClass js_ASTNodeClass = { Is there any advantage to giving AST node objects a separate class? It seems to me that it would be simplest just to make them plain old native objects. +static JSBool +reflect_parse(JSContext *cx, uintN argc, jsval *vp) +{ + if (argc < 1) { + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED, + "Reflect.parse", "0", "s"); + return JS_FALSE; + } + + JSString *src = js_ValueToString(cx, JS_ARGV(cx, vp)[0]); + if (!src) + return JS_FALSE; + + const char *filename = NULL; + if (argc > 1) { + JSString *str = js_ValueToString(cx, JS_ARGV(cx, vp)[1]); + if (!str) + return JS_FALSE; + filename = JS_GetStringBytes(str); + } It would be nice to accept a third argument specifying the starting line number, so people can use it for subregions of files. + + Parser parser(cx); + if (!parser.init(JS_GetStringChars(src), JS_GetStringLength(src), NULL, filename, 1)) + return JS_FALSE; + + JSParseNode *pn = parser.parse(NULL); + if (!pn) + return JS_FALSE; + + if (!JS_EnterLocalRootScope(cx)) + return JS_FALSE; This API is marked as obsolete; we scan the stack conservatively now, so this and the "leave" call can just be dropped. +namespace js { + +typedef enum ASTType { Actually, the typedefs are unnecessary here: C++ always creates typedefs for enums (and structs and classes). Same for the subsequent enums. static JSBool +ResolveClass(JSContext *cx, JSObject *obj, jsid id, JSBool *resolved) +{ + if (!JS_ResolveStandardClass(cx, obj, id, resolved)) + return JS_FALSE; + + if (! *resolved) { Style nit: no space after the unary '!' operator. + JSAtom *atom = OFFSET_TO_ATOM(cx->runtime, CLASS_ATOM_OFFSET(Reflect)); Can this simply be |CLASS_ATOM(cx, Reflect)|? + if (ATOM_KEY(atom) == id) { This needs to use JSID_IS_ATOM now, I think.

I'll post an updated patch that addresses everything soon, but just wanted to respond to a couple review points. > Do the operators in binary, logical, update, and assignment nodes need to be > their own nodes? The fact that the type of the expression node dictates the > type of the operator node ("BinaryExpression" means "BinaryOperator", etc.) > suggests to me that they don't. Wouldn't it suffice for the "operator" property > to simply be the string that would be the "token" property of the operator > node? The reason I made them nodes, which maybe premature, was future-compatibility with potential standardization; some implementations might want to store source location information for the individual operator. It'd be nice for tools that want to point to the operator rather than the entire expression. Unfortunately the SpiderMonkey parser doesn't track that, so I just give the node a NULL source location property. But maybe I just shouldn't worry too much about future-proofing, since we aren't committing to this API yet and can change it down the road if necessary? > It doesn't seem that you've addressed Brendan's comments about using > pre-interned atoms... Yeah, if it's okay with you, I'd like to push that off to be part of bug 575416. > +/* > + * These must be listed in exactly this order so the AST_xxx fields match up > + * with their integer definition in jsreflect.cpp. > > Is this comment still true? It seems like the .tbl file is the sole definition > of those values, their names, etc. Yes, because nodeTypeNames is indexed in that order. I'll clarify the comment. > + JSAtom *atomize(const char *s) { return js_Atomize(cx, s, strlen(s), 0); } > > You should use the runtime's atomState here. If we pre-interned the atoms? Again, I'd like to hold off on that as a separate patch if it's okay with you. > It looks like the calls to internalError should be calls to JS_ASSERT. I agree that internalError should look more like an assertion, and I used it in places where it should've just been JS_ASSERT. However, there are some cases where I'd still like assertions to be protected by a runtime error: in particular, the JSParseNode data structure invariants are evolutionary, not completely documented, and somewhat intricate. So I propose to do something like the LOCAL_ASSERT macros in jsopcode.cpp: for parse node invariants, I'd like to protect them with a dynamic error using a LOCAL_ASSERT macro. For everything else (e.g., invariants that are completely protected within jsreflect.cpp), I'll use JS_ASSERT. Also, I'll get rid of the internalError method, and the JS_BAD_PARSE_NODE error message shouldn't bother trying to produce diagnostic information about the corrupted parse node. Failing code shouldn't be poking around a suspicious data structure, and in development, we'll use the debugger to get much more diagnostic information anyway. And we shouldn't be presenting low-level details to users who can't do anything with/about it. > + if (!JS_EnterLocalRootScope(cx)) > + return JS_FALSE; > > This API is marked as obsolete; we scan the stack conservatively now, so this > and the "leave" call can just be dropped. Andreas originally r-'ed the patch because stack scanning hadn't landed and I wasn't properly rooting. You've found a few holes still, since I added in explicit rooting. But now that we have scanning, I'm just removing all explicit rooting. As I say, new patch coming soon. Thanks, Dave

(In reply to comment #46) > some implementations might want to store source > location information for the individual operator. It'd be nice for tools that > want to point to the operator rather than the entire expression. I agree that it would be nice to have token-by-token position information, but we'd want that for all kinds of nodes: the '.' in a member access expression, the '?' and ':' in a conditional expression, the braces and parens of a function expression, and so on. Perhaps the origin of the difficulty here is the idea that a parse node has a single source position: if parse nodes may have many relevant source positions, then there's no need to have lots of leaf nodes. It seems to me that one could provide this information in the future with additional properties of the parse node: detailed-loc:{ questionMark:..., colon:... } That could be added in a backwards-compatible way to the existing tree structure, and to my mind, and it seems just as legible: node.detailed-loc.questionMark > > It doesn't seem that you've addressed Brendan's comments about using > > pre-interned atoms... > > Yeah, if it's okay with you, I'd like to push that off to be part of bug > 575416. That's fine with me. > > +/* > > + * These must be listed in exactly this order so the AST_xxx fields match up > > + * with their integer definition in jsreflect.cpp. > > > > Is this comment still true? It seems like the .tbl file is the sole definition > > of those values, their names, etc. > > Yes, because nodeTypeNames is indexed in that order. I'll clarify the comment. If what the new comment is going to say is, "These need to be defined in numeric order, with no gaps", perhaps you could just leave out the integer values altogether. > > + JSAtom *atomize(const char *s) { return js_Atomize(cx, s, strlen(s), 0); } > > > > You should use the runtime's atomState here. > > If we pre-interned the atoms? Again, I'd like to hold off on that as a separate > patch if it's okay with you. Yes, that's fine. > > It looks like the calls to internalError should be calls to JS_ASSERT. > > I agree that internalError should look more like an assertion, and I used it in > places where it should've just been JS_ASSERT. However, there are some cases > where I'd still like assertions to be protected by a runtime error: in > particular, the JSParseNode data structure invariants are evolutionary, not > completely documented, and somewhat intricate. So I propose to do something > like the LOCAL_ASSERT macros in jsopcode.cpp: for parse node invariants, I'd > like to protect them with a dynamic error using a LOCAL_ASSERT macro. For > everything else (e.g., invariants that are completely protected within > jsreflect.cpp), I'll use JS_ASSERT. So in non-DEBUG builds, people will get exceptions? Okay.

(In reply to comment #47) > (In reply to comment #46) > > some implementations might want to store source > > location information for the individual operator. It'd be nice for tools that > > want to point to the operator rather than the entire expression. YAGNI is too strong a response, but I agree with jimb -- leave it out for now, it's easy to add later. > If what the new comment is going to say is, "These need to be defined in > numeric order, with no gaps", perhaps you could just leave out the integer > values altogether. jsopcode.tbl has the bytecode values explicitly entered because bytecode has been part of an external data format for years, and it helps in extreme debugging situations to have those numbers manifest in the source. Likewise jsproto.tbl, although that one is not sanity-checked to be dense using #error, AFAIK. The other .tbl files do not hardcode dense enumerator values, as they (AFAIK) don't appear in external data. So if the JSON encodings are free of magic numbers then yeah, leave out the codes. /be

Updated patch. Will reply to individual review points in subsequent comment, hopefully right now unless I have to run to catch the train, else in a couple hours. Dave

Several things that weren't directly requested in the review but I've addressed: - Everything uses js::Value instead of jsval. - I've removed all explicit rooting to rely on stack scanning instead. - I eliminated the leftAssociate operation from NodeBuilder and moved it to the other side of the abstraction where it belongs: ASTSerializer. Now there's no need for the builder to extract source location information from nodes it's already built in order to synthesize the tree from the list of left-associative operands. Instead, the serializer does the tree synthesis. - Replaced the use of JSVAL_HOLE with a custom "magic val" thanks to the new js::Value representation. This kind of thing is exactly what the magic val enum type was designed for. > I like the format you've chosen for NodeBuilder to construct. JsonMLAST is not > an especially fluent design; I'd *much* rather write |node.type| than > |node[0]|. Agreed. In bug 569487 I'll generalize the interface so people can write a JsonMLAST version in JS if they want, but it's not the right default. > Do the operators in binary, logical, update, and assignment nodes need to be > their own nodes? No, removed. > There seem to be no unit tests for "loc" properties. Indeed they were completely broken. Added tests and fixed. Also discovered bug 588061 in the process. Good catch! > It doesn't seem that you've addressed Brendan's comments about using > pre-interned atoms, or about avoiding JS_* calls in engine code Eliminated all JS_* calls except JS_ReportErrorNumber and a couple harmless ones in js_InitReflectClass. > The comment on https://developer.mozilla.org/en/SpiderMonkey/Parser_API about > getters and setters being SpiderMonkey-specific is mysterious; ES5 has getters > and setters in object literals Brain-****. Updated the page. > I mention below that the uses of JSMSG_BAD_INTERNAL_ARG should be calls to > JS_ASSERT, so JSMSG_BAD_INTERNAL_ARG should go away. Gone. > + * These must be listed in exactly this order so the AST_xxx fields match up > + * with their integer definition in jsreflect.cpp. > > Is this comment still true? Gone, and eliminated the explicit indices. > + * code without bracing the decompilation of the function body's lexical > + * scope. > > Do you want to fix the second reference to "lexical scope" here as well? "...without bracing the decompilation of the function body." > It seems to me that to-do lists like this should go in the bug, not in the > code. Gone. > + * Builder class that constructs JavaScript ASTNode objects. > > Include a link to the devmo wiki page here? Added. > no need to document universal practice. Gone. > + char const *src; /* nullable */ > > This member's comment should say what the member is, not simply that it is > nullable. Fixed. > + private: > + JSAtom *atomize(const char *s) { return js_Atomize(cx, s, strlen(s), 0); } > > You should use the runtime's atomState here. I didn't understand this one? > It looks like the calls to internalError should be calls to JS_ASSERT. Discussed above in the comment thread. > Nit: standard practice is to return early for errors; the spine of the function > should be the non-error code (which can thus be less indented). Fixed. > setNodeLoc allocates, and |node| isn't rooted here. No more rooting concerns (other than corner cases where the compiler un-allocates stack locals early, which Luke warned me about -- there should be no such cases in this code, especially since the vast majority of the cases involve stack locals that are passed by address to other functions). > There's a macro ATOM_TO_JSVAL that does this; there are several other places > below that could use it, too. Fixed. > Could you simply pass elts.length() and elts.begin() to JS_NewArrayObject here > (after making a pass over elts to fix up the holes)? Among other things, I > can't figure out whether JS_SetElement could ever GC, which matters because > |array| isn't rooted. No more rooting concerns. I use js_ArrayCompPush now. Since I need to convert magic vals to null, it's not okay to just pass the array directly, or I'd have to do an additional pass over them all to convert first. Makes more sense to just fuse the two linear passes. > (Also, this should use js_NewArrayObject.) Fixed. > Nit: are the casts to jsdouble really necessary? Gone, thanks to new js::Value methods anyway. > Since JS_GetProperty stores JSVAL_VOID when the property doesn't exist, it > seems to me the calls to JS_HasProperty here and in NodeBuilder::getTokenPos > could be removed with no visible change in behavior. > ... > Perhaps you could just use JSVAL_IS_INT above and use JSVAL_TO_INT in the > assignments to dst's members below. > ... > I believe JSVAL_IS_OBJECT returns true for JSVAL_NULL. Better perhaps to use > |Valueify(node).isObject()|, here and throughout. > ... > As above, the JS_GetProperty call below will do the job for you, I think. All moot since these methods are gone now. > + JSContext *cx; /* ! */ > + NodeBuilder builder; /* _ */ > > What do these comments mean? Is this ES4 type notation? I think it would > probably be best to write them out in English. Whoops, leftovers from an earlier version of this patch, which tried to introduce some new lightweight "nullability" notation -- just ignore me being overly clever. Gone now. > + bool internalError(JSParseNode *pn) { > > Are you sure this is an error which we should bother letting the user handle? I > think I'd use JS_ASSERT for this, as well. Discussed above in comment thread. > Could this error message include a filename? Moot, since internalError is gone. > > +bool > +ASTSerializer::program(JSParseNode *pn, jsval *dst) > +{ > + if (!pn) { > > Is there some reason to check this here, but not in the other productions? Changed to JS_ASSERT. > +JSClass js_ASTNodeClass = { > > Is there any advantage to giving AST node objects a separate class? It seems to > me that it would be simplest just to make them plain old native objects. Decent point. It might be nice to actually create an explicit JS ASTNode function and prototype object, so that people can monkey-patch in their own additional node methods in, but they can also just write functions. By just being Objects and nothing else, they're also easy to synthesize, serialize, deserialize, JSONify, etc. Anyway, I've removed the silly class. > It would be nice to accept a third argument specifying the starting line > number, so people can use it for subregions of files. Added and documented on devmo. > This API is marked as obsolete; we scan the stack conservatively now, so this > and the "leave" call can just be dropped. Gone. > +typedef enum ASTType { > > Actually, the typedefs are unnecessary here You sure? It didn't work for me. I removed the typedefs and the compiler freaked out. So I left them in. > Style nit: no space after the unary '!' operator. Fixed. > > + JSAtom *atom = OFFSET_TO_ATOM(cx->runtime, > CLASS_ATOM_OFFSET(Reflect)); > > Can this simply be |CLASS_ATOM(cx, Reflect)|? Fixed. > + if (ATOM_KEY(atom) == id) { > > This needs to use JSID_IS_ATOM now, I think. Fixed. Phew. And now I'm late for the train, ack! Dave

> > You should use the runtime's atomState here. > > I didn't understand this one? Sorry, ignore that one -- we already cleared this up earlier in the comment thread. Dave

> > +typedef enum ASTType { > > > > Actually, the typedefs are unnecessary here > > You sure? It didn't work for me. I removed the typedefs and the compiler > freaked out. So I left them in. Stupid Dave! Compilers tend to freak out when you send them syntactic gibberish ("enum { ... } Foo;" instead of "enum Foo { ... };"). I will fix this and update the patch in a sec. Also, I missed a FIXME in jsreflect.h, and I may throw in a couple JS_ASSERT's that there aren't magic values other than JS_SERIALIZE_NO_NODE. Dave

Minor fixes: - eliminated one last FIXME in reflect.h - a couple JS_ASSERT_IF's ensuring magic values are only JS_SERIALIZE_NO_NODE - typedef enum { ... } T --> enum T { ... } Dave

>-#define ATOM_TO_STRING(atom) ((JSString *)atom) >-#define ATOM_TO_JSVAL(atom) STRING_TO_JSVAL(ATOM_TO_STRING(atom)) >+#define ATOM_TO_STRING(atom) ((JSString *)(atom)) >+#define ATOM_TO_JSVAL(atom) STRING_TO_JSVAL(ATOM_TO_STRING((atom))) Great catch! But just leave ATOM_TO_JSVAL as it is. The rule is that macros are supposed to be callable with arbitrary expressions, so if the extra parens in ATOM_TO_JSVAL were necessary, it would be ATOM_TO_STRING's fault. r=me with this fixed.

So, is this going to be enabled in the browser, or just the shell?

Eventually, I mean. Obviously this patch doesn't enable it in the browser.

> Great catch! Well, I got bitten by it. But at least my blackbelt in macrology helped me track it down pretty quickly. ;) > are supposed to be callable with arbitrary expressions, so if the extra parens > in ATOM_TO_JSVAL were necessary, it would be ATOM_TO_STRING's fault. Good call. > So, is this going to be enabled in the browser, or just the shell? In the browser, hopefully. But post-FF4, I'm pretty sure. Dave

(In reply to comment #57) ... > In the browser, hopefully. But post-FF4, I'm pretty sure. Is there a bugzilla bug for that effort? Firebug would build UI for this feature as soon as it was available.

> Is there a bugzilla bug for that effort? Firebug would build UI for this > feature as soon as it was available. I've created bug 590973 for enabling in chrome and CC'ed you. Dave