fix comment-attachment issue (#638)

The issue in brief: Within an object literal, if a standalone comment was followed by a commented symbol, the symbol's comment would not be attached correctly.

The fix essentially reverts the changes for #565, which are no longer needed thanks to 50cd99fa2fca753fcf7c9ec3ecf70afd47168e94.

The fix also corrects the order in which we walk a MemberExpression's child nodes. Without this correction, comments would not be attached correctly inside CallExpression nodes.

lib/jsdoc/src/astbuilder.js

@@ -44,20 +44,6 @@ var acceptsLeadingComments = (function() {
     return accepts;
 })();
 
-// TODO: docs
-var searchDescendants = (function() {
-    var search = {};
-    var shouldSearch = [
-        Syntax.CallExpression
-    ];
-
-    for (var i = 0, l = shouldSearch.length; i < l; i++) {
-        search[shouldSearch[i]] = true;
-    }
-
-    return search;
-})();
-
 // TODO: docs
 function canAcceptComment(node) {
     var canAccept = false;
@@ -208,8 +194,6 @@ function CommentAttacher(comments, tokens) {
     this._astRoot = null;
     this._strayComments = [];
 
-    this._parentRange = [];
-
     this._resetPendingComment()
         ._resetCandidates();
 }
@@ -239,11 +223,6 @@ CommentAttacher.prototype._nextToken = function() {
     return this._tokens[this._tokenIndex] || null;
 };
 
-// TODO: docs
-CommentAttacher.prototype._extractComments = function(index, count) {
-    return this._comments.splice(index, count);
-};
-
 // TODO: docs
 // find the index of the atom whose end position is closest to (but not after) the specified
 // position
@@ -277,16 +256,8 @@ CommentAttacher.prototype._advanceTokenIndex = function(node) {
 
 // TODO: docs
 CommentAttacher.prototype._fastForwardComments = function(node) {
-    var commentIndex;
+    var position = node.range[0];
-    var position;
+    var commentIndex = this._nextIndexBefore(0, this._comments, position);
-
-    // don't fast-forward if there might be a sibling that can receive the comment
-    if ( this._parentRange.length && isWithin(node.range, this._parentRange) ) {
-        return;
-    }
-
-    position = node.range[0];
-    commentIndex = this._nextIndexBefore(0, this._comments, position);
 
     // all comments before the node (except the last one) are considered stray comments
     if (commentIndex > 0) {
@@ -299,7 +270,10 @@ CommentAttacher.prototype._fastForwardComments = function(node) {
 CommentAttacher.prototype._attachPendingComment = function() {
     var target;
 
-    if (this._pendingComment) {
+    if (!this._pendingComment) {
+        return this;
+    }
+
     if (this._candidates.length > 0) {
         target = this._candidates[this._candidates.length - 1];
         target.leadingComments = target.leadingComments || [];
@@ -308,7 +282,6 @@ CommentAttacher.prototype._attachPendingComment = function() {
     else {
         this._strayComments.push(this._pendingComment);
     }
-    }
 
     this._resetPendingComment()
         ._resetCandidates();
@@ -344,7 +317,7 @@ CommentAttacher.prototype._isEligible = function(node) {
 
 // TODO: docs
 CommentAttacher.prototype._shouldSkipNode = function(node) {
-    return !isWithin(node.range, this._pendingCommentRange) && !this._parentRange.length;
+    return !isWithin(node.range, this._pendingCommentRange);
 };
 
 // TODO: docs
@@ -360,28 +333,14 @@ CommentAttacher.prototype.visit = function(node) {
     // set the AST root if necessary
     this._astRoot = this._astRoot || node;
 
-    // clear the parent range if we've moved past it
-    if ( this._parentRange.length && !isWithin(node.range, this._parentRange) ) {
-        this._parentRange = [];
-    }
-
-    // if we need to search this node's descendants, set the range in which to search
-    if (searchDescendants[node.type] && !this._parentRange.length) {
-        this._parentRange = node.range.slice(0);
-    }
-
     // move to the next token, and fast-forward past comments that can no longer be attached
     this._advanceTokenIndex(node);
     this._fastForwardComments(node);
     // now we can check whether the current node is in the right position to accept the next comment
     isEligible = this._isEligible(node);
 
-    // if we already had a pending comment, and the current node is in the wrong place to be a
+    // attach the pending comment, if there is one
-    // comment target, try attaching the comment
-    if ( (this._pendingComment && !isWithin(node.range, this._pendingCommentRange)) ||
-        !searchDescendants[node.type] ) {
     this._attachPendingComment();
-    }
 
     // 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

lib/jsdoc/src/walker.js

@@ -285,10 +285,10 @@ walkers[Syntax.Literal] = leafNode;
 walkers[Syntax.LogicalExpression] = walkers[Syntax.BinaryExpression];
 
 walkers[Syntax.MemberExpression] = function memberExpression(node, parent, state, cb) {
+    cb(node.object, node, state);
     if (node.property) {
         cb(node.property, node, state);
     }
-    cb(node.object, node, state);
 };
 
 walkers[Syntax.MethodDefinition] = function methodDefinition(node, parent, state, cb) {

test/fixtures/virtual2.js

@@ -0,0 +1,28 @@
+var Person = Klass.extend(
+/** @lends Person.prototype */
+{
+    /** @constructs Person */
+    initialize: function(name) {
+        this.name = name;
+    },
+
+    /**
+     * Callback for `say`.
+     *
+     * @callback Person~sayCallback
+     * @param {?string} err - Information about the error, if any.
+     * @param {?string} message - The message.
+     */
+    /**
+     * Speak a message asynchronously.
+     *
+     * @param {Person~sayCallback} cb
+     */
+    say: function(message, cb) {
+        if (!message) {
+            cb('You forgot the message!');
+        }
+
+        cb(null, this.name + ' says: ' + message);
+    }
+});

test/specs/documentation/virtual.js

@@ -1,16 +1,33 @@
-/*global describe: true, expect: true, it: true, jasmine: true */
+/*global describe, expect, it, jasmine */
-describe("virtual symbols", function() {
+describe('virtual symbols', function() {
+
+    describe('simple cases', function() {
         var docSet = jasmine.getDocSetFromFile('test/fixtures/virtual.js');
-    var found = [
+        var dimensions = docSet.getByLongname('dimensions');
-        docSet.getByLongname('dimensions'),
+        var width = docSet.getByLongname('width');
-        docSet.getByLongname('width')
-    ];
 
         it('should document virtual symbols', function() {
-        expect(found[0].length).toEqual(1);
+            expect(dimensions.length).toBe(1);
         });
 
         it('should document an undocumented symbol found after a comment for a virtual symbol', function() {
-        expect(found[1].length).toEqual(1);
+            expect(width.length).toBe(1);
+        });
+    });
+
+    describe('complex cases', function() {
+        var docSet = jasmine.getDocSetFromFile('test/fixtures/virtual2.js');
+        var say = docSet.getByLongname('Person#say')[0];
+        var sayCallback = docSet.getByLongname('Person~sayCallback')[0];
+
+        it('should document virtual symbols inside an object literal', function() {
+            expect(sayCallback).toBeDefined();
+            expect(sayCallback.undocumented).not.toBeDefined();
+        });
+
+        it('should attach the comment to a documented symbol that follows a virtual symbol', function() {
+            expect(say).toBeDefined();
+            expect(say.undocumented).not.toBeDefined();
+        });
     });
 });