fire jsdocCommentFound and symbolFound events in the correct order (#770)

On Rhino, for each file, we now fire all the jsdocCommentFound events in source order, followed by all the symbolFound events in source order. This behavior is consistent with previous versions of JSDoc.

On Node.js, we now fire interleaved jsdocCommentFound and symbolFound events in source order.

Includes a new Rhino .jar file: jsdoc3/rhino@5fbcc2d953

lib/jsdoc/src/astbuilder.js

@@ -185,17 +185,16 @@ function CommentAttacher(comments, tokens) {
     this._tokens = tokens || [];
 
     this._tokenIndex = 0;
-    this._previousNodeEnd = 0;
+    this._previousNode = null;
     this._astRoot = null;
-    this._strayComments = [];
 
-    this._resetPendingComment()
+    this._resetPendingComments()
         ._resetCandidates();
 }
 
 // TODO: docs
-CommentAttacher.prototype._resetPendingComment = function() {
+CommentAttacher.prototype._resetPendingComments = function() {
-    this._pendingComment = null;
+    this._pendingComments = [];
     this._pendingCommentRange = null;
 
     return this;
@@ -221,7 +220,7 @@ CommentAttacher.prototype._nextToken = function() {
 // TODO: docs
 // find the index of the atom whose end position is closest to (but not after) the specified
 // position
-CommentAttacher.prototype._nextIndexBefore = function(startIndex, atoms, position) {
+CommentAttacher.prototype._nextIndexBefore = function(atoms, startIndex, position) {
     var atom;
 
     var newIndex = startIndex;
@@ -244,7 +243,7 @@ CommentAttacher.prototype._nextIndexBefore = function(startIndex, atoms, positio
 CommentAttacher.prototype._advanceTokenIndex = function(node) {
     var position = node.range[0];
 
-    this._tokenIndex = this._nextIndexBefore(this._tokenIndex, this._tokens, position);
+    this._tokenIndex = this._nextIndexBefore(this._tokens, this._tokenIndex, position);
 
     return this;
 };
@@ -252,33 +251,54 @@ CommentAttacher.prototype._advanceTokenIndex = function(node) {
 // TODO: docs
 CommentAttacher.prototype._fastForwardComments = function(node) {
     var position = node.range[0];
-    var commentIndex = this._nextIndexBefore(0, this._comments, position);
+    var commentIndex = this._nextIndexBefore(this._comments, 0, position);
 
-    // all comments before the node (except the last one) are considered stray comments
+    // all comments before the node (except the last one) are pended
     if (commentIndex > 0) {
-        this._strayComments = this._strayComments.concat( this._comments.splice(0,
+        this._pendingComments = this._pendingComments.concat( this._comments.splice(0,
             commentIndex) );
     }
 };
 
+CommentAttacher.prototype._attachPendingCommentsAsLeading = function(target) {
+    target.leadingComments = (target.leadingComments || []).concat(this._pendingComments);
+};
+
+CommentAttacher.prototype._attachPendingCommentsAsTrailing = function(target) {
+    target.trailingComments = (target.trailingComments || []).concat(this._pendingComments);
+};
+
 // TODO: docs
-CommentAttacher.prototype._attachPendingComment = function() {
+CommentAttacher.prototype._attachPendingComments = function(currentNode) {
     var target;
 
-    if (!this._pendingComment) {
+    if (!this._pendingComments.length) {
         return this;
     }
 
+    // if there are one or more candidate nodes, attach the pending comments before the last
+    // candidate node
     if (this._candidates.length > 0) {
         target = this._candidates[this._candidates.length - 1];
-        target.leadingComments = target.leadingComments || [];
+        this._attachPendingCommentsAsLeading(target);
-        target.leadingComments.push(this._pendingComment);
     }
+    // if we don't have a previous node, attach pending comments before the AST root; this should
+    // mean that we haven't encountered any other nodes yet, or that the source file contains
+    // JSDoc comments but not code
+    else if (!this._previousNode) {
+        target = this._astRoot;
+        this._attachPendingCommentsAsLeading(target);
+    }
+    // otherwise, the comments must come after the current node (or the last node of the AST, if
+    // we've run out of nodes)
     else {
-        this._strayComments.push(this._pendingComment);
+        this._attachPendingCommentsAsTrailing(currentNode || this._previousNode);
     }
 
-    this._resetPendingComment()
+    // update the previous node
+    this._previousNode = currentNode;
+
+    this._resetPendingComments()
         ._resetCandidates();
 
     return this;
@@ -329,8 +349,8 @@ CommentAttacher.prototype.visit = function(node) {
     // now we can check whether the current node is in the right position to accept the next comment
     isEligible = this._isEligible(node);
 
-    // attach the pending comment, if there is one
+    // attach the pending comments, if any
-    this._attachPendingComment();
+    this._attachPendingComments(node);
 
     // okay, now that we've done all that bookkeeping, we can check whether the current node accepts
     // leading comments and add it to the candidate list if needed
@@ -341,8 +361,8 @@ CommentAttacher.prototype.visit = function(node) {
         }
         this._candidates.push(node);
 
-        // we have a candidate node, so pend the current comment if necessary
+        // we have a candidate node, so pend the current comment
-        this._pendingComment = this._pendingComment || this._comments.splice(0, 1)[0];
+        this._pendingComments.push(this._comments.splice(0, 1)[0]);
     }
 
     return VISITOR_CONTINUE;
@@ -350,41 +370,50 @@ CommentAttacher.prototype.visit = function(node) {
 
 // TODO: docs
 CommentAttacher.prototype.finish = function() {
-    // any remaining comments are stray comments
+    var length = this._comments.length;
-    this._strayComments = this._strayComments.concat(this._comments);
 
-    // deal with the pending comment, if there is one
+    // any leftover comments are pended
-    this._attachPendingComment();
+    if (length) {
-
+        this._pendingComments = this._pendingComments.concat( this._comments.splice(0, length) );
-    // attach stray comments to the AST root
-    if (this._strayComments.length) {
-        this._astRoot.trailingComments = this._strayComments.slice(0);
     }
+
+    // attach the pending comments, if any
+    this._attachPendingComments();
 };
 
 // TODO: docs
 // TODO: refactor to make this extensible
 /**
+ * @private
  * @param {string} filename - The full path to the source file.
  * @param {Object} ast - An abstract syntax tree that conforms to the Mozilla Parser API.
  */
 AstBuilder.prototype._postProcess = function(filename, ast) {
     var attachComments = !!ast.comments && !!ast.comments.length;
-    var commentAttacher = new CommentAttacher( scrubComments(ast.comments.slice(0)), ast.tokens );
+    var commentAttacher;
-    var visitor = {
+    var scrubbed;
+    var visitor;
+    var walker;
+
+    if (!attachComments) {
+        return;
+    }
+
+    scrubbed = scrubComments(ast.comments.slice(0));
+    commentAttacher = new CommentAttacher(scrubbed.slice(0), ast.tokens);
+    visitor = {
         visit: function(node) {
-            if (attachComments) {
+            return commentAttacher.visit(node);
-                attachComments = commentAttacher.visit(node);
-            }
         }
     };
+    walker = new jsdoc.src.Walker();
 
-    var walker = new jsdoc.src.Walker();
+    walker.recurse(ast, visitor, filename);
-    walker.recurse(filename, ast, visitor);
 
     commentAttacher.finish();
 
-    // remove the comment/token arrays; we no longer need then
+    // replace the comments with the filtered comments
-    ast.comments = [];
+    ast.comments = scrubbed;
+    // we no longer need the tokens
     ast.tokens = [];
 };

lib/jsdoc/src/parser.js

@@ -1,5 +1,3 @@
-/*global env, Packages */
-/*eslint no-script-url:0 */
 /**
  * @module jsdoc/src/parser
  */
@@ -26,13 +24,15 @@ var util = require('util');
 var hasOwnProp = Object.prototype.hasOwnProperty;
 var Syntax = jsdoc.src.syntax.Syntax;
 
-// Prefix for JavaScript strings that were provided in lieu of a filename.
-var SCHEMA = 'javascript:';
 // TODO: docs
 var PARSERS = exports.PARSERS = {
     esprima: 'jsdoc/src/parser',
     rhino: 'rhino/jsdoc/src/parser'
 };
+/*eslint-disable no-script-url */
+// Prefix for JavaScript strings that were provided in lieu of a filename.
+var SCHEMA = 'javascript:';
+/*eslint-enable no-script-url */
 
 // TODO: docs
 exports.createParser = function(type) {
@@ -120,7 +120,7 @@ Parser.prototype.clear = function() {
  * var docs = jsdocParser.parse(myFiles);
  */
 Parser.prototype.parse = function(sourceFiles, encoding) {
-    encoding = encoding || env.conf.encoding || 'utf8';
+    encoding = encoding || global.env.conf.encoding || 'utf8';
 
     var filename = '';
     var sourceCode = '';
@@ -233,7 +233,7 @@ Parser.prototype._parseSourceCode = function(sourceCode, sourceName) {
 
         ast = this._astBuilder.build(sourceCode, sourceName);
         if (ast) {
-            this._walker.recurse(sourceName, ast, this._visitor);
+            this._walkAst(ast, this._visitor, sourceName);
         }
     }
 
@@ -241,6 +241,11 @@ Parser.prototype._parseSourceCode = function(sourceCode, sourceName) {
     logger.info('complete.');
 };
 
+/** @private */
+Parser.prototype._walkAst = function(ast, visitor, sourceName) {
+    this._walker.recurse(ast, visitor, sourceName);
+};
+
 // TODO: docs
 Parser.prototype.addDocletRef = function(e) {
     var node;

lib/jsdoc/src/visitor.js

@@ -182,13 +182,6 @@ Visitor.prototype.visit = function(node, filename) {
         this._visitors[i].call(this, node, this._parser, filename);
     }
 
-    // we also need to visit standalone comments, which are not attached to a node
-    if (node.type === Syntax.Program && node.comments && node.comments.length) {
-        for (i = 0, l = node.comments.length; i < l; i++) {
-            this.visitNodeComments.call(this, node.comments[i], this._parser, filename);
-        }
-    }
-
     return true;
 };
 
@@ -206,8 +199,8 @@ function isValidJsdoc(commentSrc) {
 
 // TODO: docs
 function hasJsdocComments(node) {
-    return (node && node.leadingComments && node.leadingComments.length > 0) ||
+    return (node && node.leadingComments && node.leadingComments.length) ||
-        (node && node.trailingComments && node.trailingComments.length > 0);
+        (node && node.trailingComments && node.trailingComments.length);
 }
 
 // TODO: docs
@@ -233,13 +226,10 @@ Visitor.prototype.visitNodeComments = function(node, parser, filename) {
         return true;
     }
 
-    comments = [];
+    comments = (node.type === BLOCK_COMMENT) ? [node] : [];
-    if (node.type === BLOCK_COMMENT) {
-        comments.push(node);
-    }
 
     if (node.leadingComments && node.leadingComments.length) {
-        comments = node.leadingComments.slice(0);
+        comments = comments.concat( node.leadingComments.slice(0) );
     }
 
     if (node.trailingComments && node.trailingComments.length) {
@@ -269,12 +259,6 @@ Visitor.prototype.visitNode = function(node, parser, filename) {
 
     var e = this.makeSymbolFoundEvent(node, parser, filename);
 
-    if (!node.nodeId) {
-        Object.defineProperty(node, 'nodeId', {
-            value: parser.getUniqueId()
-        });
-    }
-
     if (this._nodeVisitors && this._nodeVisitors.length) {
         for (i = 0, l = this._nodeVisitors.length; i < l; i++) {
             this._nodeVisitors[i].visitNode(node, e, parser, filename);

lib/jsdoc/src/walker.js

@@ -521,14 +521,17 @@ Walker.prototype._recurse = function(filename, ast) {
 };
 
 // TODO: docs
-// TODO: allow visitor.visit to prevent traversal of child nodes
 // TODO: skip the AST root node to be consistent with Rhino?
-Walker.prototype.recurse = function(filename, ast, visitor) {
+Walker.prototype.recurse = function(ast, visitor, filename) {
+    var shouldContinue;
     var state = this._recurse(filename, ast);
 
     if (visitor) {
         for (var i = 0, l = state.nodes.length; i < l; i++) {
-            visitor.visit.call(visitor, state.nodes[i], filename);
+            shouldContinue = visitor.visit.call(visitor, state.nodes[i], filename);
+            if (!shouldContinue) {
+                break;
+            }
         }
     }
 

rhino/jsdoc/src/parser.js

@@ -36,3 +36,13 @@ Parser.prototype.addNodeVisitor = function(visitor) {
 Parser.prototype.getNodeVisitors = function() {
     return this._visitor.getRhinoNodeVisitors();
 };
+
+// TODO: docs
+Parser.prototype._walkAst = function(ast, visitor, sourceName) {
+    // On Rhino, we visit the comments all at once before we walk the AST
+    this._visitor.visitNodeComments({
+        leadingComments: ast.comments
+    }, this, sourceName);
+
+    Parser.super_.prototype._walkAst.call(this, ast, visitor, sourceName);
+};

rhino/jsdoc/src/visitor.js

@@ -15,6 +15,11 @@ var Visitor = exports.Visitor = function(parser) {
     // Rhino nodes retrieved from the org.jsdoc.AstBuilder instance
     this._rhinoNodes = null;
 
+    // only visit nodes, not their comments--the parser visits all the comments at once
+    this._visitors = [
+        this.visitNode
+    ];
+
     this.addAstNodeVisitor({
         visitNode: this._visitRhinoNode.bind(this)
     });
@@ -26,6 +31,14 @@ Visitor.prototype.addRhinoNodeVisitor = function(visitor) {
     this._rhinoNodeVisitors.push(visitor);
 };
 
+// TODO: docs (deprecated)
+Visitor.prototype.removeRhinoNodeVisitor = function(visitor) {
+    var idx = this._rhinoNodeVisitors.indexOf(visitor);
+    if (idx !== -1) {
+        this._rhinoNodeVisitors.splice(idx, 1);
+    }
+};
+
 // TODO: docs (deprecated)
 Visitor.prototype.getRhinoNodeVisitors = function() {
     return this._rhinoNodeVisitors;
@@ -34,9 +47,8 @@ Visitor.prototype.getRhinoNodeVisitors = function() {
 // TODO: docs (deprecated)
 Visitor.prototype._visitRhinoNode = function(astNode, e, parser, filename) {
     var rhinoNode;
-
     var visitors = this._rhinoNodeVisitors;
-    // if there are no visitors, bail out before we retrieve all the nodes
+
     if (!visitors.length) {
         return;
     }

test/fixtures/eventorder.js

@@ -0,0 +1,32 @@
+'use strict';
+
+var _ = require('underscore');
+
+/**
+ * Socket class.
+ * @class
+ */
+function Socket() {
+    // ...
+}
+
+/**
+ * Send a packet.
+ * @param {Packet} packet - The packet to send.
+ * @return {boolean} `true` on success, `false` on failure.
+ */
+Socket.prototype.send = function(packet) {
+    // ...
+};
+
+/**
+ * Virtual doclet for `Packet` class.
+ * @class Packet
+ */
+
+/**
+ * Close the socket.
+ */
+Socket.prototype.close = function() {
+    // ...
+};

test/specs/jsdoc/src/parser.js

@@ -340,6 +340,71 @@ describe('jsdoc/src/parser', function() {
                     expect(doclet.comment).toMatch('REPLACED!');
                 });
             });
+
+            describe('event order', function() {
+                var events = {
+                    all: [],
+                    jsdocCommentFound: [],
+                    symbolFound: []
+                };
+                var source = fs.readFileSync(path.join(global.env.dirname,
+                    'test/fixtures/eventorder.js'), 'utf8');
+
+                function pushEvent(e) {
+                    events.all.push(e);
+                    events[e.event].push(e);
+                }
+
+                function sourceOrderSort(atom1, atom2) {
+                    if (atom1.range[1] < atom2.range[0]) {
+                        return -1;
+                    }
+                    else if (atom1.range[0] < atom2.range[0] && atom1.range[1] === atom2.range[1]) {
+                        return 1;
+                    }
+                    else {
+                        return 0;
+                    }
+                }
+
+                // Rhino fires events in a different order
+                if (jasmine.jsParser === 'rhino') {
+                    it('should fire all jsdocCommentFound events, in source order, ' +
+                        'then all symbolFound events, in source order', function() {
+                        parser.on('jsdocCommentFound', pushEvent);
+
+                        parser.on('symbolFound', pushEvent);
+
+                        jsdoc.src.handlers.attachTo(parser);
+                        parser.parse('javascript:' + source);
+
+                        // make sure jsdocCommentFound events are in the correct order
+                        events.jsdocCommentFound.slice(0).sort(sourceOrderSort)
+                            .forEach(function(e, i) {
+                            expect(e).toBe(events.jsdocCommentFound[i]);
+                        });
+                        // make sure symbolFound events are in the correct order
+                        events.symbolFound.slice(0).sort(sourceOrderSort).forEach(function(e, i) {
+                            expect(e).toBe(events.symbolFound[i]);
+                        });
+                        // make sure jsdocCommentFound events are all first
+                        events.all.slice(0, events.jsdocCommentFound.length)
+                            .forEach(function(e, i) {
+                            expect(e).toBe(events.all[i]);
+                        });
+                    });
+                }
+                else {
+                    it('should fire interleaved jsdocCommentFound and symbolFound events, ' +
+                        'in source order', function() {
+                        jsdoc.src.handlers.attachTo(parser);
+                        parser.parse(source);
+                        events.all.slice(0).sort(sourceOrderSort).forEach(function(e, i) {
+                            expect(e).toBe(events.all[i]);
+                        });
+                    });
+                }
+            });
         });
 
         describe('addAstNodeVisitor', function() {