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.

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

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

Merged
77 changes: 77 additions & 0 deletions docs/rules/func-names.md
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
93 changes: 73 additions & 20 deletions lib/rules/func-names.js
Expand Up @@ -33,20 +33,55 @@ 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}}."
}
},

create(context) {
const never = context.options[0] === "never";
const asNeeded = context.options[0] === "as-needed";

/**
* Returns the config option for the given node.
* @param {ASTNode} node - A node to get the config for.
* @returns {string} The config option.
*/
function getConfigForNode(node) {
if (
node.generator &&
context.options.length > 1 &&
context.options[1].generators
) {
return context.options[1].generators;
}

return context.options[0] || "always";
}

/**
* Determines whether the current FunctionExpression node is a get, set, or
Expand Down Expand Up @@ -83,6 +118,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 +155,15 @@ module.exports = {
}

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

if (never) {
if (config === "never") {
if (hasName) {
context.report({
node,
messageId: "named",
data: { name }
});
reportUnexpectedNamedFunction(node);
}
} else {
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(node))) {
context.report({
node,
messageId: "unnamed",
data: { name }
});
if (!hasName && (config === "as-needed" ? !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.

Could this be simplified to something like:

} else if (config === "as-needed") {
    if (!hasName && !hasInferredName(node)) {
        reportUnexpectedUnnamedFunction(node);
    }
} else {
    if (!isObjectOrClassMethod(node)) {
        reportUnexpectedUnnamedFunction(node);
    }
}

In my opinion, the expression !hasName && (config === "as-needed" ? !hasInferredName(node) : !isObjectOrClassMethod(node)) is fairly difficult to read and understand (most of this complexity was already there before this change).

reportUnexpectedUnnamedFunction(node);
}
}
}
Expand Down