Skip to content

Commit

Permalink
feat(lint): Identify explicit tags that don't match inference in lint…
Browse files Browse the repository at this point in the history
… stage

This will flag cases where people explicitly document things in JSDoc that don't reflect the code
reality - in many cases, misnamed parameter names in documentation tags.

Fixes #575
  • Loading branch information
tmcw committed May 1, 2017
1 parent 0894cc4 commit ed5c2a0
Show file tree
Hide file tree
Showing 14 changed files with 135 additions and 96 deletions.
16 changes: 1 addition & 15 deletions declarations/comment.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,7 @@ type CommentContextGitHub = {
url: string
};

type CommentTagBase = {
title: string
};

type CommentTag = CommentTagBase & {
name?: string,
title: string,
description?: Object,
default?: any,
lineNumber?: number,
type?: DoctrineType,
properties?: Array<CommentTag>
};

type CommentTagNamed = CommentTag & {
type CommentTag = {
name?: string,
title: string,
description?: Object,
Expand Down
19 changes: 9 additions & 10 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,16 @@ var fs = require('fs'),

/**
* Build a pipeline of comment handlers.
* @param {...Function|null} args - Pipeline elements. Each is a function that accepts
* @param {Array<Function>} fns - Pipeline elements. Each is a function that accepts
* a comment and can return a comment or undefined (to drop that comment).
* @returns {Function} pipeline
* @private
*/
function pipeline() {
var elements = arguments;
function pipeline(fns) {
return comment => {
for (var i = 0; comment && i < elements.length; i++) {
if (elements[i]) {
comment = elements[i](comment);
for (var i = 0; comment && i < fns.length; i++) {
if (fns[i]) {
comment = fns[i](comment);
}
}
return comment;
Expand Down Expand Up @@ -95,7 +94,7 @@ function buildInternal(inputsAndConfig) {

var parseFn = config.polyglot ? polyglot : parseJavaScript;

var buildPipeline = pipeline(
var buildPipeline = pipeline([
inferName,
inferAccess(config.inferPrivate),
inferAugments,
Expand All @@ -108,7 +107,7 @@ function buildInternal(inputsAndConfig) {
inferType,
config.github && github,
garbageCollect
);
]);

let extractedComments = _.flatMap(inputs, function(sourceFile) {
if (!sourceFile.source) {
Expand All @@ -130,7 +129,7 @@ function lintInternal(inputsAndConfig) {

let parseFn = config.polyglot ? polyglot : parseJavaScript;

let lintPipeline = pipeline(
let lintPipeline = pipeline([
lintComments,
inferName,
inferAccess(config.inferPrivate),
Expand All @@ -142,7 +141,7 @@ function lintInternal(inputsAndConfig) {
inferMembership(),
inferType,
nest
);
]);

let extractedComments = _.flatMap(inputs, sourceFile => {
if (!sourceFile.source) {
Expand Down
1 change: 0 additions & 1 deletion lib/infer/membership.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ function normalizeMemberof(comment /*: Comment*/) /*: Comment */ {
* annotations within a file
*
* @private
* @param {Object} comment parsed comment
* @returns {Object} comment with membership inferred
*/
module.exports = function() {
Expand Down
50 changes: 29 additions & 21 deletions lib/infer/params.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const _ = require('lodash');
const findTarget = require('./finders').findTarget;
const flowDoctrine = require('../flow_doctrine');
const util = require('util');
const debuglog = util.debuglog('documentation');

/**
* Infers param tags by reading function parameter names
Expand Down Expand Up @@ -45,17 +44,22 @@ function inferParams(comment /*: Comment */) {

function inferAndCombineParams(params, comment) {
var inferredParams = params.map((param, i) => paramToDoc(param, '', i));
var mergedParams = mergeTrees(inferredParams, comment.params);
var mergedParamsAndErrors = mergeTrees(inferredParams, comment.params);

// Then merge the trees. This is the hard part.
return _.assign(comment, {
params: mergedParams
params: mergedParamsAndErrors.mergedParams,
errors: comment.errors.concat(mergedParamsAndErrors.errors)
});
}

// Utility methods ============================================================
//
const PATH_SPLIT_CAPTURING = /(\[])?(\.)/g;
const PATH_SPLIT = /(?:\[])?\./g;
function tagDepth(tag /*: CommentTag */) /*: number */ {
return (tag.name || '').split(PATH_SPLIT).length;
}

/**
* Index tags by their `name` property into an ES6 map.
Expand Down Expand Up @@ -199,7 +203,7 @@ function paramToDoc(
};
default:
// (a)
var newParam /*: CommentTagNamed */ = {
var newParam /*: CommentTag*/ = {
title: 'param',
name: prefix ? prefixedName : param.name,
lineNumber: param.loc.start.line
Expand Down Expand Up @@ -261,25 +265,29 @@ function mergeTrees(inferred, explicit) {
function mergeTopNodes(inferred, explicit) {
const mapExplicit = mapTags(explicit);
const inferredNames = new Set(inferred.map(tag => tag.name));
const explicitTagsWithoutInference = explicit.filter(
tag => !inferredNames.has(tag.name)
);
const explicitTagsWithoutInference = explicit.filter(tag => {
return tagDepth(tag) === 1 && !inferredNames.has(tag.name);
});

if (explicitTagsWithoutInference.length) {
debuglog(
`${explicitTagsWithoutInference.length} tags were specified but didn't match ` +
`inferred information ${explicitTagsWithoutInference
.map(t => t.name)
.join(', ')}`
);
}
var errors = explicitTagsWithoutInference.map(tag => {
return {
message: `An explicit parameter named ${tag.name || ''} was specified but didn't match ` +
`inferred information ${Array.from(inferredNames).join(', ')}`,
commentLineNumber: tag.lineNumber
};
});

return inferred
.map(inferredTag => {
const explicitTag = mapExplicit.get(inferredTag.name);
return explicitTag ? combineTags(inferredTag, explicitTag) : inferredTag;
})
.concat(explicitTagsWithoutInference);
return {
errors,
mergedParams: inferred
.map(inferredTag => {
const explicitTag = mapExplicit.get(inferredTag.name);
return explicitTag
? combineTags(inferredTag, explicitTag)
: inferredTag;
})
.concat(explicitTagsWithoutInference)
};
}

// This method is used for _non-root_ properties only - we use mergeTopNodes
Expand Down
2 changes: 1 addition & 1 deletion lib/input/shallow.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var smartGlob = require('../smart_glob.js');
* or without fs access.
*
* @param indexes entry points
* @param options parsing options
* @param config parsing options
* @return promise with parsed files
*/
module.exports = function(
Expand Down
6 changes: 2 additions & 4 deletions lib/nest.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const PATH_SPLIT = /(?:\[])?\./g;

function removeUnnamedTags(
tags /*: Array<CommentTag> */
) /*: Array<CommentTagNamed> */ {
) /*: Array<CommentTag> */ {
return tags.filter(tag => typeof tag.name === 'string');
}

Expand All @@ -32,9 +32,7 @@ var tagDepth = tag => tag.name.split(PATH_SPLIT).length;
* \-> [].baz
*
* @private
* @param {Object} comment a comment with tags
* @param {string} tagTitle the tag to nest
* @param {string} target the tag to nest into
* @param {Array<CommentTag>} tags a list of tags
* @returns {Object} nested comment
*/
var nestTag = (
Expand Down
4 changes: 2 additions & 2 deletions lib/output/html.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ var mergeConfig = require('../merge_config');
* Formats documentation as HTML.
*
* @param comments parsed comments
* @param {Object} args Options that can customize the output
* @param {string} [args.theme='default_theme'] Name of a module used for an HTML theme.
* @param {Object} config Options that can customize the output
* @param {string} [config.theme='default_theme'] Name of a module used for an HTML theme.
* @returns {Promise<Array<Object>>} Promise with results
* @name formats.html
* @public
Expand Down
1 change: 0 additions & 1 deletion lib/output/util/formatters.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ module.exports = function(getHref /*: Function*/) {
/**
* Link text to this page or to a central resource.
* @param {string} text inner text of the link
* @param {string} description link text override
* @returns {string} potentially linked HTML
*/
formatters.autolink = function(text /*: string*/) {
Expand Down
11 changes: 11 additions & 0 deletions lib/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,17 @@ function parseJSDoc(
result.description = parseMarkdown(result.description);
}

// Reject parameter tags without a parameter name
result.tags.filter(function(tag) {
if (tag.title === 'param' && tag.name === undefined) {
result.errors.push({
message: 'A @param tag without a parameter name was rejected'
});
return false;
}
return true;
});

result.tags.forEach(function(tag) {
if (tag.errors) {
for (var j = 0; j < tag.errors.length; j++) {
Expand Down
2 changes: 1 addition & 1 deletion lib/parsers/polyglot.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ var getComments = require('get-comments'),
* Documentation stream parser: this receives a module-dep item,
* reads the file, parses the JavaScript, parses the JSDoc, and
* emits parsed comments.
* @param {Object} data a chunk of data provided by module-deps
* @param sourceFile a chunk of data provided by module-deps
* @return {Array<Object>} adds to memo
*/
function parsePolyglot(sourceFile /*: SourceFile*/) {
Expand Down
6 changes: 6 additions & 0 deletions test/fixture/lint/lint.input.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* @param {Array<Number>} foo bar
* @param {Array<Number>} foo bar
* @param {Array|Number} foo bar
* @param {boolean}
* @returns {object} bad object return type
* @type {Array<object>} bad object type
* @memberof notfound
Expand All @@ -13,3 +14,8 @@
* @property {String} bad property
* @private
*/

/**
* @param {number} c explicit but not found
*/
function add(a, b) {}
17 changes: 11 additions & 6 deletions test/fixture/lint/lint.output
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
2:1 warning type String found, string is standard
2:1 warning An explicit parameter named foo was specified but didn't match inferred information a, b
3:1 warning type Number found, number is standard
3:1 warning An explicit parameter named foo was specified but didn't match inferred information a, b
4:1 warning type Number found, number is standard
4:1 warning An explicit parameter named foo was specified but didn't match inferred information a, b
5:1 warning type Number found, number is standard
6:1 warning type object found, Object is standard
5:1 warning An explicit parameter named foo was specified but didn't match inferred information a, b
6:1 warning An explicit parameter named boolean was specified but didn't match inferred information a, b
7:1 warning type object found, Object is standard
8:1 warning @memberof reference to notfound not found
11:1 warning could not determine @name for hierarchy
12:1 warning type String found, string is standard
8:1 warning type object found, Object is standard
9:1 warning @memberof reference to notfound not found
13:1 warning type String found, string is standard
13:1 warning An explicit parameter named baz was specified but didn't match inferred information a, b
14:1 warning type String found, string is standard
19:1 warning An explicit parameter named c was specified but didn't match inferred information a, b

11 warnings
16 warnings
19 changes: 18 additions & 1 deletion test/fixture/params.output.json
Original file line number Diff line number Diff line change
Expand Up @@ -1212,7 +1212,24 @@
}
},
"augments": [],
"errors": [],
"errors": [
{
"message": "An explicit parameter named address was specified but didn't match inferred information ",
"commentLineNumber": 5
},
{
"message": "An explicit parameter named groups was specified but didn't match inferred information ",
"commentLineNumber": 6
},
{
"message": "An explicit parameter named third was specified but didn't match inferred information ",
"commentLineNumber": 7
},
{
"message": "An explicit parameter named foo was specified but didn't match inferred information ",
"commentLineNumber": 8
}
],
"examples": [
{
"description": "var address = new Address6('2001::/32');"
Expand Down
Loading

0 comments on commit ed5c2a0

Please sign in to comment.