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
New rule: getter-return #8460
New rule: getter-return #8460
Changes from 10 commits
de3427b
9723ee0
98f9e7c
8983a83
b622d92
6c25dbe
6bc00fd
40b1c8f
c53493a
fec1d29
926356c
b2ec492
b6bbdb8
dd47811
5aa8c5b
8fbaa74
cbffeff
64847d3
fbf2702
396625a
a170476
9da8b40
17d35f8
d5b172e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
# Enforces that a return statement is present in property getters (getter-return) | ||
|
||
The get syntax binds an object property to a function that will be called when that property is looked up. It was first introduced in ECMAScript 5: | ||
|
||
```js | ||
var p = { | ||
get name(){ | ||
return "nicholas"; | ||
} | ||
}; | ||
|
||
Object.defineProperty(p, "age", { | ||
get: function (){ | ||
return 17; | ||
} | ||
}); | ||
``` | ||
|
||
Note that every `getter` is expected to return a value. | ||
|
||
## Rule Details | ||
|
||
This rule enforces that a return statement is present in property getters. | ||
|
||
Examples of **incorrect** code for this rule: | ||
|
||
```js | ||
/*eslint getter-return: "error"*/ | ||
|
||
p = { | ||
get name(){ | ||
// no returns. | ||
} | ||
}; | ||
|
||
Object.defineProperty(p, "age", { | ||
get: function (){ | ||
// no returns. | ||
} | ||
}); | ||
|
||
class P{ | ||
get name(){ | ||
// no returns. | ||
} | ||
} | ||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```js | ||
/*eslint getter-return: "error"*/ | ||
|
||
p = { | ||
get name(){ | ||
return "nicholas"; | ||
} | ||
}; | ||
|
||
Object.defineProperty(p, "age", { | ||
get: function (){ | ||
return 18; | ||
} | ||
}); | ||
|
||
class P{ | ||
get name(){ | ||
return "nicholas"; | ||
} | ||
} | ||
``` | ||
|
||
## Options | ||
|
||
This rule has an object option: | ||
|
||
* `"allowImplicit": false` (default) disallows implicitly return a value. | ||
|
||
## When Not To Use It | ||
|
||
If your project will not be using getter you do not need this rule. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: This should say "if your project will not be using getters". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might even call them "ES5 property getters" to distinguish them from generic getFoo methods. |
||
|
||
## Further Reading | ||
|
||
* [MDN: Functions getter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get) | ||
* [Understanding ES6: Accessor Properties](https://leanpub.com/understandinges6/read/#leanpub-auto-accessor-properties) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
/** | ||
* @fileoverview Enforces that a return statement is present in property getters. | ||
* @author Aladdin-ADD(hh_2013@foxmail.com) | ||
*/ | ||
|
||
"use strict"; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Requirements | ||
//------------------------------------------------------------------------------ | ||
|
||
const astUtils = require("../ast-utils"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Helpers | ||
//------------------------------------------------------------------------------ | ||
|
||
const TARGET_NODE_TYPE = /^FunctionExpression$/; | ||
|
||
/** | ||
* Checks a given code path segment is reachable. | ||
* | ||
* @param {CodePathSegment} segment - A segment to check. | ||
* @returns {boolean} `true` if the segment is reachable. | ||
*/ | ||
function isReachable(segment) { | ||
return segment.reachable; | ||
} | ||
|
||
/** | ||
* Gets a readable location. | ||
* | ||
* - FunctionExpression -> the function name or `function` keyword. | ||
* | ||
* @param {ASTNode} node - A function node to get. | ||
* @returns {ASTNode|Token} The node or the token of a location. | ||
*/ | ||
function getLocation(node) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you rename this function to something like |
||
return node.id || node; | ||
} | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: "enforce `return` statements in getters", | ||
category: "Possible Errors", | ||
recommended: false | ||
}, | ||
fixable: null, | ||
schema: [ | ||
{ | ||
type: "object", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this object should probably have |
||
properties: { | ||
allowImplicit: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add documentation for this option :-) |
||
type: "boolean" | ||
} | ||
}, | ||
additionalProperties: false | ||
} | ||
] | ||
}, | ||
|
||
create(context) { | ||
|
||
const options = context.options[0] || { allowImplicit: false }; | ||
|
||
let funcInfo = { | ||
upper: null, | ||
codePath: null, | ||
hasReturn: false, | ||
shouldCheck: false, | ||
node: null | ||
}; | ||
|
||
/** | ||
* Checks whether or not the last code path segment is reachable. | ||
* Then reports this function if the segment is reachable. | ||
* | ||
* If the last code path segment is reachable, there are paths which are not | ||
* returned or thrown. | ||
* | ||
* @param {ASTNode} node - A node to check. | ||
* @returns {void} | ||
*/ | ||
function checkLastSegment(node) { | ||
if (funcInfo.shouldCheck && | ||
funcInfo.codePath.currentSegments.some(isReachable) | ||
) { | ||
context.report({ | ||
node, | ||
loc: getLocation(node, context.getSourceCode()).loc.start, | ||
message: funcInfo.hasReturn | ||
? "Expected to return a value at the end of {{name}}." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this message fits for some cases: {
get foo() {
if (bar) {
return true;
} else if (baz) {
doSomething();
// missing return here
} else {
return false;
}
}
} In the example above, the return isn't missing at the end of the function. I think a better message might be something like this: "Expected to return a value from all code paths of {{name}}." But there might be even better options. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about "Expected {{name}} to always return a value."? |
||
: "Expected to return a value in {{name}}.", | ||
data: { | ||
name: astUtils.getFunctionNameWithKind(funcInfo.node) | ||
} | ||
}); | ||
} | ||
} | ||
|
||
return { | ||
|
||
// Stacks this function's information. | ||
onCodePathStart(codePath, node) { | ||
const parent = node.parent; | ||
|
||
funcInfo = { | ||
upper: funcInfo, | ||
codePath, | ||
hasReturn: false, | ||
shouldCheck: | ||
TARGET_NODE_TYPE.test(node.type) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Since |
||
node.body.type === "BlockStatement" && | ||
|
||
// check if it is a "getter", or a method named "get". | ||
(parent.kind === "get" || astUtils.getStaticPropertyName(parent) === "get"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for code like this: foo.__defineGetter__("bar", function baz() { return ""; });
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a method named |
||
node | ||
}; | ||
}, | ||
|
||
// Pops this function's information. | ||
onCodePathEnd() { | ||
funcInfo = funcInfo.upper; | ||
}, | ||
|
||
// Checks the return statement is valid. | ||
ReturnStatement(node) { | ||
if (funcInfo.shouldCheck) { | ||
funcInfo.hasReturn = true; | ||
|
||
// if allowImplicit: true, should also check node.argument | ||
if (options.allowImplicit && !node.argument) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this conditional should actually be if (!options.allowImplicit && !node.argument) { We want it it warn when we disallow implicit return values, but this would actually warn when we allow implicit return values and there isn't one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To see this behavior, try the following test with the current behavior: // test option: {allowImplicit: true}
{ code: "var foo = { get bar(){return;} };", options }, I think we should add this as a regression test once this is fixed. Good catch on this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, in previous implement, the default option is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for working on this :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, no worries about having multiple commits in a PR |
||
context.report({ | ||
node, | ||
message: "Expected to return a value in {{name}}.", | ||
data: { | ||
name: astUtils.getFunctionNameWithKind(funcInfo.node) | ||
} | ||
}); | ||
} | ||
} | ||
}, | ||
|
||
// Reports a given function if the last path is reachable. | ||
"FunctionExpression:exit": checkLastSegment | ||
}; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
/** | ||
* @fileoverview Enforces that a return statement is present in property getters. | ||
* @author Aladdin-ADD(hh_2013@foxmail.com) | ||
*/ | ||
|
||
"use strict"; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Requirements | ||
//------------------------------------------------------------------------------ | ||
|
||
const rule = require("../../../lib/rules/getter-return"); | ||
const RuleTester = require("../../../lib/testers/rule-tester"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Tests | ||
//------------------------------------------------------------------------------ | ||
|
||
const ruleTester = new RuleTester(); | ||
|
||
// data is not working, so specify a name: "getter 'bar'" | ||
const name = "getter 'bar'"; | ||
const noReturnMessage = `Expected to return a value in ${name}.`; | ||
const noLastReturnMessage = `Expected to return a value at the end of ${name}.`; | ||
const parserOptions = { ecmaVersion: 6 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Instead of using const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } }); Then you won't need to repeat |
||
const options = [{ allowImplicit: true }]; | ||
|
||
ruleTester.run("getter-return", rule, { | ||
|
||
valid: [ | ||
|
||
// test obj: get, option: {allowImplicit: false} | ||
{ code: "var foo = { get bar(){return true;} };" }, | ||
{ code: "var foo = { get bar(){return;} };" }, | ||
{ code: "var foo = { bar: function(){return true;} };" }, | ||
{ code: "var foo = { bar: function(){return;} };" }, | ||
{ code: "var foo = { bar(){return true;} };", parserOptions }, | ||
{ code: "var foo = { bar(){return;} };", parserOptions }, | ||
{ code: "var foo = { bar(){~function (){}();return;} };", parserOptions }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually have never seen this before. Mind explaining it to me? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. based on above case, I just add a IIFE, to test a nested function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the explanation! This is why I love working on ESLint - I get to learn all sorts of things I wouldn't encounter at my day job :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find a mistake, the default option is allowImplicit : false, so it means we cannot return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the case then a number of these test cases should be invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'm a bit confused - line 36, for instance, should be a failing test, right? |
||
{ code: "var foo = { bar(){~function (){return true;}();return;} };", parserOptions }, | ||
|
||
// test class: get, option: {allowImplicit: false} | ||
{ code: "class foo { get bar(){return true;} }", parserOptions }, | ||
{ code: "class foo { get bar(){if(baz){return true;} else {return false;} } }", parserOptions }, | ||
{ code: "class foo { get bar(){if(baz){return;} else {return false;} } }", parserOptions }, | ||
{ code: "class foo { get(){return true;} }", parserOptions }, | ||
{ code: "var foo = { get bar(){if(bar) {return;} return true;} };", parserOptions, errors: [] }, | ||
{ code: "var foo = { get bar(){ ~function (){ return true; }(); return; } };", parserOptions, errors: [] }, | ||
{ code: "var foo = { get bar(){ ~function (){}(); return; } };", parserOptions, errors: [] }, | ||
|
||
// test object.defineProperty(s), option: {allowImplicit: false} | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () {return true;}});" }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () {return;}});" }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return true;}} });" }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return;}} });" }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () { ~function (){ return true; }();return true;}});" }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () { ~function (){}();return;}});" }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () { ~function (){ return true; }(); return true;}} });" }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () { ~function (){}(); return;}} });" }, | ||
|
||
|
||
// test option: {allowImplicit: true} | ||
{ code: "var foo = { get bar(){return true;} };", options }, | ||
{ code: "class foo { get bar(){return true;} }", options, parserOptions }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () {return true;}});", options }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return true;}} });", options }, | ||
|
||
// not getter. | ||
{ code: "var get = function(){};" }, | ||
{ code: "var get = function(){ return true; };" }, | ||
{ code: "var foo = { bar: function(){} };" }, | ||
{ code: "var foo = { bar: function(){ return true; } };" }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test seems identical to line 33? |
||
{ code: "var foo = { bar(){} };", parserOptions }, | ||
{ code: "var foo = { bar(){ return true; } };", parserOptions } | ||
], | ||
|
||
invalid: [ | ||
|
||
// test obj: get, option: {allowImplicit: false} | ||
{ code: "var foo = { get bar() {} };", parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return true;}} };", parserOptions, errors: [{ message: noLastReturnMessage }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return true;} ;} };", parserOptions, errors: [{ message: noLastReturnMessage }] }, | ||
|
||
// test class: get, option: {allowImplicit: false} | ||
{ code: "class foo { get bar(){} }", parserOptions, errors: [{ message: noReturnMessage }] }, | ||
|
||
// test object.defineProperty, option: {allowImplicit: false} | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){if(bar) {return true;}}});", errors: [{ message: "Expected to return a value at the end of method 'get'." }] }, | ||
|
||
// test option: {allowImplicit: true} | ||
{ code: "var foo = { get bar() {return;} };", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar() {return; ;} };", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar() {return; return 1;} };", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return true;} return;} };", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return;} return true;} };", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "class foo { get bar(){return;} }", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "class foo { get bar(){if(bar) {return true;}} }", options, parserOptions, errors: [{ message: noLastReturnMessage }] }, | ||
{ code: "class foo { get bar(){if(bar) {return;} return true;} }", options, parserOptions, errors: [{ message: noReturnMessage }] }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){return;}});", options, errors: [{ message: "Expected to return a value in method 'get'." }] }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){if(bar) {return;} return true;}});", options, errors: [{ message: "Expected to return a value in method 'get'." }] }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return;}} });", options, errors: [{ message: "Expected to return a value in method 'get'." }] }, | ||
|
||
{ code: "class foo { get bar(){} }", options, parserOptions, errors: [{ message: noReturnMessage }] } | ||
|
||
] | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add examples for this option?
Also, I think it would be a good idea to clarify the option description, e.g. "...disallows implicitly returning
undefined
with areturn;
statement".