From f635b10b6f4fb7d3f53f436810c8d6aee0018a23 Mon Sep 17 00:00:00 2001 From: Jeff Williams Date: Fri, 25 Apr 2014 21:46:58 -0700 Subject: [PATCH] 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 | 73 +++++++---------------------- lib/jsdoc/src/walker.js | 2 +- test/fixtures/virtual2.js | 28 +++++++++++ test/specs/documentation/virtual.js | 39 ++++++++++----- 4 files changed, 73 insertions(+), 69 deletions(-) create mode 100644 test/fixtures/virtual2.js diff --git a/lib/jsdoc/src/astbuilder.js b/lib/jsdoc/src/astbuilder.js index ed78c9d8..4b065974 100644 --- a/lib/jsdoc/src/astbuilder.js +++ b/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; - - // 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); + var position = node.range[0]; + var commentIndex = this._nextIndexBefore(0, this._comments, position); // all comments before the node (except the last one) are considered stray comments if (commentIndex > 0) { @@ -299,15 +270,17 @@ CommentAttacher.prototype._fastForwardComments = function(node) { CommentAttacher.prototype._attachPendingComment = function() { var target; - if (this._pendingComment) { - if (this._candidates.length > 0) { - target = this._candidates[this._candidates.length - 1]; - target.leadingComments = target.leadingComments || []; - target.leadingComments.push(this._pendingComment); - } - else { - this._strayComments.push(this._pendingComment); - } + if (!this._pendingComment) { + return this; + } + + if (this._candidates.length > 0) { + target = this._candidates[this._candidates.length - 1]; + target.leadingComments = target.leadingComments || []; + target.leadingComments.push(this._pendingComment); + } + else { + this._strayComments.push(this._pendingComment); } this._resetPendingComment() @@ -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 - // comment target, try attaching the comment - if ( (this._pendingComment && !isWithin(node.range, this._pendingCommentRange)) || - !searchDescendants[node.type] ) { - this._attachPendingComment(); - } + // attach the pending comment, if there is one + 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 diff --git a/lib/jsdoc/src/walker.js b/lib/jsdoc/src/walker.js index 5de97c9a..ce724c23 100644 --- a/lib/jsdoc/src/walker.js +++ b/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) { diff --git a/test/fixtures/virtual2.js b/test/fixtures/virtual2.js new file mode 100644 index 00000000..329b2ab3 --- /dev/null +++ b/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); + } +}); diff --git a/test/specs/documentation/virtual.js b/test/specs/documentation/virtual.js index 73b73913..4809c99b 100644 --- a/test/specs/documentation/virtual.js +++ b/test/specs/documentation/virtual.js @@ -1,16 +1,33 @@ -/*global describe: true, expect: true, it: true, jasmine: true */ -describe("virtual symbols", function() { - var docSet = jasmine.getDocSetFromFile('test/fixtures/virtual.js'); - var found = [ - docSet.getByLongname('dimensions'), - docSet.getByLongname('width') - ]; +/*global describe, expect, it, jasmine */ +describe('virtual symbols', function() { - it('should document virtual symbols', function() { - expect(found[0].length).toEqual(1); + describe('simple cases', function() { + var docSet = jasmine.getDocSetFromFile('test/fixtures/virtual.js'); + var dimensions = docSet.getByLongname('dimensions'); + var width = docSet.getByLongname('width'); + + it('should document virtual symbols', function() { + expect(dimensions.length).toBe(1); + }); + + it('should document an undocumented symbol found after a comment for a virtual symbol', function() { + expect(width.length).toBe(1); + }); }); - it('should document an undocumented symbol found after a comment for a virtual symbol', function() { - expect(found[1].length).toEqual(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(); + }); }); });