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 proposal: no-setter-return #12285

Closed
mdjermanovic opened this issue Sep 18, 2019 · 6 comments · May be fixed by rsumner868/librealsense2.1#1, rsumner868/librealsense#4, O330oei/node#4 or O330oei/node#11
Closed

Comments

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 18, 2019

Please describe what the rule should do:

no-setter-return disallows returning values from setter functions.

Reports return statement with an argument inside a setter. Targets setters in object literals, classes and property descriptors (4 well-known functions).

What category of rule is this? (place an "X" next to just one item)

[X] Warns about a potential error (problem)

Provide 2-3 code examples that this rule will warn about:

const foo = {
    set a(val) { 
        this._a = val; 
        return val; 
    }
}

class foo {
    set a(val) { 
        this._a = val * 100; 
        return this._a; 
    }
    static set b(val) {
        doSomething(val);
        return true;
    }
}

Object.defineProperty(obj, "foo", { 
    set(val) { 
        return 5 * val; 
    }
});

// also Object.defineProperties, Object.create, Reflect.defineProperty

Why should this rule be included in ESLint (instead of a plugin)?

Returning a value from a setter is either unnecessary or a possible error in logic (intention to somehow use the returned value when it's different).

As far as I know, even if there is an intention to use the value returned from a setter, it's simply impossible in ES. E.g., a.prop = foo, a.prop += foo, ++a.prop etc. do not use values returned from setters to evaluate the whole expression.

I think that just return; without value should be allowed, as it can be used for control flow.
Returning any value should be disallowed.

Are you willing to submit a pull request to implement this rule?

Yes.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 18, 2019

I support this! One thing to consider - is it worth creating a separate smaller rule, or to try to combine this with https://eslint.org/docs/rules/getter-return into a new rule (setter-getter-return)?

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Sep 19, 2019

Cases like this should be also reported, so it isn't technically just return statement:

Object.defineProperty(obj, "foo", { 
    set: val => val 
});
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Sep 19, 2019

I support this! One thing to consider - is it worth creating a separate smaller rule, or to try to combine this with https://eslint.org/docs/rules/getter-return into a new rule (setter-getter-return)?

That's an interesting idea! I didn't think about it. Both rules work with accessors, though require the opposite things, I'm not sure what would be better from a user's perspective.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 19, 2019

Just thought it was worth discussing as we're figuring it out!

Having thought about it a bit more, I think I'm leaning towards having a separate rule (i.e. using the current proposal). Smaller, focused rules tend to be easier to understand from a configuration perspective, though they might increase the overhead a little when they're first being added to the user's config (reading multiple rule doc pages, etc.).

@ilyavolodin ilyavolodin added accepted and removed evaluating labels Sep 25, 2019
@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Sep 25, 2019

@mdjermanovic Second example is also using return, it's just shortcut syntax, but it's still a return. So I think the name is fine.

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Sep 29, 2019

I'm working on this, should be ready soon.

mdjermanovic added a commit that referenced this issue Oct 1, 2019
@eslint eslint bot locked and limited conversation to collaborators May 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.