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
Merged
kaicataldo
merged 24 commits into
eslint:master
from
aladdin-add:feature/enforce-return-in-get
Jun 27, 2017
Merged
New rule: getter-return #8460
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
de3427b
New: getter-return (#8449)
aladdin-add 9723ee0
add more test
aladdin-add 98f9e7c
add test
8983a83
Docs: for rule getter-return
b622d92
add option: noImplicit
aladdin-add 6c25dbe
1.update category.
aladdin-add 6bc00fd
add more tests: IIFE...
aladdin-add 40b1c8f
update: change option name & default val
aladdin-add c53493a
docs: fix typo.
aladdin-add fec1d29
update: additionalProperties & docs option
aladdin-add 926356c
fix: wrong option behavior & update tests
aladdin-add b2ec492
fix comment.
aladdin-add b6bbdb8
del dulp test.
aladdin-add dd47811
add more invalid tests.
aladdin-add 5aa8c5b
add more tests.
aladdin-add 8fbaa74
and more.
aladdin-add cbffeff
update: reg => string.
aladdin-add 64847d3
docs.
aladdin-add fbf2702
rename func name.
aladdin-add 396625a
set default parser option: ecmaVersion 6
aladdin-add a170476
update
aladdin-add 9da8b40
docs: add option example.
aladdin-add 17d35f8
update msg. & tests.
aladdin-add d5b172e
update
aladdin-add File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
# 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 returning undefined with a return; statement. | ||
|
||
Examples of **correct** code for the `{ "allowImplicit": true }` option: | ||
|
||
```js | ||
/*eslint getter-return: ["error", { allowImplicit: true }]*/ | ||
p = { | ||
get name(){ | ||
return; // return undefined implicitly. | ||
} | ||
}; | ||
``` | ||
|
||
## When Not To Use It | ||
|
||
If your project will not be using ES5 property getters you do not need this rule. | ||
|
||
## 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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,151 @@ | ||
/** | ||
* @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 | ||
//------------------------------------------------------------------------------ | ||
|
||
/** | ||
* 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 getId(node) { | ||
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", | ||
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: getId(node).loc.start, | ||
message: funcInfo.hasReturn | ||
? "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: | ||
node.type === "FunctionExpression" && | ||
node.body.type === "BlockStatement" && | ||
|
||
// check if it is a "getter", or a method named "get". | ||
(parent.kind === "get" || astUtils.getStaticPropertyName(parent) === "get"), | ||
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: false, should also check node.argument | ||
if (!options.allowImplicit && !node.argument) { | ||
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 | ||
}; | ||
} | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/** | ||
* @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({ parserOptions: { ecmaVersion: 6 } }); | ||
|
||
// 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 ${name} to always return a value.`; | ||
const options = [{ allowImplicit: true }]; | ||
|
||
ruleTester.run("getter-return", rule, { | ||
|
||
valid: [ | ||
|
||
// test obj: get | ||
// option: {allowImplicit: false} | ||
{ code: "var foo = { get bar(){return true;} };" }, | ||
|
||
// option: {allowImplicit: true} | ||
{ code: "var foo = { get bar() {return;} };", options }, | ||
{ code: "var foo = { get bar(){return true;} };", options }, | ||
{ code: "var foo = { get bar(){if(bar) {return;} return true;} };", options }, | ||
|
||
// test class: get | ||
// option: {allowImplicit: false} | ||
{ code: "class foo { get bar(){return true;} }" }, | ||
{ code: "class foo { get bar(){if(baz){return true;} else {return false;} } }" }, | ||
{ code: "class foo { get(){return true;} }" }, | ||
|
||
// option: {allowImplicit: true} | ||
{ code: "class foo { get bar(){return true;} }", options }, | ||
{ code: "class foo { get bar(){return;} }", options }, | ||
|
||
// test object.defineProperty(s) | ||
// option: {allowImplicit: false} | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () {return true;}});" }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () { ~function (){ return true; }();return true;}});" }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return true;}} });" }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () { ~function (){ return true; }(); return true;}} });" }, | ||
|
||
// option: {allowImplicit: true} | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function () {return true;}});", options }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){return;}});", options }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return true;}} });", options }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {return;}} });", options }, | ||
|
||
// not getter. | ||
{ code: "var get = function(){};" }, | ||
{ code: "var get = function(){ return true; };" }, | ||
{ code: "var foo = { bar(){} };" }, | ||
{ code: "var foo = { bar(){ return true; } };" }, | ||
{ code: "var foo = { bar: function(){} };" }, | ||
{ code: "var foo = { bar(){ return true; } };" }, | ||
{ code: "var foo = { bar: function(){return;} };" }, | ||
{ code: "var foo = { bar: function(){return true;} };" } | ||
], | ||
|
||
invalid: [ | ||
|
||
// test obj: get | ||
// option: {allowImplicit: false} | ||
{ code: "var foo = { get bar() {} };", errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar(){if(baz) {return true;}} };", errors: [{ message: noLastReturnMessage }] }, | ||
{ code: "var foo = { get bar() { ~function () {return true;}} };", errors: [{ message: noReturnMessage }] }, | ||
|
||
// option: {allowImplicit: true} | ||
{ code: "var foo = { get bar() {} };", options, errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar() {if (baz) {return;}} };", options, errors: [{ message: noLastReturnMessage }] }, | ||
|
||
// test class: get | ||
// option: {allowImplicit: false} | ||
{ code: "class foo { get bar(){} }", errors: [{ message: noReturnMessage }] }, | ||
{ code: "class foo { get bar(){ if (baz) { return true; }}}", errors: [{ noLastReturnMessage }] }, | ||
{ code: "class foo { get bar(){ ~function () { return true; }()}}", errors: [{ noLastReturnMessage }] }, | ||
|
||
// option: {allowImplicit: true} | ||
{ code: "class foo { get bar(){} }", options, errors: [{ message: noReturnMessage }] }, | ||
{ code: "class foo { get bar(){if (baz) {return true;} } }", options, errors: [{ message: noLastReturnMessage }] }, | ||
|
||
// test object.defineProperty(s) | ||
// option: {allowImplicit: false} | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){}});", errors: [{ noReturnMessage }] }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){if(bar) {return true;}}});", errors: [{ message: "Expected method 'get' to always return a value." }] }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){ ~function () { return true; }()}});", errors: [{ noReturnMessage }] }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {}} });", options, errors: [{ noReturnMessage }] }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function (){if(bar) {return true;}}}});", options, errors: [{ message: "Expected method 'get' to always return a value." }] }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {~function () { return true; }()}} });", options, errors: [{ noReturnMessage }] }, | ||
|
||
// option: {allowImplicit: true} | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){}});", options, errors: [{ message: "Expected to return a value in method 'get'." }] }, | ||
{ code: "Object.defineProperies(foo, { bar: { get: function () {}} });", options, errors: [{ noReturnMessage }] } | ||
] | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this object should probably have
additionalProperties: false
set on it