-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Conversation
@aladdin-add, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @platinumazure and @kaicataldo to be potential reviewers. |
LGTM |
bd9c4c0
to
a8379bc
Compare
LGTM |
@aladdin-add Could you update the first post with the template information please? For example, is there an issue where this change was discussed? |
@ilyavolodin sorry for that, link to the issue #8449 , as it is WIP, so... |
22d4bff
to
25b3a38
Compare
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.
What about Object.defineProperties, plural?
What about the legacy __defineGetter__
functions?
Can you add some "invalid" tests with return statements nested inside callbacks, as well as some more tests with nested returns to ensure that there must always be an unconditional return?
tests/lib/rules/getter-return.js
Outdated
{ code: "var foo = { get bar(){if(bar) {return true;}} };", parserOptions, errors: [{ message: noLastReturnMessage, data: { name: "getter 'bar'" } }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return true;} return;} };", parserOptions, errors: [{ message: noReturnMessage, data: { name: "getter 'bar'" } }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return true;} ;} };", parserOptions, errors: [{ message: noLastReturnMessage, data: { name: "getter 'bar'" } }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return;} return true;} };", parserOptions, errors: [{ message: noReturnMessage, data: { name: "getter 'bar'" } }] }, |
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.
Why is this invalid? undefined
is a valid return value for a getter.
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.
should return undefined
explicitly. it is working in the same way as the rule array-callback-return
.
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.
Why should it? There's already a separate rule to handle that (consistent-return) if that's desired.
tests/lib/rules/getter-return.js
Outdated
{ code: "class foo { get bar(){} }", parserOptions, errors: [{ message: noReturnMessage, data: { name: "getter 'bar'" } }] }, | ||
{ code: "class foo { get bar(){return;} }", parserOptions, errors: [{ message: noReturnMessage, data: { name: "getter 'bar'" } }] }, | ||
{ code: "class foo { get bar(){if(bar) {return true;}} }", parserOptions, errors: [{ message: noLastReturnMessage, data: { name: "getter 'bar'" } }] }, | ||
{ code: "class foo { get bar(){if(bar) {return;} return true;} }", parserOptions, errors: [{ message: noReturnMessage, data: { name: "getter 'bar'" } }] }, |
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.
Why is this invalid? undefined
is a valid return value for a getter.
tests/lib/rules/getter-return.js
Outdated
// add object.defineProperty | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){return;}});", errors: [{ message: "Expected to return a value in method 'get'.", data: { name: "getter 'bar'" } }] }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){if(bar) {return true;}}});", errors: [{ message: "Expected to return a value at the end of method 'get'.", data: { name: "getter 'bar'" } }] }, | ||
{ code: "Object.defineProperty(foo, \"bar\", { get: function (){if(bar) {return;} return true;}});", errors: [{ message: "Expected to return a value in method 'get'.", data: { name: "getter 'bar'" } }] } |
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.
Why is this invalid? undefined
is a valid return value for a getter.
Is the goal of this rule to detect undefined returns, or to detect a lack of ReturnStatement nodes in a code path? My assumption/preference would be the latter, in line with @ljharb's questions on the PR. But if I missed some discussion/conversation around this, please let me know. |
@platinumazure @ljharb if we only detect a lack of ReturnStatement nodes in a code path, get foo(){
//...
return;
} will be valid. it is werid.. so this |
If the rule is to enforce an explicit return in getters, maybe it should be called: |
Or maybe we could add an option allowing users to choose which behavior
they want?
…On May 2, 2017 8:28 PM, "Reyad Attiyat" ***@***.***> wrote:
If the rule is to enforce an explicit return in getters, maybe it should
be called: no-implicit-getter-return
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8460 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AARWekHHs6H6ZRmFFuDpso5K_To1-zzFks5r19hPgaJpZM4M9014>
.
|
as it is very similar to |
I don't see why we can't add an option in the new rule, but if we definitely can't have an option in this initial implementation, then my preference would be for the rule to be as tolerant as possible and consider |
I mean if we add the option, maybe we should also add it to |
My opinion is that we should be consistent with |
fwiw, I use The stated purpose of the rule is "enforces that a return statement is present in property getters.". |
I'm in agreement with @ljharb. To me it feels like a bit of an overreach to disallow |
I'm not sure which option should be default. while writing |
|
25b3a38
to
2272cbf
Compare
LGTM |
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.
Thanks for the pull request! I just have a few small recommendations.
docs/rules/getter-return.md
Outdated
|
||
This rule has an object option: | ||
|
||
* `"allowImplicit": false` (default) disallows implicitly return a value. |
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 a return;
statement".
docs/rules/getter-return.md
Outdated
|
||
## 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 comment
The 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 comment
The 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.
lib/rules/getter-return.js
Outdated
codePath, | ||
hasReturn: false, | ||
shouldCheck: | ||
TARGET_NODE_TYPE.test(node.type) && |
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.
Nitpick: Since TARGET_NODE_TYPE
only has one type of function, can you change this to just be node.type === "FunctionExpression"
?
lib/rules/getter-return.js
Outdated
* @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 comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this function to something like getId
? I think the name getLocation
is misleading because it's not actually returning a location.
tests/lib/rules/getter-return.js
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Instead of using parserOptions
for each test, you can set defaults:
const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
Then you won't need to repeat parserOptions
in so many places.
LGTM |
LGTM |
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.
Looks good to me. Thanks!
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.
Mostly minor suggestions, though I also had a suggestion about one of the error messages.
lib/rules/getter-return.js
Outdated
) { | ||
context.report({ | ||
node, | ||
loc: getId(node, context.getSourceCode()).loc.start, |
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.
I'm a bit confused by this call, which passes context.getSourceCode()
as an argument but which does not match any formal parameter defined on the function. I think the second argument can probably be removed?
lib/rules/getter-return.js
Outdated
node, | ||
loc: getId(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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Expected {{name}} to always return a value."?
tests/lib/rules/getter-return.js
Outdated
{ 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems identical to line 33?
tests/lib/rules/getter-return.js
Outdated
|
||
// test obj: get, option: {allowImplicit: false} | ||
{ code: "var foo = { get bar() {} };", errors: [{ message: noReturnMessage }] }, | ||
{ code: "var foo = { get bar(){if(bar) {return true;}} };", errors: [{ message: noLastReturnMessage }] }, |
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.
Wondering if it would be less confusing to use if(baz)
, like in later tests, to avoid confusion around the variable name being the same as the property.
LGTM |
updated. and reorganized the tests, to avoid duplicate. :( |
LGTM |
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.
LGTM, thanks!
Thanks for contributing to ESLint! |
fixes #8449
What is the purpose of this pull request? (put an "X" next to item)
[ x] New rule (template)
What changes did you make? (Give an overview)
New rule: enforces that a return statement is present in property getters.
Example code:
Is there anything you'd like reviewers to focus on?
so the question is whether these test cases are fit for?
it is not a
getter
function, but naming a methodget
that does not return a value, is so confusing, causing an error may be somewhat reasonable.