Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update: Added generators option to func-names (fixes #9511) #10697

Merged
merged 9 commits into from Sep 5, 2018
Merged

Update: Added generators option to func-names (fixes #9511) #10697

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
109faf2
Update: Added generators option to func-names (fixes #9511)
OscarBarrett Jul 26, 2018
13a8c74
Updated option schema to match base and added alternative object schema
OscarBarrett Aug 6, 2018
7d845ae
Removed object schema
OscarBarrett Aug 6, 2018
67867f7
Lint fix: no-prototype-builtins
OscarBarrett Aug 28, 2018
0950db3
Handle as-needed case for generators
OscarBarrett Aug 28, 2018
df0be72
Merge branch 'master' into feature/func-names-generators
OscarBarrett Aug 28, 2018
2f087c5
Added additional test cases and updated docs for as-needed generators
OscarBarrett Aug 28, 2018
c54e1f7
Simplified logic
OscarBarrett Sep 4, 2018
41b828a
Improve readability of "as-needed" and "always" checks
OscarBarrett Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 77 additions & 0 deletions docs/rules/func-names.md
View file
Open in desktop
Expand Up @@ -20,6 +20,15 @@ This rule has a string option:
* `"as-needed"` requires function expressions to have a name, if the name cannot be assigned automatically in an ES6 environment
* `"never"` disallows named function expressions, except in recursive functions, where a name is needed

This rule has an object option:

* `"generators": "always" | "as-needed" | "never"`
* `"always"` require named generators
* `"as-needed"` require named generators if the name cannot be assigned automatically in an ES6 environment.
* `"never"` disallow named generators where possible.

When a value for `generators` is not provided the behavior for generator functions falls back to the base option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of true and false for this case (meaning "use the base option" and "invert the base option") is a bit unintuitive to me. Since there three possible base options (always, as-needed, and never) and only two possible generators options, a boolean generators option isn't able to express the full space of possible behaviors here. It also seems like the wording of "require generators to follow the func-names rule" is self-referential since the option itself is describing what it means for a generator to follow the func-names rule.

What would you think about having the generators option be a string with the same possible values as the regular option? This seems like it could reduce the cognitive overhead when configuring the option, because the generators option would behave the same way as the base option. For example:

{
    "rules": {
        "func-names": ["error", "never", {"generators": "as-needed"}]
    }
}

With this scheme, the default behavior of the generators option would be to fall back to the base option.

Another possible change could be to make the first option an object, e.g.

{
    "rules": {
        "func-names": [{"base": "never", "generators": "as-needed"}]
    }
}

With this setup, it might be clearer how the generators and base options interact with each other, without reading the documentation. (Note that we would still need to support the string option for backwards compatibility.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense. I'll look at updating the schema as suggested.

### always

Examples of **incorrect** code for this rule with the default `"always"` option:
Expand Down Expand Up @@ -100,6 +109,74 @@ Foo.prototype.bar = function() {};
}())
```

### generators

Examples of **incorrect** code for this rule with the `"always", { "generators": "as-needed" }` options:

```js
/*eslint func-names: ["error", "always", { "generators": "as-needed" }]*/

(function*() {
// ...
}())
```

Examples of **correct** code for this rule with the `"always", { "generators": "as-needed" }` options:

```js
/*eslint func-names: ["error", "always", { "generators": "as-needed" }]*/

var foo = function*() {};
```

Examples of **incorrect** code for this rule with the `"always", { "generators": "never" }` options:

```js
/*eslint func-names: ["error", "always", { "generators": "never" }]*/

var foo = bar(function *baz() {});
```

Examples of **correct** code for this rule with the `"always", { "generators": "never" }` options:

```js
/*eslint func-names: ["error", "always", { "generators": "never" }]*/

var foo = bar(function *() {});
```

Examples of **incorrect** code for this rule with the `"as-needed", { "generators": "never" }` options:

```js
/*eslint func-names: ["error", "as-needed", { "generators": "never" }]*/

var foo = bar(function *baz() {});
```

Examples of **correct** code for this rule with the `"as-needed", { "generators": "never" }` options:

```js
/*eslint func-names: ["error", "as-needed", { "generators": "never" }]*/

var foo = bar(function *() {});
```

Examples of **incorrect** code for this rule with the `"never", { "generators": "always" }` options:

```js
/*eslint func-names: ["error", "never", { "generators": "always" }]*/

var foo = bar(function *() {});
```

Examples of **correct** code for this rule with the `"never", { "generators": "always" }` options:

```js
/*eslint func-names: ["error", "never", { "generators": "always" }]*/

var foo = bar(function *baz() {});
```

## Further Reading

* [Functions Explained](http://markdaggett.com/blog/2013/02/15/functions-explained/)
Expand Down
94 changes: 74 additions & 20 deletions lib/rules/func-names.js
View file
Open in desktop
Expand Up @@ -33,11 +33,31 @@ module.exports = {
url: "https://eslint.org/docs/rules/func-names"
},

schema: [
{
enum: ["always", "as-needed", "never"]
}
],
schema: {
definitions: {
value: {
enum: [
"always",
"as-needed",
"never"
]
}
},
items: [
{
$ref: "#/definitions/value"
},
{
type: "object",
properties: {
generators: {
$ref: "#/definitions/value"
}
},
additionalProperties: false
}
]
},
messages: {
unnamed: "Unexpected unnamed {{name}}.",
named: "Unexpected named {{name}}."
Expand All @@ -47,6 +67,13 @@ module.exports = {
create(context) {
const never = context.options[0] === "never";
const asNeeded = context.options[0] === "as-needed";
let requireNamedGenerators = !never;
let asNeededGenerators = asNeeded;

if (context.options[1] && Object.prototype.hasOwnProperty.call(context.options[1], "generators")) {
requireNamedGenerators = context.options[1].generators !== "never";
asNeededGenerators = context.options[1].generators === "as-needed";
}

/**
* Determines whether the current FunctionExpression node is a get, set, or
Expand Down Expand Up @@ -83,6 +110,32 @@ module.exports = {
(parent.type === "AssignmentPattern" && parent.right === node);
}

/**
* Reports that an unnamed function should be named
* @param {ASTNode} node - The node to report in the event of an error.
* @returns {void}
*/
function reportUnexpectedUnnamedFunction(node) {
context.report({
node,
messageId: "unnamed",
data: { name: astUtils.getFunctionNameWithKind(node) }
});
}

/**
* Reports that a named function should be unnamed
* @param {ASTNode} node - The node to report in the event of an error.
* @returns {void}
*/
function reportUnexpectedNamedFunction(node) {
context.report({
node,
messageId: "named",
data: { name: astUtils.getFunctionNameWithKind(node) }
});
}

return {
"FunctionExpression:exit"(node) {

Expand All @@ -94,23 +147,24 @@ module.exports = {
}

const hasName = Boolean(node.id && node.id.name);
const name = astUtils.getFunctionNameWithKind(node);

if (never) {
if (hasName) {
context.report({
node,
messageId: "named",
data: { name }
});

if (node.generator) {
const failsAsNeededRequirement = asNeededGenerators ? !hasInferredName(node) : !isObjectOrClassMethod(node);

if (requireNamedGenerators && !hasName && failsAsNeededRequirement) {
reportUnexpectedUnnamedFunction(node);
} else if (!requireNamedGenerators && hasName) {
reportUnexpectedNamedFunction(node);
}
} else {
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(node))) {
context.report({
node,
messageId: "unnamed",
data: { name }
});
if (never) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the branching here makes this code a bit difficult to read; there is similar logic for both regular and generator functions, but the code ends up duplicated.

Would it be possible to have something like a getConfigForNode function that accepts a function AST node and returns the appropriate config for it? For example:

function (node) {
    if (
        node.generator &&
        context.options.length > 1 &&
        context.options[1].generators
    ) {
        return context.options[1].generators;
    } else {
        return context.options[0] || "always";
    }
}

Then the "FunctionExpression:exit" listener would just perform the appropriate check based on the result of calling getConfigForNode, without needing to have separate cases for when the given node is a generator. (This would also make the never, asNeeded, requireNamedGenerators, and asNeededGenerators options unnecessary.)

if (hasName) {
reportUnexpectedNamedFunction(node);
}
} else {
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(node))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a bug here where the as-needed option never gets used for generators. For example, based on reading this I think the rule will incorrectly report an error for this code:

/* eslint func-names: [error, always, {generators: as-needed}] */

var foo = function*() {};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - I missed this case. I've updated the PR to handle it properly.

reportUnexpectedUnnamedFunction(node);
}
}
}
}
Expand Down