Skip to content

feat(lint): Identify explicit tags that don't match inference in lint… #764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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