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

New rule: getter-return #8460

Merged
merged 24 commits into from Jun 27, 2017

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Apr 14, 2017

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:

/*eslint getter-return: "error"*/

// Incorrect
class MyClass {
  get a() {
     this.retreiveVal('a');
  }
}

function MyConstructorFunction() {
  Object.defineProperty(this, 'a', {
    get: function () {
      this.retreiveVal('a');
    }
  });
}


// Correct
class MyClass {
  get a() {
     return this.retreiveVal('a');
  }
}

function MyConstructorFunction() {
  Object.defineProperty(this, 'a', {
    get: function () {
       return this.retreiveVal('a');
    }
  });
}

Is there anything you'd like reviewers to focus on?

  • I have confirmed the test coverage is 100%:

100% Statements 19/19 100% Branches 16/16 100% Functions 7/7 100% Lines 19/19

so the question is whether these test cases are fit for?

  • for code like this:
  var foo = { get: function () {bar();} };

it is not a getter function, but naming a method get that does not return a value, is so confusing, causing an error may be somewhat reasonable.

  • My English is not so good(as you can see ^-^). More attention can be attached to the docs and comments.

@mention-bot
Copy link

@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.

@eslintbot
Copy link

LGTM

@aladdin-add aladdin-add changed the title New: enforce-return-in-getter [wip]New: enforce-return-in-getter Apr 14, 2017
@aladdin-add aladdin-add reopened this May 1, 2017
@aladdin-add aladdin-add force-pushed the feature/enforce-return-in-get branch from bd9c4c0 to a8379bc Compare May 1, 2017 19:13
@eslintbot
Copy link

LGTM

@aladdin-add aladdin-add changed the title [wip]New: enforce-return-in-getter [wip]New: getter-return May 1, 2017
@ilyavolodin
Copy link
Member

@aladdin-add Could you update the first post with the template information please? For example, is there an issue where this change was discussed?

@aladdin-add
Copy link
Member Author

aladdin-add commented May 1, 2017

@ilyavolodin sorry for that, link to the issue #8449 , as it is WIP, so...
I will add more info later.

@aladdin-add aladdin-add force-pushed the feature/enforce-return-in-get branch 3 times, most recently from 22d4bff to 25b3a38 Compare May 2, 2017 12:11
@aladdin-add aladdin-add changed the title [wip]New: getter-return New: getter-return May 2, 2017
@aladdin-add aladdin-add changed the title New: getter-return New rule: getter-return May 2, 2017
Copy link
Sponsor Contributor

@ljharb ljharb left a 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?

{ 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'" } }] },
Copy link
Sponsor Contributor

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.

Copy link
Member Author

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.

Copy link
Sponsor Contributor

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.

{ 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'" } }] },
Copy link
Sponsor Contributor

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.

// 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'" } }] }
Copy link
Sponsor Contributor

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.

@platinumazure
Copy link
Member

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.

@aladdin-add
Copy link
Member Author

aladdin-add commented May 3, 2017

@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 return means return a value explicitly. the name getter-return seems not imply this, any suggestion?

@soda0289
Copy link
Member

soda0289 commented May 3, 2017

If the rule is to enforce an explicit return in getters, maybe it should be called: no-implicit-getter-return

@platinumazure
Copy link
Member

platinumazure commented May 3, 2017 via email

@aladdin-add
Copy link
Member Author

aladdin-add commented May 3, 2017

as it is very similar to array-callback-return, for consistency consideration, we better open a new issue to talk about adding the new option to array-callback-return . #8539

@platinumazure
Copy link
Member

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 return; as a valid return statement for the purpose of this rule.

@aladdin-add
Copy link
Member Author

aladdin-add commented May 3, 2017

I mean if we add the option, maybe we should also add it to array-callback-return.
updated the comment. sorry for inaccurate representation~~

@not-an-aardvark
Copy link
Member

My opinion is that we should be consistent with array-callback-return and raise an error for return;. Usually, when I see return; it's being used for control flow, not returning undefined.

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 3, 2017

fwiw, I use return exclusively over return undefined because undefined isn't a keyword, and isn't safe to rely on in non-app code (and return void 0 is ugly).

The stated purpose of the rule is "enforces that a return statement is present in property getters.". return; is a return statement. If I'm typing it, I mean it - the rule is meant to warn me when I omit an explicit return.

@kaicataldo
Copy link
Member

I'm in agreement with @ljharb. To me it feels like a bit of an overreach to disallow return;.

@aladdin-add
Copy link
Member Author

aladdin-add commented May 7, 2017

I'm not sure which option should be default.

while writing return; a developer may forget to return a value explicitly. So if not too much case return undefined in getter, I prefer this not to be default. you recon? @ljharb

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 7, 2017

return; is returning a value.

@aladdin-add aladdin-add force-pushed the feature/enforce-return-in-get branch from 25b3a38 to 2272cbf Compare May 8, 2017 20:50
@eslintbot
Copy link

LGTM

Copy link
Member

@not-an-aardvark not-an-aardvark left a 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.


This rule has an object option:

* `"allowImplicit": false` (default) disallows implicitly return a value.
Copy link
Member

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".


## When Not To Use It

If your project will not be using getter you do not need this rule.
Copy link
Member

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".

Copy link
Sponsor Contributor

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.

codePath,
hasReturn: false,
shouldCheck:
TARGET_NODE_TYPE.test(node.type) &&
Copy link
Member

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"?

* @param {ASTNode} node - A function node to get.
* @returns {ASTNode|Token} The node or the token of a location.
*/
function getLocation(node) {
Copy link
Member

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.

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 };
Copy link
Member

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.

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

Copy link
Member

@not-an-aardvark not-an-aardvark left a 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!

Copy link
Member

@platinumazure platinumazure left a 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.

) {
context.report({
node,
loc: getId(node, context.getSourceCode()).loc.start,
Copy link
Member

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?

node,
loc: getId(node, context.getSourceCode()).loc.start,
message: funcInfo.hasReturn
? "Expected to return a value at the end of {{name}}."
Copy link
Member

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.

Copy link
Member

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."?

{ code: "var get = function(){};" },
{ code: "var get = function(){ return true; };" },
{ code: "var foo = { bar: function(){} };" },
{ code: "var foo = { bar: function(){ return true; } };" },
Copy link
Member

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?


// 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 }] },
Copy link
Member

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.

@eslintbot
Copy link

LGTM

@aladdin-add
Copy link
Member Author

updated. and reorganized the tests, to avoid duplicate. :(

@eslintbot
Copy link

LGTM

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kaicataldo kaicataldo merged commit 8698a92 into eslint:master Jun 27, 2017
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: Enforce return in getter
9 participants